-
Notifications
You must be signed in to change notification settings - Fork 651
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
FIX-#1854: groupby() with arbitrary series #1886
FIX-#1854: groupby() with arbitrary series #1886
Conversation
Signed-off-by: Itamar Turner-Trauring <itamar@itamarst.org>
…ies. Signed-off-by: Itamar Turner-Trauring <itamar@itamarst.org>
Codecov Report
@@ Coverage Diff @@
## master #1886 +/- ##
==========================================
+ Coverage 81.39% 81.47% +0.07%
==========================================
Files 79 79
Lines 9165 9192 +27
==========================================
+ Hits 7460 7489 +29
+ Misses 1705 1703 -2
Continue to review full report at Codecov.
|
Thanks @itamarst. First of all, please update your PR with last changes from master branch. |
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.
@itamarst, thanks for the changes! I've left some question for you
@dchigarev many thanks for the review! |
Signed-off-by: Itamar Turner-Trauring <itamar@itamarst.org>
Signed-off-by: Itamar Turner-Trauring <itamar@itamarst.org>
Signed-off-by: Itamar Turner-Trauring <itamar@itamarst.org>
Signed-off-by: Itamar Turner-Trauring <itamar@itamarst.org>
Signed-off-by: Itamar Turner-Trauring <itamar@itamarst.org>
Signed-off-by: Itamar Turner-Trauring <itamar@itamarst.org>
Signed-off-by: Itamar Turner-Trauring <itamar@itamarst.org>
Should be ready for another 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.
@itamarst thanks for the changes! I've left some minor comments, everything else is looks good for me.
@devin-petersohn want to take a look?
Signed-off-by: Itamar Turner-Trauring <itamar@itamarst.org>
Signed-off-by: Itamar Turner-Trauring <itamar@itamarst.org>
Signed-off-by: Itamar Turner-Trauring <itamar@itamarst.org>
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.
LGTM, thanks @itamarst!
I agree that this implementation is somewhat uncomfortable, but luckily it is a very rare edge case. At least in this new implementation it is working. Thanks again!
…t#1886) Signed-off-by: Itamar Turner-Trauring <itamar@itamarst.org>
What do these changes do?
This attempts to fix #1854.
I am uncomfortable with this approach; it works, insofar as tests pass, but I feel like I'm scattering brittle workarounds everywhere. Even just adding some abstractions on the hack would probably help, but I don't understand the code base quite yet to know how to do; suggestions would be welcome.
flake8 modin
black --check modin
git commit -s