FIX-#3498 Fix groupby to use kwargs for getting the groups #3523
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
@ahallermed Great PR! The changes look good to me.
commit lint is failing due to a missing colon (:) between the issue number and commit description. The first line needs to look like this:
FIX-#3498: Fix groupby to use kwargs for getting the groups
|
Codecov Report
@@ Coverage Diff @@
## master #3523 +/- ##
==========================================
+ Coverage 85.45% 89.19% +3.73%
==========================================
Files 197 197
Lines 16437 17434 +997
==========================================
+ Hits 14047 15551 +1504
+ Misses 2390 1883 -507
Continue to review full report at Codecov.
|
Co-authored-by: Devin Petersohn <[email protected]> Signed-off-by: Andreas Haller <[email protected]>
devin-petersohn
force-pushed
the
master
branch
from
cb36c50 to
5dcf83c
Compare
|
|
vnlitvinov
reviewed
Nov 2, 2021
There was a problem hiding this comment.
Overall looks good!
@ahallermed please note some tests are failing, and those do not seem like some false positives. Could you please look into those?
vnlitvinov
reviewed
Nov 19, 2021
Co-authored-by: Vasily Litvinov <[email protected]>
|
I think @dchigarev is looking into this PR, as it apparently is a little more complicated than that. |
|
Currently, tests are failing because of the lines that pass kwargs to the There is a bug in pandas (pandas-dev/pandas#35202) that doesn't allow us to take >>> df = pandas.DataFrame({"a": [1, 2, 2, np.nan]})
>>> grp = df.groupby("a", dropna=False)
>>> grp.groups
ValueError: Categorical categories cannot be null
>>> grp.indices # `.indices` works fine though
{1.0: array([0], dtype=int64), 2.0: array([1, 2], dtype=int64), nan: array([3], dtype=int64)}OFC we could use an I also found that the groups comparator works a bit incorrect and doesn't properly verify that the bug (#3498) is actually fixed. Filled an issue as well (#3756). It also must be fixed before this PR. |
|
Thank you @vnlitvinov and @dchigarev for the deep dive. I wasn't able to have a look at your discussion for a while. Sorry about that. |
Yes, and I'm going to push the solution to the PR's branch in a short time |
Signed-off-by: Dmitry Chigarev <[email protected]>
Signed-off-by: Dmitry Chigarev <[email protected]>
dchigarev
reviewed
Dec 10, 2021
Comment on lines
-48
to
+49
| assert g1[0] == g2[0] | ||
| value_equals(g1[0], g2[0]) |
There was a problem hiding this comment.
changed so nan values be compared correctly
|
PR is ready for review |
There was a problem hiding this comment.
Implementation looks good to me, thanks @dchigarev for the updates. Should we have a default to pandas warning here?
Signed-off-by: Dmitry Chigarev <[email protected]>
Yes, we should. Added a warning. |
YarShev
approved these changes
Dec 17, 2021
dorisjlee
pushed a commit
to dorisjlee/modin
that referenced
this pull request
Jan 13, 2022
…ps (modin-project#3523) Co-authored-by: Devin Petersohn <[email protected]> Co-authored-by: Vasily Litvinov <[email protected]> Co-authored-by: Dmitry Chigarev <[email protected]> Signed-off-by: Andreas Haller <[email protected]> Signed-off-by: Doris Lee <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Signed-off-by: Andreas Haller [email protected]
What do these changes do?
Adds
dropna=Falsesupport for getting the group indices (GroupBy.indices/.groups) by forwarding this parameter to the pandas groupby object that actually computes the thing.Note that this PR doesn't ensure full support of
dropna=False. See the epic (#3828) for the rest of the tasks.flake8 modinblack --check modingit commit -sGroupBy.groups/.indicesignores dropna=False as kwarg #3498docs/developer/architecture.rstis up-to-date -> Not needed