Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
ENH/BUG: Use Kleene logic for groupby any/all #40819
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ENH/BUG: Use Kleene logic for groupby any/all #40819
Changes from 31 commits
088ca14
2554921
9a8f9c9
5ca9c4b
68fd995
6530491
26146c2
20f475d
924b38e
423f43f
b1408ac
47ef037
4415060
bb04c1c
9c90886
ef3fbe2
f4c8a8a
1c3cb7d
7cbf85b
809b8a4
58fd33a
80a65bb
c9b9d5f
7514568
740ad7b
a116bed
8a428d4
8e3c5be
b627618
23b3b64
a30496c
3051a99
4cd2833
98cd401
a92c637
7c5c8e6
c66d1fd
0950234
c81c1a5
d2b8ad0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this expected is really hard to parse can you do in multiple steps / make simpler
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressing your comment above makes this simpler, let me know if you think any piece is still confusing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this check is really hard to parse.
what exactly are you checking? can you make this much simpler
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What this is essentially doing is
series.any(skipna=skipna)
, but then withgetattr
because it's generic for any/all. Do you have a concrete suggestion to change? (or to clarify the comment?)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be very concrete, you mean breaking it up into 2 lines, like
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).