-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
BUG: fix sorting with observed=True in groupby #25173
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
Conversation
@@ -269,7 +269,7 @@ def test_groupby_categorical_index_and_columns(self, observed): | |||
# if we are not-observed we undergo a reindex | |||
# so need to adjust the output as our expected sets us up | |||
# to be non-observed | |||
expected_columns = CategoricalIndex(['A', 'B'], | |||
expected_columns = CategoricalIndex(['B', 'A'], |
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.
expected_columns should have ['B', 'A']
, because ordered=True
.
Codecov Report
@@ Coverage Diff @@
## master #25173 +/- ##
==========================================
+ Coverage 92.37% 92.37% +<.01%
==========================================
Files 166 166
Lines 52408 52409 +1
==========================================
+ Hits 48412 48413 +1
Misses 3996 3996
Continue to review full report at Codecov.
|
1 similar comment
Codecov Report
@@ Coverage Diff @@
## master #25173 +/- ##
==========================================
+ Coverage 92.37% 92.37% +<.01%
==========================================
Files 166 166
Lines 52408 52409 +1
==========================================
+ Hits 48412 48413 +1
Misses 3996 3996
Continue to review full report at Codecov.
|
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.
I am not sure I buy your test example. there are actually 4 cases here observed / sort both True/False. Please enumerate what the expected results are.
@@ -360,6 +360,14 @@ def test_observed(observed): | |||
expected = groups2.agg('mean').reset_index() | |||
tm.assert_frame_equal(result, expected) | |||
|
|||
# GH 25167 |
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.
make this a separate test, parameterize by sort as well
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.
can you merge master and update to comments
@@ -73,7 +73,7 @@ Bug Fixes | |||
|
|||
**Reshaping** | |||
|
|||
- | |||
- Fixed bug in :meth:`DataFrame.groupby` where using ``observed=True`` didn't sort properly (:issue:`25167`) |
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.
move to 0.25.0
Closing as stale ping if you'd like to continue |
As documented in pandas-dev#25871, groupby() on an ordered Categorical messes up category order when 'observed=True' is specified. Specifically, group labels will be ordered by first occurrence (as for an unordered Categorical), but grouped aggregation results will retain the Categorical's order. The fix is a modified subset of pandas-dev#25173, which fixes a related case, but has not been merged yet.
* BUG: Fix groupby on ordered Categoricals (GH25871) As documented in pandas-dev#25871, groupby() on an ordered Categorical messes up category order when 'observed=True' is specified. Specifically, group labels will be ordered by first occurrence (as for an unordered Categorical), but grouped aggregation results will retain the Categorical's order. The fix is a modified subset of pandas-dev#25173, which fixes a related case, but has not been merged yet. * BUG: Fix groupby on ordered Categoricals (GH25871) * new test * Fix groupby on ordered Categoricals (GH25871) Testing all combinations of: - ordered vs. unordered grouping column - 'observed' True vs. False - 'sort' True vs. False In all cases, result group ordering must be correct. The test is built such that the result index labels are equal to aggregation results if all goes well (except for the one unobserved category)
* BUG: Fix groupby sorting on ordered Categoricals (GH25871) (#1) * BUG: Fix groupby on ordered Categoricals (GH25871) As documented in pandas-dev#25871, groupby() on an ordered Categorical messes up category order when 'observed=True' is specified. Specifically, group labels will be ordered by first occurrence (as for an unordered Categorical), but grouped aggregation results will retain the Categorical's order. The fix is a modified subset of pandas-dev#25173, which fixes a related case, but has not been merged yet. * BUG: Fix groupby on ordered Categoricals (GH25871) * new test * Fix groupby on ordered Categoricals (GH25871) Testing all combinations of: - ordered vs. unordered grouping column - 'observed' True vs. False - 'sort' True vs. False In all cases, result group ordering must be correct. The test is built such that the result index labels are equal to aggregation results if all goes well (except for the one unobserved category) * Revert "BUG: Fix groupby sorting on ordered Categoricals (GH25871) (#1)" This reverts commit b265349.
git diff upstream/master -u -- "*.py" | flake8 --diff
Fix bug in #25167.