ENH: Allow storing ExtensionArrays in containers
State: closed

Followup number 1 to #19268 

This adds changes to frame, series, dtypes, etc. (all the places other that `core/internals.py`) to support 3rd party extension arrays.

These changes were developed to support IntervalArrays (https://github.com/TomAugspurger/pandas/compare/pandas-array-upstream+fu1...TomAugspurger:pandas-array-upstream+fu1+interval separate PR) and https://github.com/continuumio/pandas-ip (external).

Also closes https://github.com/pandas-dev/pandas/issues/19585

Labels:
name: Enhancement
name: Dtype Conversions
description: Unexpected or buggy dtype conversions
name: Internals
description: Related to non-user accessible pandas implementation

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

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

```diff
@@            Coverage Diff             @@
##           master   #19520      +/-   ##
==========================================
+ Coverage   91.58%   91.63%   +0.04%     
==========================================
  Files         150      150              
  Lines       48892    48931      +39     
==========================================
+ Hits        44780    44840      +60     
+ Misses       4112     4091      -21
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.01% <94.79%> (+0.05%)` | :arrow_up: |
| #single | `41.77% <61.45%> (-0.02%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/19520?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/dtypes/common.py](https://codecov.io/gh/pandas-dev/pandas/pull/19520/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZHR5cGVzL2NvbW1vbi5weQ==) | `94.16% <ø> (-1.22%)` | :arrow_down: |
| [pandas/core/dtypes/dtypes.py](https://codecov.io/gh/pandas-dev/pandas/pull/19520/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZHR5cGVzL2R0eXBlcy5weQ==) | `95.9% <ø> (-0.19%)` | :arrow_down: |
| [pandas/core/indexing.py](https://codecov.io/gh/pandas-dev/pandas/pull/19520/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhpbmcucHk=) | `93.11% <ø> (ø)` | :arrow_up: |
| [pandas/core/indexes/base.py](https://codecov.io/gh/pandas-dev/pandas/pull/19520/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9iYXNlLnB5) | `96.36% <100%> (-0.09%)` | :arrow_down: |
| [pandas/core/dtypes/base.py](https://codecov.io/gh/pandas-dev/pandas/pull/19520/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZHR5cGVzL2Jhc2UucHk=) | `91.66% <100%> (+44.04%)` | :arrow_up: |
| [pandas/core/internals.py](https://codecov.io/gh/pandas-dev/pandas/pull/19520/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW50ZXJuYWxzLnB5) | `95.53% <100%> (+0.47%)` | :arrow_up: |
| [pandas/core/dtypes/generic.py](https://codecov.io/gh/pandas-dev/pandas/pull/19520/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZHR5cGVzL2dlbmVyaWMucHk=) | `100% <100%> (ø)` | :arrow_up: |
| [pandas/core/algorithms.py](https://codecov.io/gh/pandas-dev/pandas/pull/19520/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYWxnb3JpdGhtcy5weQ==) | `94.18% <100%> (+0.01%)` | :arrow_up: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/19520/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `97.23% <100%> (ø)` | :arrow_up: |
| [pandas/core/arrays/base.py](https://codecov.io/gh/pandas-dev/pandas/pull/19520/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2Jhc2UucHk=) | `75.67% <100%> (+15.67%)` | :arrow_up: |
| ... and [21 more](https://codecov.io/gh/pandas-dev/pandas/pull/19520/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/19520?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/19520?src=pr&el=footer). Last update [cd1b168...ea5562b](https://codecov.io/gh/pandas-dev/pandas/pull/19520?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

Hello @TomAugspurger! Thanks for updating the PR.



























































Cheers ! There are no PEP8 issues in this Pull Request. :beers: 

##### Comment last updated on February 22, 2018 at 16:31 Hours UTC
Added a test-case "JSONArray" that stores mappings.
Meta-discussion, what methods should we consider "necessary" to work. Currently we have

- constructors
- slicing (`__getitem__`, `loc`, `iloc`)
- reindex
- isna
- concat
- memory_usage

My current TODO list includes

- [x] dropna (will do here)
- [ ] fillna
- [x] value_counts
- [ ] groupby (not too hard, but separate PR I think). This include algos like factorize and unique.
- [ ] reductions (separate PR)
- [ ] ops (separate PR)
- [ ] cumulative ops (separate PR)

Am I missing anything obvious? (cc @shoyer, @chris-b1 if you have opinions on this point).
For any kind of ops, my "minimum" is a (decent) error message for now. 
Although with my block-based implementation in geopandas, I had working comparison ops (at least equality, as greater/smaller than doesn't make sense for geometries), and only errors for arithmetic or reducing ops.

I think `value_counts` will also need some kind of factorize/unique implementation?

Further:

- `astype`: for now a good error message, but do we want a way to enable this in the future? (an `astype` on ExtensionArray ?)


> I think value_counts will also need some kind of factorize/unique implementation?

A *good* one, yes :) But for now the only correctness issue is NA-handling. For now we can (maybe) drop missing values, convert to an object array, and then do `value_counts`.
@jorisvandenbossche https://github.com/pandas-dev/pandas/pull/19520#discussion_r166605819 should not be a goal at all. there are already too many special cases. sure if its possible great GeoPandas will simply have to adjust to this.
I agree it should not be a goal. But if all it takes is one `getattr` for now (which I actually think is not needed, see latest comment above), then that would make my life easier.
It should also not take too much time to adapt to the new interface, so it shouldn't be needed for long. At that time, I would actually just remove the full `test_external_blocks` tests.
Fixed the stuff with extension blocks. The only change needed to the test from the old way was to inherit from `NonConsolidatableMixin` and `Block`. Before my first PR, it was just `NonConsolidatableMixin`. I see that geopandas already inherits from both, so we should be good to go.


A thorny issue: What to do with comparison methods (cc @jbrockmendel for thoughts).

Neither the Python 2 or Python 3 object defaults are appropriate, so I think we should make it abstract or provide a default implementation.

We *should* be able to do a default implementation for the simple case of `binop(extension_array, extension_array)` by

1. masking nulls in either `self | other`
2. cast self / other to ndarrays
3. compare and apply null mask

How much coercion do we want to attempt? I would vote to stay simple and only attempt a comparison if

1. `other` is an instance of the same `ExtensionArray` type
2. `other` is a scalar of `self.dtype.type`.

Otherwise we return `NotImplemented`. Subclasses can of course choose more or less aggressive coercion rules.

Thoughts?
> We should be able to do a default implementation for the simple case of binop(extension_array, extension_array) by
> ...
> 2. cast self / other to ndarrays

The problem with this as default behaviour is that this can be a big performance bottleneck in case casting to an array is expensive. And of course, something like `Series(extension_array) + 1` might still raise the desired error because `extension_scalar + 1` will raise this error. But I think it can be annoying that at lot of ops first need to do a lot of computations (casting to array) before erroring properly.

So in the ideal world, for me as an extension author, I would want to be able to specify for certain ops that they are not possible (an error is raised without casting to array and trying) and for certain ops provide my own fast implementation (that does not need casting to an array).
If we want to do something like what I want, I suppose the most logical solution is to fall back to `ExtensionArray.__op__` to see if that is defined / works, and wrapping the result of it back into Series with the correct index?
> So in the ideal world, for me as an extension author,

Isn't that the world we live in? :) If your class doesn't implement `__add__` then you define `__add__` to raise a `TypeError`?

Seeing your second post now, I should clarify that I'm just talking about ExtensionArray ops at the moment, not `Series[ExtensionArray]`. Your suggestion of the Series ops dispatching to `ExtensionArray` ops makes since, but will require more work in `nanops.py` to support.

I'm tempted to shove all that into a separate PR, but can go either way.
Ah, yes, I thought you were talking about the `Series([extensionarray])` case.

> I'm tempted to shove all that into a separate PR, but can go either way.

Yes, for sure. Erroring for all is fine for now,  let's first get this merged.
> Isn't that the world we live in? :) If your class doesn't implement __add__ then you define __add__ to raise a TypeError?

Yep, that's fine for me. Then we should add that to the documentation of ExtensionArray that it is *recommended* (IMO) to implement those ops (even if it is to raise a TypeError) to prevent casting to array of scalars.
Just a question: how is this discussion related to this PR (I thought here it was about being able to store it inside a Series, but if you say for Series ops we do not yet dispatch to the underlying extension array?)
It's not strictly related to this PR, I just happened to write some tests for it without really thinking :) I'll split those off to the next PR.
> A thorny issue: What to do with comparison methods

I've been planning on moving the Index/Series arithmetic/comparison ops into Array subclasses, so that `Series.__op__(other)` could just wrap `self._data.blocks[0].__op__(other)` and we get consistency between Series/DataFrame ops for free.

_Most_ of the comparison ops are ready to make that jump; I can prioritize it if getting that done quickly will help you out.

One caveat I'm concerned about is that the existing implementations in the Index subclasses have gotten tangled up with a bunch of unrelated Index machinery.  Ideally I'd like these operations to go into self-contained mixin classes that rely on constructor methods, but are independent of slicing/concat/reindex/dropna/... mentioned above.
@jbrockmendel Tom opened https://github.com/pandas-dev/pandas/issues/19577 for the ops discussion, so maybe best post that there
Latest commit will fail till I get boolean setitem working. Hopefully everything else passes.

> We could maybe think about encouraging (or at least make it possible, which you already have been doing for the internal exampel tests) for extension authors to subclass those tests so they can make sure everything passes

That's what I've been doing for my IPAddress tests, everything seems to work fine. We'll just need to document it.
Dropna is now working. (side note, we'll now avoid materializing the values for a `DataFrame` with a single column of `Categorical` dtype on `count` and `dropna`).
Quick status update:

- I've split ExtensionBlock.setitem to https://github.com/pandas-dev/pandas/issues/19599 (xfailing the tests for now)
- I'm going to pick up https://github.com/pandas-dev/pandas/pull/19558 again, remove all the index vs. Series special casing / `_as_best_array` stuff

Then this should be ready for another round of reviews when people have time.
I tested this out with GeometryArray, some feedback (I am putting everything here, and not each should be fixed in this PR, some could also be put in a general to do list):

- easy to implement (based on what there already was of course), and subclassed the general dtype/array tests and they all pass, only xfailed the setitem ones (we don't yet support setitem) and the value_counts (shapely geometry objects are not hashable, so the ndarray materializing fallback does not work)
- It seems we could have a default implementation for `ExtensionDtype.construct_from_string` ? (I now just copy pasted from the decimal example, and I think json example also has the same basic one)
- The requirement that GeometryArray(..) needs to be able to accept instances of itself, makes it a little bit more complex (but certainly no problem if needed, I just need to think a bit about the memory management / gc). But my question is maybe: which semantic do we (pandas) expect here? That it just returns that same instance, or that it takes kind of a (deep) copy?
- Is `ExtensionArray._fill_value` actually used somewhere? Because we have two different notions of fill values: the internal one for the underlying data (just a 0 pointer), and the external one we use when unboxing (None). But since the first is internal, I suppose whatever pandas needs it for, it should be the second? 
  Anyhow, there is no test failing with or without this property. Only one test relied on it for the construction of the expected result. But that could also indicate that this property is not really needed for the interface?
- Numerical operations give AssertionError, instead of TypeError (although the correct TypeError is somewhere in the traceback). Eg consider `s` a series with extension array: `s + 1` gives assertion error
- `s == scalar` already works (eg ``s == s[1]``), so could add a test for it. But this is probably because it is materializing the array? 
- We should eventually make the "AttributeError: 'GeometryArray' object has no attribute '_reduce'" error a bit more user friendly
- `s.astype(object)` or `s.astype(str)` worked for geopandas (because I did some custom code for that in the block), to have this here, we might need to extend the interface with an `astype` method?
- `Series.where` does not yet preserve extension array
- We need to special case `.astype(object)` I think, irregardless of a more general solution for ``astype``. Because eg Series.apply (and suppose also map) relies on `values = self.astype(object).values`. Or of course we could special case extension types there, but maybe this pattern is used more throughout the code base.
- `series.align(dataframe)` where the series contains an extension array fails with "data type not understood" (somewhere deep in the reindexing machinery)

I did not yet fully integrate it in the rest of geopandas, so probably more will come :-)


> It seems we could have a default implementation for ExtensionDtype.construct_from_string ? (I now just copy pasted from the decimal example, and I think json example also has the same basic one)

https://github.com/pandas-dev/pandas/issues/19600

> But my question is maybe: which semantic do we (pandas) expect here? That it just returns that same instance, or that it takes kind of a (deep) copy?

I'll take a look and clarify. I suspect that just returning the same instance is fine, but perhaps surprising compared to other container types?

> Is ExtensionArray._fill_value actually used somewhere?

Not sure... I want to nail this down too. Pandas uses `fill_value` for two meanings too. Externally, `fill_value` is typically for "Fill missing values with this." Internally, on Block, `fill_value` is the missing value sentinel.

> we might need to extend the interface with an astype method?

Perhaps, the idea being that `ExtensionArray.astype(object)` returns an ndarray of your `.dtype.type` objects? Does `np.array(extension_array, dtype=object)` suffice? We would then just have to say that the default `ExtensionArray.astype` only astypes to object.

> Series.where

Will take a look, and push if it seems complicated :)

> series.align(dataframe)

Will fix this here
Latest push contains the changes in https://github.com/pandas-dev/pandas/pull/19558. Just making sure the CI passes, put the other PR will ahve to be merged first.
Ugh, https://github.com/pandas-dev/pandas/pull/19520/commits/a312ba5c59c2e96854a286bde74d7fd4562afbf8, which set `PANDAS_DEPRECATION_WARNING` to work around https://github.com/pandas-dev/pandas/issues/19585 turned up some deprecation warnings we were ignoring.

I'll fix those in a separate PR, and then push this again.
Latest commit should fix up https://github.com/pandas-dev/pandas/issues/19585.
I can split the test parquet stuff off to a different issue, but I'd prefer to leave it hear since (for whatever reason) the issue in #19585 manifests here and not on master.
@TomAugspurger Is this ready for a new review?
Yeah, sorry. The only changes I'm making at the moment specifying exact warnings to catch in the parquet tests to fix https://github.com/pandas-dev/pandas/issues/19585, which is interacting badly with another test in `test_internals.py`
But apart from that, have there been changes since I last reviewed? (I suppose you integrated the `_values` PR?)