ENH: add groupby & reduce support to EA #22762

jreback · 2018-09-19T13:31:08Z

closes #21789
closes #22346
xref #22865

pep8speaks · 2018-09-19T13:31:16Z

Hello @jreback! Thanks for updating the PR.

There are no PEP8 issues in the file pandas/conftest.py !
There are no PEP8 issues in the file pandas/core/arrays/base.py !
There are no PEP8 issues in the file pandas/core/arrays/categorical.py !
There are no PEP8 issues in the file pandas/core/arrays/integer.py !
There are no PEP8 issues in the file pandas/core/series.py !
There are no PEP8 issues in the file pandas/tests/arrays/test_integer.py !
There are no PEP8 issues in the file pandas/tests/dtypes/test_common.py !
There are no PEP8 issues in the file pandas/tests/extension/arrow/test_bool.py !
There are no PEP8 issues in the file pandas/tests/extension/base/__init__.py !
There are no PEP8 issues in the file pandas/tests/extension/base/groupby.py !
There are no PEP8 issues in the file pandas/tests/extension/base/reduce.py !
There are no PEP8 issues in the file pandas/tests/extension/decimal/array.py !
There are no PEP8 issues in the file pandas/tests/extension/decimal/test_decimal.py !
There are no PEP8 issues in the file pandas/tests/extension/json/test_json.py !
There are no PEP8 issues in the file pandas/tests/extension/test_categorical.py !
There are no PEP8 issues in the file pandas/tests/extension/test_integer.py !
There are no PEP8 issues in the file pandas/tests/extension/test_interval.py !

Comment last updated on October 06, 2018 at 14:08 Hours UTC

jreback · 2018-09-19T13:32:17Z

cc @TomAugspurger @jorisvandenbossche

pandas/core/arrays/integer.py

pandas/tests/extension/base/reduce.py

pandas/core/arrays/datetimelike.py

pandas/core/dtypes/common.py

codecov · 2018-09-19T23:16:09Z

Codecov Report

Merging #22762 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            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%> (ø)`	⬆️
#single	`42.29% <32%> (-0.01%)`	⬇️

Impacted Files	Coverage Δ
pandas/core/arrays/base.py	`95.94% <100%> (+0.05%)`	⬆️
pandas/core/series.py	`93.76% <100%> (+0.01%)`	⬆️
pandas/core/arrays/integer.py	`94.9% <100%> (+0.31%)`	⬆️
pandas/core/arrays/categorical.py	`95.62% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c8ce3d0...aeaf5f3. Read the comment docs.

jorisvandenbossche

From a quick look: I have the feeling this is exposing too much internals of pandas to EA (eg this basically makes all our nanops implementations public).
We should maybe rethink the _reduce interface a bit? (I assume now this is basically identical to what we already have defined on series/dataframe?)

pandas/core/arrays/base.py

jreback · 2018-09-23T16:41:52Z

@jorisvandenbossche simplified

TomAugspurger · 2018-09-26T11:49:52Z

pandas/core/arrays/base.py

@@ -708,6 +712,25 @@ def _add_comparison_ops(cls):
        cls.__le__ = cls._create_comparison_method(operator.le)
        cls.__ge__ = cls._create_comparison_method(operator.ge)

+    def _reduce(self, name, skipna=True, **kwargs):


What sort of things go into kwargs? Is it things like ddof?

yes, they are specific to the reducer

TomAugspurger · 2018-09-27T19:41:10Z

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?

jreback · 2018-09-28T15:52:36Z

@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?

TomAugspurger · 2018-09-28T15:56:40Z

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.

TomAugspurger · 2018-09-28T15:57:22Z

So my earlier comment "On this branch I think a NotImplementedError" may have been incorrect.

jorisvandenbossche · 2018-09-28T15:57:50Z

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.

jreback · 2018-10-01T12:45:36Z

ok updated to generically raise TypeError on EA's if a reduction operation is attempted and not overriden.

doc/source/whatsnew/v0.24.0.txt

pandas/core/arrays/base.py

pandas/core/arrays/integer.py

pandas/core/dtypes/common.py

pandas/core/series.py

pandas/core/arrays/integer.py

pandas/tests/extension/decimal/array.py

jreback · 2018-10-03T12:10:29Z

@jorisvandenbossche updated

jorisvandenbossche · 2018-10-05T11:29:51Z

@jreback can you answer to some of my remaining comments?

jreback · 2018-10-05T11:49:58Z

@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.

…oked and not overriden, test the same

jreback · 2018-10-11T21:38:12Z

@jorisvandenbossche addressed comments & removed some xfailed tests.

pandas/tests/arrays/test_integer.py

TomAugspurger · 2018-10-12T11:14:34Z

That test looks good. +1 to merge.

jreback · 2018-10-12T12:13:49Z

@jorisvandenbossche your commits all look fine.

jorisvandenbossche · 2018-10-12T12:19:07Z

I opened #23106 for the return type issue

jorisvandenbossche · 2018-10-12T12:20:51Z

@jreback Thanks!

jreback · 2018-10-12T12:22:13Z

yeah the return type seems easy but ran into some issues.

jreback added Numeric Operations Arithmetic, Comparison, and Logical operations ExtensionArray Extending pandas with custom dtypes or arrays. labels Sep 19, 2018

jreback added this to the 0.24.0 milestone Sep 19, 2018

TomAugspurger reviewed Sep 19, 2018

View reviewed changes

pandas/core/arrays/integer.py Show resolved Hide resolved

pandas/core/arrays/integer.py Show resolved Hide resolved

pandas/tests/extension/base/reduce.py Show resolved Hide resolved

pandas/tests/extension/base/reduce.py Outdated Show resolved Hide resolved

This was referenced Sep 19, 2018

ENH: allow passing a mask to nanops to facilitate EA reduction ops #22764

Closed

Decimal fields dropped in group by with more than one column #22275

Closed

jreback force-pushed the groupby branch from 6becc87 to 4e2b5d6 Compare September 19, 2018 16:07

jbrockmendel reviewed Sep 19, 2018

View reviewed changes

pandas/core/arrays/datetimelike.py Outdated Show resolved Hide resolved

jbrockmendel reviewed Sep 19, 2018

View reviewed changes

pandas/core/dtypes/common.py Outdated Show resolved Hide resolved

jreback force-pushed the groupby branch from 4e2b5d6 to 43e2176 Compare September 19, 2018 23:15

jorisvandenbossche reviewed Sep 21, 2018

View reviewed changes

pandas/core/arrays/base.py Outdated Show resolved Hide resolved

jreback mentioned this pull request Sep 23, 2018

TST/CLN: Fixturize frame/test_analytics #22733

Merged

3 tasks

jreback force-pushed the groupby branch from 2955ae8 to 8103b8c Compare September 23, 2018 16:41

TomAugspurger reviewed Sep 26, 2018

View reviewed changes

jreback force-pushed the groupby branch from 5a83940 to cb0499c Compare October 1, 2018 12:45

jorisvandenbossche reviewed Oct 1, 2018

View reviewed changes

jreback force-pushed the groupby branch from cb0499c to c68b9b5 Compare October 3, 2018 12:10

jreback added 11 commits October 11, 2018 07:44

simplify EA._reduce

e7fbe0f

fix categorical

fddf938

make _reduce an always defined method on Base; raise TypeError if inv…

49efedf

…oked and not overriden, test the same

pass mask to nanops

371ab54

review comments

d94b85e

cleanup

6774791

doc

6699080

lint issue

29c3cf7

comments

0fdaf68

fix test

08f5830

comments

3e763c4

TomAugspurger mentioned this pull request Oct 11, 2018

SparseArray is an ExtensionArray #22325

Merged

4 tasks

jreback added 3 commits October 11, 2018 17:04

add op tests for reduction result dtype

b543386

remove xfails on frame arithmetic ops comparisons

b808778

remove groupby overrides

2b3d96f

jreback force-pushed the groupby branch from 0c80cbf to 2b3d96f Compare October 11, 2018 21:38

jorisvandenbossche reviewed Oct 11, 2018

View reviewed changes

pandas/tests/arrays/test_integer.py Show resolved Hide resolved

jorisvandenbossche added 2 commits October 12, 2018 11:48

format docstring

3037ccd

add test ensuring mean always gives float

b872607

add prod to list of type preserving reductions

aeaf5f3

jorisvandenbossche mentioned this pull request Oct 12, 2018

API: IntegerArray reductions: data type of the scalar result? #23106

Open

jorisvandenbossche approved these changes Oct 12, 2018

View reviewed changes

jorisvandenbossche merged commit 08e2752 into pandas-dev:master Oct 12, 2018

tm9k1 pushed a commit to tm9k1/pandas that referenced this pull request Nov 19, 2018

ENH: add groupby & reduce support to EA (pandas-dev#22762)

b843388

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: add groupby & reduce support to EA #22762

ENH: add groupby & reduce support to EA #22762

jreback commented Sep 19, 2018 •

edited

Loading

pep8speaks commented Sep 19, 2018 •

edited

Loading

jreback commented Sep 19, 2018

codecov bot commented Sep 19, 2018 •

edited

Loading

jorisvandenbossche left a comment

jreback commented Sep 23, 2018

TomAugspurger Sep 26, 2018

jreback Sep 28, 2018

TomAugspurger commented Sep 27, 2018

jreback commented Sep 28, 2018

TomAugspurger commented Sep 28, 2018

TomAugspurger commented Sep 28, 2018

jorisvandenbossche commented Sep 28, 2018 •

edited

Loading

jreback commented Oct 1, 2018

jreback commented Oct 3, 2018

jorisvandenbossche commented Oct 5, 2018

jreback commented Oct 5, 2018

jreback commented Oct 11, 2018

TomAugspurger commented Oct 12, 2018

jreback commented Oct 12, 2018

jorisvandenbossche commented Oct 12, 2018

jorisvandenbossche commented Oct 12, 2018

jreback commented Oct 12, 2018

ENH: add groupby & reduce support to EA #22762

ENH: add groupby & reduce support to EA #22762

Conversation

jreback commented Sep 19, 2018 • edited Loading

pep8speaks commented Sep 19, 2018 • edited Loading

Comment last updated on October 06, 2018 at 14:08 Hours UTC

jreback commented Sep 19, 2018

codecov bot commented Sep 19, 2018 • edited Loading

Codecov Report

jorisvandenbossche left a comment

Choose a reason for hiding this comment

jreback commented Sep 23, 2018

TomAugspurger Sep 26, 2018

Choose a reason for hiding this comment

jreback Sep 28, 2018

Choose a reason for hiding this comment

TomAugspurger commented Sep 27, 2018

jreback commented Sep 28, 2018

TomAugspurger commented Sep 28, 2018

TomAugspurger commented Sep 28, 2018

jorisvandenbossche commented Sep 28, 2018 • edited Loading

jreback commented Oct 1, 2018

jreback commented Oct 3, 2018

jorisvandenbossche commented Oct 5, 2018

jreback commented Oct 5, 2018

jreback commented Oct 11, 2018

TomAugspurger commented Oct 12, 2018

jreback commented Oct 12, 2018

jorisvandenbossche commented Oct 12, 2018

jorisvandenbossche commented Oct 12, 2018

jreback commented Oct 12, 2018

jreback commented Sep 19, 2018 •

edited

Loading

pep8speaks commented Sep 19, 2018 •

edited

Loading

codecov bot commented Sep 19, 2018 •

edited

Loading

jorisvandenbossche commented Sep 28, 2018 •

edited

Loading