ENH: add groupby & reduce support to EA
State: closed

closes #21789
closes #22346
xref #22865 

Labels:
name: Numeric Operations
description: Arithmetic, Comparison, and Logical operations
name: ExtensionArray
description: Extending pandas with custom dtypes or arrays.

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

 - There are no PEP8 issues in the file [`pandas/conftest.py`](https://github.com/jreback/pandas/blob/08a2add69fa856542be4b58d660dfbf64d170bff/pandas/conftest.py) !

 - There are no PEP8 issues in the file [`pandas/core/arrays/base.py`](https://github.com/jreback/pandas/blob/08a2add69fa856542be4b58d660dfbf64d170bff/pandas/core/arrays/base.py) !

 - There are no PEP8 issues in the file [`pandas/core/arrays/categorical.py`](https://github.com/jreback/pandas/blob/08a2add69fa856542be4b58d660dfbf64d170bff/pandas/core/arrays/categorical.py) !

 - There are no PEP8 issues in the file [`pandas/core/arrays/integer.py`](https://github.com/jreback/pandas/blob/08a2add69fa856542be4b58d660dfbf64d170bff/pandas/core/arrays/integer.py) !

 - There are no PEP8 issues in the file [`pandas/core/series.py`](https://github.com/jreback/pandas/blob/08a2add69fa856542be4b58d660dfbf64d170bff/pandas/core/series.py) !

 - There are no PEP8 issues in the file [`pandas/tests/arrays/test_integer.py`](https://github.com/jreback/pandas/blob/08a2add69fa856542be4b58d660dfbf64d170bff/pandas/tests/arrays/test_integer.py) !

 - There are no PEP8 issues in the file [`pandas/tests/dtypes/test_common.py`](https://github.com/jreback/pandas/blob/08a2add69fa856542be4b58d660dfbf64d170bff/pandas/tests/dtypes/test_common.py) !

 - There are no PEP8 issues in the file [`pandas/tests/extension/arrow/test_bool.py`](https://github.com/jreback/pandas/blob/08a2add69fa856542be4b58d660dfbf64d170bff/pandas/tests/extension/arrow/test_bool.py) !

 - There are no PEP8 issues in the file [`pandas/tests/extension/base/__init__.py`](https://github.com/jreback/pandas/blob/08a2add69fa856542be4b58d660dfbf64d170bff/pandas/tests/extension/base/__init__.py) !

 - There are no PEP8 issues in the file [`pandas/tests/extension/base/groupby.py`](https://github.com/jreback/pandas/blob/08a2add69fa856542be4b58d660dfbf64d170bff/pandas/tests/extension/base/groupby.py) !

 - There are no PEP8 issues in the file [`pandas/tests/extension/base/reduce.py`](https://github.com/jreback/pandas/blob/08a2add69fa856542be4b58d660dfbf64d170bff/pandas/tests/extension/base/reduce.py) !

 - There are no PEP8 issues in the file [`pandas/tests/extension/decimal/array.py`](https://github.com/jreback/pandas/blob/08a2add69fa856542be4b58d660dfbf64d170bff/pandas/tests/extension/decimal/array.py) !

 - There are no PEP8 issues in the file [`pandas/tests/extension/decimal/test_decimal.py`](https://github.com/jreback/pandas/blob/08a2add69fa856542be4b58d660dfbf64d170bff/pandas/tests/extension/decimal/test_decimal.py) !

 - There are no PEP8 issues in the file [`pandas/tests/extension/json/test_json.py`](https://github.com/jreback/pandas/blob/08a2add69fa856542be4b58d660dfbf64d170bff/pandas/tests/extension/json/test_json.py) !

 - There are no PEP8 issues in the file [`pandas/tests/extension/test_categorical.py`](https://github.com/jreback/pandas/blob/08a2add69fa856542be4b58d660dfbf64d170bff/pandas/tests/extension/test_categorical.py) !

 - There are no PEP8 issues in the file [`pandas/tests/extension/test_integer.py`](https://github.com/jreback/pandas/blob/08a2add69fa856542be4b58d660dfbf64d170bff/pandas/tests/extension/test_integer.py) !

 - There are no PEP8 issues in the file [`pandas/tests/extension/test_interval.py`](https://github.com/jreback/pandas/blob/08a2add69fa856542be4b58d660dfbf64d170bff/pandas/tests/extension/test_interval.py) !



##### Comment last updated on October 06, 2018 at 14:08 Hours UTC
cc @TomAugspurger @jorisvandenbossche 
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/22762?src=pr&el=h1) Report
> Merging [#22762](https://codecov.io/gh/pandas-dev/pandas/pull/22762?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/c8ce3d01e9ffafc24c6f9dd568cd9eb7e42c610c?src=pr&el=desc) will **increase** coverage by `<.01%`.
> The diff coverage is `100%`.

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

```diff
@@            Coverage Diff             @@
##           master   #22762      +/-   ##
==========================================
+ Coverage    92.2%    92.2%   +<.01%     
==========================================
  Files         169      169              
  Lines       50924    50944      +20     
==========================================
+ Hits        46952    46972      +20     
  Misses       3972     3972
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.62% <100%> (ø)` | :arrow_up: |
| #single | `42.29% <32%> (-0.01%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/22762?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/arrays/base.py](https://codecov.io/gh/pandas-dev/pandas/pull/22762/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2Jhc2UucHk=) | `95.94% <100%> (+0.05%)` | :arrow_up: |
| [pandas/core/series.py](https://codecov.io/gh/pandas-dev/pandas/pull/22762/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvc2VyaWVzLnB5) | `93.76% <100%> (+0.01%)` | :arrow_up: |
| [pandas/core/arrays/integer.py](https://codecov.io/gh/pandas-dev/pandas/pull/22762/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2ludGVnZXIucHk=) | `94.9% <100%> (+0.31%)` | :arrow_up: |
| [pandas/core/arrays/categorical.py](https://codecov.io/gh/pandas-dev/pandas/pull/22762/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2NhdGVnb3JpY2FsLnB5) | `95.62% <100%> (ø)` | :arrow_up: |

------

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

@jorisvandenbossche simplified
In the meeting, we discussed what error should be raised when an ExtensionArray doesn't implement a specific reduction (e.g. StringArray won't implement `mean`). On this branch I think a `NotImplementedError` will be raised. Perhaps a `TypeError` would be more appropriate?
@TomAugspurger 
> In the meeting, we discussed what error should be raised when an ExtensionArray doesn't implement a specific reduction (e.g. StringArray won't implement mean). On this branch I think a NotImplementedError will be raised. Perhaps a TypeError would be more appropriate?

so we have a base class ``_reduce`` which raises ``AbstractMethodError()``. so you are suggesting that we have an overriden ``_reduce``? or just generically change that to ``TypeError``?
I'm not really sure what's cleanest.

If a class implements just some reductions, changing what we do in the base class doesn't won't make a difference. At that point I think it's up to the EA to raise the correct exception for methods they don't support.

For classes that don't implement any reductions, we could either raise a TypeError, or put each call in a

```
try:
    values._reduce(...)
except NotImplementedError:
    raise TypeError(...)
```

but doing that everywhere seems error prone.
So my earlier comment "On this branch I think a NotImplementedError" may have been incorrect.
> or just generically change that to TypeError?

I would do that, because in the base scenario (the EA does not support reductions), users should see a TypeError and not an AbstractMethodError. And if we already raise that in the base implementation, an EA author does not need to do that.
ok updated to generically raise TypeError on EA's if a reduction operation is attempted and not overriden.
@jorisvandenbossche updated
@jreback can you answer to some of my remaining comments?


@jorisvandenbossche 

> You could also mask the data for now before passing to nanops ?

can't see to reply above, but that is not possible. you change behavior by pre-filtering, the nan's need to be handled by the nanops routines which already do the special cases (e.g. sum and such), and its just much simpler.
>  you change behavior by pre-filtering, the nan's need to be handled by the nanops routines which already do the special cases (e.g. sum and such), and its just much simpler.

OK. But related to this: I didn't fully follow the other PR that allows now to pass a mask to the nanops. Should't that allow to simply pass the integer array without casting to float? As the nanops will skip the nans according to the mask, and the nanops don't need to be able to infer the missing values from the actual data? 

> OK. But related to this: I didn't fully follow the other PR that allows now to pass a mask to the nanops. Should't that allow to simply pass the integer array without casting to float? As the nanops will skip the nans according to the mask, and the nanops don't need to be able to infer the missing values from the actual data?

no, nanops except only ndarrays, these cannot be converted (well they can), but its better to be explicit about this. The mask simply avoids a recomputatin of it (as I already have it).

> no, nanops except only ndarrays, these cannot be converted

I didn't mean to pass the IntegerArray, but about passing the `_data` numpy integer array. It will have nonsensical values where there are NaNs, but those should be filled with a fill_value appropriate for the operation by the nanops code, I would think?
> I didn't mean to pass the IntegerArray, but about passing the _data numpy integer array. It will have nonsensical values where there are NaNs, but those should be filled with a fill_value appropriate for the operation by the nanops code, I would think?

nope, nanops is not that smart

it doesn't mask first, rather, for example in mean, it does the mean, then just uses the mask to see whether to make the result a nan or not

e.g. from ``nanmean``

`` the_sum = _ensure_numeric(values.sum(axis, dtype=dtype_sum))``

I don't these were ever designed to use the mask in this way (in theory they could), but they are not. They *assume* that the nans will nan the result out, but valid values actually makes the calculation wrong :>

> the_sum = _ensure_numeric(values.sum(axis, dtype=dtype_sum))

But by then, the nans in the `values` are already filled with an appropriate fill value

I didn't check all of them, but eg sum and mean seem to work nicely:

```
In [44]: values = np.array([1, 2, 100, 4])

In [45]: mask = np.array([False, False, True, False])

In [46]: pd.core.nanops.nansum(values)
Out[46]: 107

In [47]: pd.core.nanops.nansum(values, mask=mask)
Out[47]: 7

In [48]: pd.core.nanops.nanmean(values)
Out[48]: 26.75

In [49]: pd.core.nanops.nanmean(values, mask=mask)
Out[49]: 2.3333333333333335
```
@jorisvandenbossche there are a lot more cases (e.g. skipna is also an arg to nanops). so will leave this for you if you want to try to make this work. to be honest its prob not even worth the perf diff.
this has been ready for a while

merging tomorrow unless other comments 
I am very busy until tomorrow, will take a final look on thursday
@jorisvandenbossche addressed comments & removed some xfailed tests.
That test looks good. +1 to merge.
@jorisvandenbossche your commits all look fine.
I opened https://github.com/pandas-dev/pandas/issues/23106 for the return type issue
@jreback Thanks!
yeah the return type seems easy but ran into some issues.