API: Added ExtensionArray constructors
State: closed

Adds two new (private, but part of the interface) constructors to EA.

Closes #19906

Labels:
name: API Design
name: ExtensionArray
description: Extending pandas with custom dtypes or arrays.

Comments:
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/19913?src=pr&el=h1) Report
> Merging [#19913](https://codecov.io/gh/pandas-dev/pandas/pull/19913?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/1e4c50a56f7e953ab84308f000dff6fc1ac71171?src=pr&el=desc) will **increase** coverage by `0.01%`.
> The diff coverage is `75%`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/19913/graphs/tree.svg?width=650&height=150&src=pr&token=eZ4WkYLtcO)](https://codecov.io/gh/pandas-dev/pandas/pull/19913?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #19913      +/-   ##
==========================================
+ Coverage   91.66%   91.67%   +0.01%     
==========================================
  Files         150      150              
  Lines       48969    49049      +80     
==========================================
+ Hits        44886    44966      +80     
  Misses       4083     4083
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.05% <75%> (+0.01%)` | :arrow_up: |
| #single | `41.79% <50%> (-0.06%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/19913?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/arrays/categorical.py](https://codecov.io/gh/pandas-dev/pandas/pull/19913/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2NhdGVnb3JpY2FsLnB5) | `95.06% <100%> (+0.01%)` | :arrow_up: |
| [pandas/core/arrays/base.py](https://codecov.io/gh/pandas-dev/pandas/pull/19913/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2Jhc2UucHk=) | `74.35% <50%> (-1.32%)` | :arrow_down: |
| [pandas/core/common.py](https://codecov.io/gh/pandas-dev/pandas/pull/19913/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvY29tbW9uLnB5) | `92.04% <0%> (-0.92%)` | :arrow_down: |
| [pandas/core/ops.py](https://codecov.io/gh/pandas-dev/pandas/pull/19913/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvb3BzLnB5) | `96.33% <0%> (-0.61%)` | :arrow_down: |
| [pandas/core/series.py](https://codecov.io/gh/pandas-dev/pandas/pull/19913/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvc2VyaWVzLnB5) | `93.85% <0%> (-0.56%)` | :arrow_down: |
| [pandas/core/indexes/datetimelike.py](https://codecov.io/gh/pandas-dev/pandas/pull/19913/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9kYXRldGltZWxpa2UucHk=) | `96.83% <0%> (-0.5%)` | :arrow_down: |
| [pandas/io/formats/style.py](https://codecov.io/gh/pandas-dev/pandas/pull/19913/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2Zvcm1hdHMvc3R5bGUucHk=) | `96.03% <0%> (-0.19%)` | :arrow_down: |
| [pandas/core/groupby.py](https://codecov.io/gh/pandas-dev/pandas/pull/19913/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ3JvdXBieS5weQ==) | `92.14% <0%> (-0.18%)` | :arrow_down: |
| [pandas/core/sparse/series.py](https://codecov.io/gh/pandas-dev/pandas/pull/19913/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvc3BhcnNlL3Nlcmllcy5weQ==) | `95.2% <0%> (-0.05%)` | :arrow_down: |
| [pandas/core/indexes/timedeltas.py](https://codecov.io/gh/pandas-dev/pandas/pull/19913/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy90aW1lZGVsdGFzLnB5) | `91.17% <0%> (-0.04%)` | :arrow_down: |
| ... and [15 more](https://codecov.io/gh/pandas-dev/pandas/pull/19913/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/19913?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/19913?src=pr&el=footer). Last update [1e4c50a...8406f11](https://codecov.io/gh/pandas-dev/pandas/pull/19913?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

this is just API bloat. what is the point here?
> what is the point here?

Not imposing things on the subclasses `__init__`. I believe @jorisvandenbossche already had issues with the old design in geopandas. You could imagine a class whose `__init__` wants to use keyword only arguments.
@jreback I think this is actually very much in line with how we are doing it currently internally (having constructors as class methods). The only difference here is that we make a constructor class method that is more specific in its behaviour, which I think is a plus both for internal usage as for the extension author to clearly communicate what we exactly expect from the constructor (in this case, that it takes a sequence of scalars). 
I think limiting the expected behaviour is a good thing. And we can call it `_constructor` to be 'consistent' with other places in the code base, but given its specific behaviour, I think embedding that in the name is better (we could also use `_constructor_from_scalars` if you like that more). 
> Not imposing things on the subclasses __init__. I believe @jorisvandenbossche already had issues with the old design in geopandas. You could imagine a class whose __init__ wants to use keyword only arguments.

To add: it is not *really* a problem for geopandas to support this in its `__init__.py`. It was just that we initially did not choose to do this, and I needed to adapt the `__init__;py` in my branch that tests the extension array interface (and for that I just needed to figure out some memory management stuff that Matthew originally did). But now it is coded, it is also not complicated to support it. 
But that said, I still think it is useful for the extension interface to be very explicit about what we expect from certain constructors, so having a separate one for constructing from scalars is IMO a good idea (giving more flexibility about the main `__init__` to the extension author). 
I think `_from` already indicates pretty well that it's a constructor. `_constructor_from` feels redundant.
> this is just API bloat. what is the point here?

@jreback does the comments https://github.com/pandas-dev/pandas/pull/19913#issuecomment-368785002 and https://github.com/pandas-dev/pandas/pull/19913#issuecomment-368728194 make sense? This is blocking https://github.com/pandas-dev/pandas/pull/19869 and factorizing EAs.
> @jreback I think this is actually very much in line with how we are doing it currently internally (having constructors as class methods). The only difference here is that we make a constructor class method that is more specific in its behaviour, which I think is a plus both for internal usage as for the extension author to clearly communicate what we exactly expect from the constructor (in this case, that it takes

I am not opposing this motivation. I agree with it. But you going off with new names w/o a very clear purpose. We *already* have somewhat of a mess, e.g. ``_shallow_copy`` and ``_simple_new`` in the Index space, and ``_constructor`` in the Series/DataFrame space. So you are creating something new here. Let's stick with the current ones. Then if you want to change they should be changed for everything. Forging yet another path is just going creating technical debt.

This is my basic hesitiion with things like creating a new ``.fillna`` impl as well. Sure adding code is exciting as it can be very clean. But the existing code base is simply being let behind, creating an *even bigger* issue for later.


> But you going off with new names w/o a very clear purpose

Sorry I wasn't clearer then. I thought the convention was that `self._constructor` was a property that returns a class. The proposal here is for a classmethod. We have places in the code that do things expect `_constructor` to be a class like `self._constructor.__name__`.

I don't really care whether we call it `_constructor` or `_from_scalars`. But they're different things so I'd prefer to have different names. Your call.
@jreback ping
I dont' see a purpose of adding this, nor are you showing utility.
I think all our long comments are trying to explain the utility. And remember it was you who asked to add a constructor class method in the first place. The last question of Tom (the "your call") was mainly about the *name*. 
Unless you disagree about adding a constructor like this that is limited in functionality (a specific one to construct from scalars) at all. But then please be more specific about what you want to see.
@jorisvandenbossche simply need ``_constructor`` as a class property. You don't need a class constructor. We do no use them anywhere else and it is a major break.
> simply need `_constructor` as a class property

Ah, I see the misunderstanding. That's not an option since it relies on `self._constructor(data)` being valid. We don't know that the subclass's `__init__` takes a sequence of scalar types.
Trying to summarize the arguments here.

Jeff wants a property `_constructor` that constructs the new EA. This would have the same requirements used in pandas today

1. `self._constructor` is a class
2. The class can be constructed with a single positional argument.

My objection to that is that I don't want to put any restrictions a 3rd party EAs `__init__` method. For example, an IPArray might be constructable in a few ways

1. sequence of `ipaddress` objects
2. sequence of integers
3. a bytestring

I want to support all these, but not with pandas' "magical" constructors of just inferring things. So I use keyword only arguments.

```python
class IPArray(ExtensionArray):
    """Holder for IP Addresses."""
    def __init__(self, *, addresses=None, ints=None, bytestring=None):
        from .parser import _to_ip_array

        if addresses is not None:
            values = _to_ip_array(addresses)
        elif ints is not None:
            ...
        else:
            values = self._from_bytes(addresses)
        self.data = values
```

We can't use pandas usual `_constructor` route

```python
@property
def _constructor(self):
    return IPArray
```

What's *possible* is to use a property that returns a function

```python
    @property
    def _constructor(self):
        def fn(values):
            return type(self)(addresses=values)
        return fn
```

Now, the question is, do we *want* this? This now puts restrictions on uses of `self._constructor` within pandas.

1. We need to ensure we're just using `self._constructor` as a callable. We can't treat it as a class to access additional attributes off it.
2. We're conflating two similar things:
  a.) A method for creating a data structure from a very specific type (sequence of your scalar type)
  b.) A method for creating a data structure from anything.

Again, I don't feel too strongly about this, but I do prefer `_from_scalars` since it's clearer that this is a very specific use case that will be limited to just `arrays/base.py`.
I think the problem with this is the name. I understand and am ok with a class method that allows an extension array author to do internal construction with a class method, eg.. ``_constructor_from_default(....)``, The ``_from_scalars`` is just confusing, because you are most often costructing from an ``ndarray`` or a ``list`` (e.g. after an internal pandas operation) of *scalars*.  

maybe even ``_constructor_from_sequence`` is more appropriate

I don't really mind the exact name, but would like to keep the word 'scalar' in it, as that is essential (it is not just accepting an array/list, but an array/list of scalars). 
The longest and complete name would be `_constructor_from_sequence_of_scalars` ... so but now we need to find the best compromise between length and clarity :-)
and I would like to remove scalars, yes its *from* a sequence of scalars, the relevant part is that its a list of *something*.

``_constructor_from_sequence`` works for me.
All green. merging later today.
Thanks!