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.
BUG: DataFrameGroupBy.transform and ngroup do not work with cumcount #27858
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
BUG: DataFrameGroupBy.transform and ngroup do not work with cumcount #27858
Changes from 11 commits
7e461a1
1314059
8bcb313
1bcf325
5c96549
42fdb0b
9b1af14
d520bcc
c298eaf
e2c51aa
c66ec83
4dc07eb
9d60bbb
0378a74
2a8e1ed
9920344
9012e53
8d360a2
8a371c6
e54f024
5a33608
45c5339
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.
leave this one
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.
Instead of this can't you just add cumcount to the transformation list? This somewhat blurs the line between
cythonized_kernels
which I think describe performance ops andtransformation_kernels
which describe output shapeThere 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.
Is this not already covered by test below? Ideally we can rely on fixtures rather than one-off tests like this
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.
it's a bit different than below... the test below is just to ensure after
transform
, the index of transformed result is the same as the original dataset.But you are right, this should not be in one-off tests... will look for if there is fixture for this already @WillAyd
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.
Is there not already a fixture for these we can leverage?
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.
ok, will change! thanks for review!
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.
Ideally, these would be xfails in the param list
Can you do that here?
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.
Sure, nice tip! thanks! Also i found out not all transform functions are tested, e.g.
fillna
ortshift
etc, is it okay I open another PR to add tests for the untested function? (some might have bug) @TomAugspurgerThere 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.
Yep, that would be great.