ENH/BUG: Use Kleene logic for groupby any/all #40819

mzeitlin11 · 2021-04-06T23:34:18Z

closes ENH: implement Kleene versions of GroupBy any/all kernels for nullable dtypes #37506
closes BUG: Groupby.any(skipna=True) #40585
closes BUG: Series.any/all with level keyword should preserve boolean dtype #33449
tests added / passed
Ensure all linting tests pass, see here for how to run them
whatsnew entry

Built on #40811

ASV's don't look affected:

before           after         ratio
     [3b4b193d]       [423f43fc]
     <master>         <enh/any_all_kleene>
          216±8ms          205±4ms     0.95  groupby.GroupByCythonAgg.time_frame_agg('float64', 'all')
          208±8ms          197±3ms     0.95  groupby.GroupByCythonAgg.time_frame_agg('float64', 'any')
         162±10μs          139±4μs    ~0.86  groupby.GroupByMethods.time_dtype_as_field('datetime', 'all', 'direct')
         161±20μs          138±9μs    ~0.86  groupby.GroupByMethods.time_dtype_as_field('datetime', 'all', 'transformation')
         159±10μs          142±5μs    ~0.90  groupby.GroupByMethods.time_dtype_as_field('datetime', 'any', 'direct')
         160±10μs          140±5μs    ~0.88  groupby.GroupByMethods.time_dtype_as_field('datetime', 'any', 'transformation')
          109±8μs         113±10μs     1.04  groupby.GroupByMethods.time_dtype_as_field('float', 'all', 'direct')
         103±10μs          109±9μs     1.06  groupby.GroupByMethods.time_dtype_as_field('float', 'all', 'transformation')
         113±20μs         132±10μs    ~1.17  groupby.GroupByMethods.time_dtype_as_field('float', 'any', 'direct')
         138±10μs         124±20μs    ~0.90  groupby.GroupByMethods.time_dtype_as_field('float', 'any', 'transformation')
          110±9μs          112±5μs     1.02  groupby.GroupByMethods.time_dtype_as_field('int', 'all', 'direct')
          115±9μs         109±10μs     0.94  groupby.GroupByMethods.time_dtype_as_field('int', 'all', 'transformation')
         123±10μs          115±7μs     0.94  groupby.GroupByMethods.time_dtype_as_field('int', 'any', 'direct')
         125±10μs          114±8μs     0.92  groupby.GroupByMethods.time_dtype_as_field('int', 'any', 'transformation')
         449±30μs         452±20μs     1.01  groupby.GroupByMethods.time_dtype_as_field('object', 'all', 'direct')
         442±30μs         445±20μs     1.01  groupby.GroupByMethods.time_dtype_as_field('object', 'all', 'transformation')
         470±10μs         455±30μs     0.97  groupby.GroupByMethods.time_dtype_as_field('object', 'any', 'direct')
         466±30μs          406±4μs    ~0.87  groupby.GroupByMethods.time_dtype_as_field('object', 'any', 'transformation')
          118±8μs          118±7μs     1.00  groupby.GroupByMethods.time_dtype_as_group('datetime', 'all', 'direct')
          134±9μs         132±20μs     0.99  groupby.GroupByMethods.time_dtype_as_group('datetime', 'all', 'transformation')
         131±20μs          115±9μs    ~0.88  groupby.GroupByMethods.time_dtype_as_group('datetime', 'any', 'direct')
         121±10μs          118±5μs     0.97  groupby.GroupByMethods.time_dtype_as_group('datetime', 'any', 'transformation')
         111±10μs          111±4μs     1.00  groupby.GroupByMethods.time_dtype_as_group('float', 'all', 'direct')
         109±10μs         120±10μs    ~1.11  groupby.GroupByMethods.time_dtype_as_group('float', 'all', 'transformation')
          110±9μs         127±10μs    ~1.16  groupby.GroupByMethods.time_dtype_as_group('float', 'any', 'direct')
         112±10μs         122±10μs     1.09  groupby.GroupByMethods.time_dtype_as_group('float', 'any', 'transformation')
         130±10μs          115±6μs    ~0.89  groupby.GroupByMethods.time_dtype_as_group('int', 'all', 'direct')
          133±7μs          115±7μs    ~0.86  groupby.GroupByMethods.time_dtype_as_group('int', 'all', 'transformation')
          117±5μs         123±10μs     1.05  groupby.GroupByMethods.time_dtype_as_group('int', 'any', 'direct')
         133±10μs         118±20μs    ~0.89  groupby.GroupByMethods.time_dtype_as_group('int', 'any', 'transformation')
          107±9μs         122±10μs    ~1.14  groupby.GroupByMethods.time_dtype_as_group('object', 'all', 'direct')
         113±10μs          107±5μs     0.94  groupby.GroupByMethods.time_dtype_as_group('object', 'all', 'transformation')
         135±10μs          108±4μs    ~0.80  groupby.GroupByMethods.time_dtype_as_group('object', 'any', 'direct')
          114±6μs          105±2μs     0.92  groupby.GroupByMethods.time_dtype_as_group('object', 'any', 'transformation')

rhshadrach

Thanks for the PR!

doc/source/whatsnew/v1.3.0.rst

pandas/tests/reductions/test_reductions.py

pandas/core/groupby/groupby.py

jorisvandenbossche

@mzeitlin11 Thanks a lot for working on this!

jorisvandenbossche · 2021-04-08T18:50:28Z

pandas/core/groupby/groupby.py

+from pandas.core.arrays.boolean import BooleanArray
+from pandas.core.arrays.masked import BaseMaskedArray


You can import both from from pandas.core.arrays import ..

jorisvandenbossche · 2021-04-08T18:53:43Z

pandas/core/groupby/groupby.py

@@ -1435,6 +1446,7 @@ def result_to_bool(result: np.ndarray, inference: type) -> np.ndarray:
            post_processing=result_to_bool,
            val_test=val_test,
            skipna=skipna,
+            masked=False,


Why is this set to always be False?

Had originally used masked being included in kwargs as a signal for whether to pass masked into the groupby func. But the original value here wasn't relevant since it gets set before being used.

This is definitely confusing and a poor solution, changed to instead use a positional arg

Thanks, the update looks clearer to follow now

jorisvandenbossche · 2021-04-08T18:58:35Z

pandas/tests/groupby/test_any_all.py

+@pytest.mark.parametrize("bool_agg_func", ["any", "all"])
+@pytest.mark.parametrize("skipna", [True, False])
+@pytest.mark.parametrize(
+    # expected indexed as [skipna][bool_agg_func == "all"]


Can you maybe also write out what that results in practice? So [skipna=False/any, skipna=False / all], [skipna=True / any, skipna=True / all], if I understand correctly

Yep have changed, that is a much clearer way to describe it

pandas/tests/groupby/test_any_all.py

pep8speaks · 2021-04-10T17:22:15Z

Hello @mzeitlin11! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-04-13 15:02:14 UTC

rhshadrach

lgtm

jorisvandenbossche

Looks good to me! (and much cleaner now ;))

I only added a few tiny stylistic/doc suggestions you can apply.

doc/source/whatsnew/v1.3.0.rst

pandas/tests/groupby/test_any_all.py

jreback

pls merge master as well

doc/source/whatsnew/v1.3.0.rst

jreback · 2021-04-13T12:21:03Z

pandas/tests/groupby/test_any_all.py

+
+    # The expected result we compared to should match aggregating on the whole
+    # series
+    result = getattr(df[0], bool_agg_func)(skipna=skipna)


this check is really hard to parse.
what exactly are you checking? can you make this much simpler

What this is essentially doing is series.any(skipna=skipna), but then with getattr because it's generic for any/all. Do you have a concrete suggestion to change? (or to clarify the comment?)

there is a complicated expexcted then a really complicated assert. This needs to be greatly simplified. It is impossible to grok with all of this logic. my suggesetion is to break this up.

To be very concrete, you mean breaking it up into 2 lines, like

series = df[0] getattr(series, method)(skipna=skipna)

?

Have tried to simplify by moving this validation of "expected" into a separate test in test_reductions.py (replacing the existing test there as this tests a superset of the logic).

jreback · 2021-04-13T12:21:19Z

pandas/tests/groupby/test_any_all.py

+def test_masked_kleene_logic(bool_agg_func, data, expected_data, skipna):
+    # GH#37506
+    df = DataFrame(data, dtype="boolean")
+    expected = DataFrame(


this expected is really hard to parse can you do in multiple steps / make simpler

Addressing your comment above makes this simpler, let me know if you think any piece is still confusing

Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>

…to enh/any_all_kleene

jreback

ok looks good. question on the rendering in the notes, ping on green.

doc/source/whatsnew/v1.3.0.rst

pandas/tests/reductions/test_reductions.py

mzeitlin11 · 2021-04-13T17:11:39Z

Greenish, failures unrelated and fixed by #40930 and flaky zip fix that just got merged

jreback · 2021-04-13T17:12:21Z

thanks @mzeitlin11 very nice!

mzeitlin11 added 11 commits April 6, 2021 14:03

WIP

088ca14

TSTS: Consolidate groupby any, all

2554921

Fixture fixup

9a8f9c9

Unmove test

5ca9c4b

Merge remote-tracking branch 'origin/master' into enh/any_all_kleene

68fd995

Merge branch 'tst/any_all' into enh/any_all_kleene

6530491

Add initial tests

26146c2

Add whatsnew, bench

20f475d

Clean up edge case

924b38e

Fix typo

423f43f

Avoid copy if possible

b1408ac

mzeitlin11 added Bug Groupby Missing-data np.nan, pd.NaT, pd.NA, dropna, isnull, interpolate labels Apr 6, 2021

mzeitlin11 added 3 commits April 6, 2021 20:17

Fix old level test

47ef037

Precommit fixup

4415060

Clean up print

bb04c1c

mzeitlin11 added NA - MaskedArrays Related to pd.NA and nullable extension arrays and removed Missing-data np.nan, pd.NaT, pd.NA, dropna, isnull, interpolate labels Apr 7, 2021

mzeitlin11 added 3 commits April 7, 2021 15:56

Merge remote-tracking branch 'origin/master' into enh/any_all_kleene

9c90886

precommit fixup

ef3fbe2

Merge remote-tracking branch 'origin/master' into enh/any_all_kleene

f4c8a8a

rhshadrach requested changes Apr 8, 2021

View reviewed changes

doc/source/whatsnew/v1.3.0.rst Outdated Show resolved Hide resolved

pandas/tests/reductions/test_reductions.py Outdated Show resolved Hide resolved

pandas/core/groupby/groupby.py Outdated Show resolved Hide resolved

mzeitlin11 added 5 commits April 8, 2021 11:32

Split out test

1c3cb7d

Split whatsnew

7cbf85b

whatsnew typo

809b8a4

Modify dispatch, add mixed test

58fd33a

Fix post proc check

80a65bb

jorisvandenbossche reviewed Apr 8, 2021

View reviewed changes

Address review comments

c9b9d5f

mzeitlin11 added 4 commits April 10, 2021 10:46

Name arguments better

a116bed

Use -1 as mask signal

8a428d4

Consistent typing

8e3c5be

Don't use inspect

b627618

mzeitlin11 added 2 commits April 10, 2021 13:34

precommit fixup

23b3b64

Clean up docstring

a30496c

rhshadrach approved these changes Apr 10, 2021

View reviewed changes

jorisvandenbossche mentioned this pull request Apr 12, 2021

Preserving boolean dtype in Series.any/all function with level keyword #33449 #33543

Closed

Merge remote-tracking branch 'origin/master' into enh/any_all_kleene

3051a99

jorisvandenbossche approved these changes Apr 13, 2021

View reviewed changes

jorisvandenbossche mentioned this pull request Apr 13, 2021

ENH: support masked arrays in groupby cython algos #37493

Closed

10 tasks

jreback requested changes Apr 13, 2021

View reviewed changes

mzeitlin11 and others added 8 commits April 13, 2021 10:29

Update doc/source/whatsnew/v1.3.0.rst

4cd2833

Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>

Update doc/source/whatsnew/v1.3.0.rst

98cd401

Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>

Update doc/source/whatsnew/v1.3.0.rst

a92c637

Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>

Update pandas/tests/groupby/test_any_all.py

7c5c8e6

Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>

Update pandas/tests/groupby/test_any_all.py

c66d1fd

Co-authored-by: Joris Van den Bossche <jorisvandenbossche@gmail.com>

Merge branch 'master' into enh/any_all_kleene

0950234

Simplify teasts

c81c1a5

Merge branch 'enh/any_all_kleene' of github.com:/mzeitlin11/pandas in…

d2b8ad0

…to enh/any_all_kleene

jreback requested changes Apr 13, 2021

View reviewed changes

doc/source/whatsnew/v1.3.0.rst Show resolved Hide resolved

pandas/tests/reductions/test_reductions.py Show resolved Hide resolved

jreback approved these changes Apr 13, 2021

View reviewed changes

jreback added this to the 1.3 milestone Apr 13, 2021

jreback merged commit bbbd15c into pandas-dev:master Apr 13, 2021

mzeitlin11 deleted the enh/any_all_kleene branch April 13, 2021 17:13

mzeitlin11 mentioned this pull request Apr 13, 2021

PERF/BUG: use masked algo in groupby cummin and cummax #40651

Merged

JulianWgs pushed a commit to JulianWgs/pandas that referenced this pull request Jul 3, 2021

ENH/BUG: Use Kleene logic for groupby any/all (pandas-dev#40819)

079ef5c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH/BUG: Use Kleene logic for groupby any/all #40819

ENH/BUG: Use Kleene logic for groupby any/all #40819

mzeitlin11 commented Apr 6, 2021 •

edited

rhshadrach left a comment

jorisvandenbossche left a comment

jorisvandenbossche Apr 8, 2021

mzeitlin11 Apr 8, 2021

jorisvandenbossche Apr 8, 2021

mzeitlin11 Apr 8, 2021

jorisvandenbossche Apr 9, 2021

jorisvandenbossche Apr 8, 2021

mzeitlin11 Apr 8, 2021

pep8speaks commented Apr 10, 2021 •

edited

rhshadrach left a comment

jorisvandenbossche left a comment

jreback left a comment

jreback Apr 13, 2021

jorisvandenbossche Apr 13, 2021

jreback Apr 13, 2021

jorisvandenbossche Apr 13, 2021

mzeitlin11 Apr 13, 2021

jreback Apr 13, 2021

mzeitlin11 Apr 13, 2021

jreback left a comment

mzeitlin11 commented Apr 13, 2021

jreback commented Apr 13, 2021

		from pandas.core.arrays.boolean import BooleanArray
		from pandas.core.arrays.masked import BaseMaskedArray

ENH/BUG: Use Kleene logic for groupby any/all #40819

ENH/BUG: Use Kleene logic for groupby any/all #40819

Conversation

mzeitlin11 commented Apr 6, 2021 • edited

rhshadrach left a comment

Choose a reason for hiding this comment

jorisvandenbossche left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pep8speaks commented Apr 10, 2021 • edited

Comment last updated at 2021-04-13 15:02:14 UTC

rhshadrach left a comment

Choose a reason for hiding this comment

jorisvandenbossche left a comment

Choose a reason for hiding this comment

jreback left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback left a comment

Choose a reason for hiding this comment

mzeitlin11 commented Apr 13, 2021

jreback commented Apr 13, 2021

mzeitlin11 commented Apr 6, 2021 •

edited

pep8speaks commented Apr 10, 2021 •

edited