Array Interface and Categorical internals Refactor
State: closed

(edit post categorical-move)

Rebased on master. Summary of the changes from master:

    Added the ExtensionArray class
    Categorical subclasses ExtensionArray

    Implements the new methods for the interface (all private. No public API
    changes)

    Adapted the ExtensionDtype class to be the public ABC
    a. Subclass that with PandasExtensionClass that does non-interface things
    like reprs, caching, etc.
    b. All our custom dtypes inherit from PandasExtensionClass, so they implement
    the interface.
    Internals Changes:
    a. Added an ExtensionBlock. This will be a parent for our current custom
    blocks, and the block type for all 3rd-party extension arrays.
    Added a new is_extension_array_dtype method. I think this is nescessary
    for now, until we've handled DatetimeTZ.

This isn't really a test of whether extension arrays work yet, since we're
still using Categorical for everything. I have a followup PR that implements
an IntervalArray that requires additional changes to, e.g., the constructors
so that things work. But all the changes from core/internals.py required to
make that work are present here.

---


1. New class hierarchy in internals

Old:

```python
class CategoricalBlock(NonConsolidatableMixin, ObjectBlock):
    pass
```

new:

```python
class ExtensionBlock(NonConsolidatableMixin, Block):
   pass

class CategoricalBlock(ExtensionBlock):
    pass
```

Figuring out which methods of `ObjectBlock` were required on `CategoricalBlock`
wasn't trivial for me. I probably messed some up.

I think that eventually we can remove `NonConsolidatableMixin`, with the idea
that all non-consolidatable blocks are blocks for extension dtypes? That's true
today anyway.

Followup PRs:

1. Making `core/arrays/period.py` and refactoring `PeriodIndex`
2. Making `core/arrays/interval.py` and refactoring `IntervalIndex`
3. Adding docs and generic tests like https://github.com/pandas-dev/pandas/pull/19174/files#diff-e448fe09dbe8aed468d89a4c90e65cff for our interface (once it's stabilized a bit).

Labels:
name: Enhancement
name: Internals
description: Related to non-user accessible pandas implementation

Comments:
Hello @TomAugspurger! Thanks for updating the PR.





























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

##### Comment last updated on February 01, 2018 at 20:55 Hours UTC
@TomAugspurger for reviewing, did you need to change a lot to `Categorical` implementation itself? (because you moved it hard to see ..)
i would much prefer that moves occur separately from changes
I did keep the move in https://github.com/pandas-dev/pandas/commit/4b06ae4338adfa6efb47d02a95362a82e329e55e

All the non-move changes to categorical are in https://github.com/pandas-dev/pandas/pull/19268/commits/a9e097228fcd48d7cbb0a1ef6894bd805f5f84c2#diff-f3b2ea15ba728b55cab4a1acd97d996d
But I can split the move into its own PR as well.

One thing: do we have code for pickle compat on module changes? The old pickle files expect a `pandas.core..categorical.Categorical`. I threw a shim in there, but would be nice to remove it.


Splitting these PRs now (I'll make a new PR for the move and then rebase this one)
https://github.com/pandas-dev/pandas/pull/19269 for the move.
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/19268?src=pr&el=h1) Report
> Merging [#19268](https://codecov.io/gh/pandas-dev/pandas/pull/19268?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/fb3b23782534c925ab7158c59dcb32c8f8390d71?src=pr&el=desc) will **increase** coverage by `<.01%`.
> The diff coverage is `76%`.

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

```diff
@@            Coverage Diff             @@
##           master   #19268      +/-   ##
==========================================
+ Coverage   91.62%   91.62%   +<.01%     
==========================================
  Files         150      150              
  Lines       48726    48672      -54     
==========================================
- Hits        44643    44598      -45     
+ Misses       4083     4074       -9
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `89.99% <76%> (ø)` | :arrow_up: |
| #single | `41.74% <62.85%> (-0.01%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/19268?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/common.py](https://codecov.io/gh/pandas-dev/pandas/pull/19268/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvY29tbW9uLnB5) | `93.41% <ø> (+0.49%)` | :arrow_up: |
| [pandas/core/dtypes/dtypes.py](https://codecov.io/gh/pandas-dev/pandas/pull/19268/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZHR5cGVzL2R0eXBlcy5weQ==) | `96.08% <100%> (-0.04%)` | :arrow_down: |
| [pandas/core/arrays/\_\_init\_\_.py](https://codecov.io/gh/pandas-dev/pandas/pull/19268/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL19faW5pdF9fLnB5) | `100% <100%> (ø)` | :arrow_up: |
| [pandas/core/dtypes/common.py](https://codecov.io/gh/pandas-dev/pandas/pull/19268/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZHR5cGVzL2NvbW1vbi5weQ==) | `95.37% <100%> (+0.05%)` | :arrow_up: |
| [pandas/core/dtypes/base.py](https://codecov.io/gh/pandas-dev/pandas/pull/19268/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZHR5cGVzL2Jhc2UucHk=) | `47.61% <47.61%> (ø)` | |
| [pandas/core/arrays/base.py](https://codecov.io/gh/pandas-dev/pandas/pull/19268/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2Jhc2UucHk=) | `56.66% <56.66%> (ø)` | |
| [pandas/core/arrays/categorical.py](https://codecov.io/gh/pandas-dev/pandas/pull/19268/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2NhdGVnb3JpY2FsLnB5) | `94.74% <66.66%> (-1.04%)` | :arrow_down: |
| [pandas/core/internals.py](https://codecov.io/gh/pandas-dev/pandas/pull/19268/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW50ZXJuYWxzLnB5) | `95.05% <85.39%> (-0.42%)` | :arrow_down: |
| [pandas/errors/\_\_init\_\_.py](https://codecov.io/gh/pandas-dev/pandas/pull/19268/diff?src=pr&el=tree#diff-cGFuZGFzL2Vycm9ycy9fX2luaXRfXy5weQ==) | `92.3% <85.71%> (-7.7%)` | :arrow_down: |
| ... and [19 more](https://codecov.io/gh/pandas-dev/pandas/pull/19268/diff?src=pr&el=tree-more) | |

------

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

Rebased on master. Summary of the changes from master:

1. Added the ExtensionArray class
2. `Categorical` subclasses `ExtensionArray`
  - Implements the new methods for the interface (all private. No public API
    changes)
3. Adapted the `ExtensionDtype` class to be the public ABC
  a. Subclass that with `PandasExtensionClass` that does non-interface things
     like reprs, caching, etc.
  b. All our custom dtypes inherit from `PandasExtensionClass`, so they implement
     the interface.
4. Internals Changes:
   a. Added an `ExtensionBlock`. This will be a parent for our current custom
      blocks, and the block type for all 3rd-party extension arrays.
5. Added a new `is_extension_array_dtype` method. I think this is nescessary
   for now, until we've handled DatetimeTZ.

This isn't *really* a test of whether extension arrays work yet, since we're
still using `CategoricalBlock` for everything. I have a followup PR that implements
an `IntervalArray` that requires additional changes to, e.g., the constructors
so that things work. But all the changes from `core/internals.py` required to
make that work are present here.

@shoyer, @chris-b1, @wesm if you're interested in the interface, this is the PR it's being defined in, though we may have to make refinements later.

I have followup PRs ready for `IntervalArray` and `PeriodArray` that build on this. Docs and such will come in the `IntervalArray` PR, once we have publicly visible changes.
> I think that eventually we can remove NonConsolidatableMixin, with the idea
that all non-consolidatable blocks are blocks for extension dtypes? That's true
today anyway.

yes you should folk NonConsolidatedBlock into the ExtensionBlock infrastructure. maybe be slightly tricky right now as Sparse has some special casing.
> this is exactly the point, this diff is already way too big. let's do a pre-cursor PR to split Block and manger (IOW internals into 2 files).

 Some overnight-rebasing notwithstanding, I've got a branch ready that splits `internals` into `internals.blocks`, `internals.managers`, `internals.joins` (and except for import updates is pure cut/paste).  Would it be helpful to push that now?  I had planned to wait because it will make a ton of rebasing necessary in other PRs.
Thanks for the comments, tried to address most inline, going back through now to see if I missed any.

> Some overnight-rebasing notwithstanding, I've got a branch ready that splits internals into internals.blocks, internals.managers, internals.joins (and except for import updates is pure cut/paste). Would it be helpful to push that now? I had planned to wait because it will make a ton of rebasing necessary in other PRs.

Doesn't matter for me. I'm only touching `Blocks` so the git conflicts shouldn't be terrible. I don't recall what else is outstanding. Either way, the diff here will be identical.

> Agreed on both accounts, though I think getting reductions, cumulative ops, and numeric ops to all work properly on extension arrays will require some careful thought an quite a bit of work

This is the bit that most closely matches my comparative advantage.  The upshot is that we can adapt the existing FooIndex implementations in most cases, but need to be careful not to let the set of Array methods transferred from Index snowball.
I quickly tried this branch with geopandas, and it turns out the `GeometryArray` we have there already almost entirely satisfies the interface (apart from the dtype and some of the block related methods). 
I could also put this in a Series and do some basic operations (get item, equality comparison, repr, concat)

First things I noticed:

- I think `__iter__` is not strictly needed to be included in the interface (if you have len and getitem, it will already be iterable)
- `s.isna()` is not working for me (but not sure if it was already meant to be working)
- `align` results in an ObjectBlock (and also eg `pd.merge`, but that is probably related). `align` is something that is working with my GeometryBlock, so I would need to check which part of it is that is making this work compared to ExtensionBlock.
- the `ExtensionDtype.name` is not used in eg `df.dtypes` (although it is used in the series repr). But given that, I think it would be good to have a default `__repr__` implementation that just returns the `name` ?


Thanks for trying it out @jorisvandenbossche, that's encouraging.

> s.isna() is not working for me (but not sure if it was already meant to be working)

That'll require https://github.com/TomAugspurger/pandas/compare/pandas-array-interface-3...TomAugspurger:pandas-array-upstream+interval#diff-ff8364cee9a3e1ef3a3825cb2cdd26d8 (that's my interval PR). Personally, I think it makes sense to move those changes to this PR, but I'm trying to keep things in small chunks.



@jreback what are your thoughts here on the way forward? I would rather try to merge this quickly, with the idea to do follow-up PRs. 
(the problem is that this PR will never be perfect, unless it does at once a full conversion of the other internal types and edits throughout the code-base to use this, but that makes it then a huge diff that is impossible to review)
FYI, I'll be without internet, but working on this, for most of tomorrow.
will have a look later today
> I would rather start with a minimal implementation of ExtensionBlock. Then fix what needs to be fixed, rather than simply move it to EB.

@TomAugspurger will be able to give more details, but I think that this is actually what he has been doing. Adding things to ExtensionBlock to make things working.
> @TomAugspurger will be able to give more details, but I think that this is actually what he has been doing. Adding things to ExtensionBlock to make things working.

Correct.
For reference, the changes in https://github.com/pandas-dev/pandas/pull/19268#pullrequestreview-92638262 turned up an issue in master with `SparseArray` blocks being consolidated,

On master:

```python
In [1]: import pandas as pd; import numpy as np

In [2]: sdf = pd.SparseDataFrame([[np.nan, 1], [2, np.nan]])

In [3]: sdf._data.blocks
Out[3]:
(SparseBlock: slice(0, 1, 1), 1 x 2, dtype: float64,
 SparseBlock: slice(1, 2, 1), 1 x 2, dtype: float64)

In [4]: sdf[0][1] = 0
/Users/taugspurger/Envs/pandas-dev/bin/ipython:1: SettingWithCopyWarning:
A value is trying to be set on a copy of a slice from a DataFrame

See the caveats in the documentation: http://pandas.pydata.org/pandas-docs/stable/indexing.html#indexing-view-versus-copy
  #!/Users/taugspurger/Envs/pandas-dev/bin/python3.6

In [5]: sdf._data.blocks
Out[5]: (SparseBlock: slice(1, 2, 1), 1 x 2, dtype: float64,)
```

On this branch the blocks are (correctly) not consolidated. Adding a test to confirm.
Looking into performance things now. `isinstance` checks are a decent amount slower

```python
s = pd.Series()
%timeit isinstance(s, pd.api.types.CategoricalDtype)
```

Master: `281 ns ± 7.39 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)`
Branch: `25.5 µs ± 4.23 µs per loop (mean ± std. dev. of 7 runs, 10000 loops each)`

This add up since we do a lot of `isinstance` calls via the `is_*` methods.

Looking into overriding `__instancecheck__` without breaking things.

Alternatively, we could just replace `abc.abstractmethod`s with implementations that raise, `NotImeplementedError`. We lose the ability to register virtual subclasses, but oh well.
w/r/t isinstance checks there's an abandoned #17599 PR that moved the ABCFoos to cython, got a boost.  IIRC the existing `isinstance(item, ABCFoo)` check is about 2x slower than `isinstance(item, Foo)`.
This is for python's `abc.ABCMeta`, which seems to be 100x slower than that even (though it does a lot more).
- Moved AbstractMethodError to `pandas.errors` in https://github.com/pandas-dev/pandas/pull/19268/commits/fc688a56e20e62e6b7b806aedc4a4a760af079b1 to avoid a circular depedency
- Modified AbstractMethodError to have proper error messages for classmethods and porperties in https://github.com/pandas-dev/pandas/pull/19268/commits/fbc846644be3e97314b203dd3eab9a36ef50a274
- Using AbstrctMethodError instead of `abc.ABCMeta` in https://github.com/pandas-dev/pandas/pull/19268/commits/030bb194e523b24442db77c8cbabe16315880c73

Rerunning the ASV now, but I think this initial PR ready.
You can always use "type(...) is" if necessary. That avoids looking through
the class hierarchy so it's generally must faster.

On Wed, Jan 31, 2018 at 8:52 AM Tom Augspurger <notifications@github.com>
wrote:

> Looking into performance things now. isinstance checks are a decent
> amount slower
>
> s = pd.Series()%timeit isinstance(s, pd.api.types.CategoricalDtype)
>
> Master: 281 ns ± 7.39 ns per loop (mean ± std. dev. of 7 runs, 1000000
> loops each)
> Branch: 25.5 µs ± 4.23 µs per loop (mean ± std. dev. of 7 runs, 10000
> loops each)
>
> This add up since we do a lot of isinstance calls via the is_* methods.
>
> Looking into overriding __instancecheck__ without breaking things.
>
> —
> You are receiving this because you were mentioned.
>
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/19268#issuecomment-361995800>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABKS1nTZA6skY30TN5O3KhKdIl2UMG9Oks5tQJo4gaJpZM4Rf8hq>
> .
>

ASV is good now that we aren't using `ABCMeta`. Personally, I'm OK with that. We can provide base classes to test against like https://github.com/TomAugspurger/pandas/blob/pandas-array-upstream%2Binterval/pandas/tests/extension_arrays/base.py if library authors are interested.
The practical consequence of no longer using abc.ABCMeta, is it only that you now cannot register a class but need to actually subclass? (which doesn't seem a big problem?)
That, and library author-wise, it's now a bit harder to validate that your extension array implements everything. Now it's possible to instantiate an `ExtensionArray` that doesn't implement the interface. With an ABC that would raise an exception.