Skip to content

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

Closed

Conversation

topper-123
Copy link
Contributor

Fix bug in #25167.

@@ -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'],
Copy link
Contributor Author

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
Copy link

codecov bot commented Feb 6, 2019

Codecov Report

Merging #25173 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #25173      +/-   ##
==========================================
+ Coverage   92.37%   92.37%   +<.01%     
==========================================
  Files         166      166              
  Lines       52408    52409       +1     
==========================================
+ Hits        48412    48413       +1     
  Misses       3996     3996
Flag Coverage Δ
#multiple 90.79% <100%> (ø) ⬆️
#single 42.86% <0%> (-0.01%) ⬇️
Impacted Files Coverage Δ
pandas/core/groupby/grouper.py 98.18% <100%> (+0.01%) ⬆️
pandas/core/groupby/categorical.py 95.23% <100%> (-0.22%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e3b0950...8ec4674. Read the comment docs.

1 similar comment
@codecov
Copy link

codecov bot commented Feb 6, 2019

Codecov Report

Merging #25173 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #25173      +/-   ##
==========================================
+ Coverage   92.37%   92.37%   +<.01%     
==========================================
  Files         166      166              
  Lines       52408    52409       +1     
==========================================
+ Hits        48412    48413       +1     
  Misses       3996     3996
Flag Coverage Δ
#multiple 90.79% <100%> (ø) ⬆️
#single 42.86% <0%> (-0.01%) ⬇️
Impacted Files Coverage Δ
pandas/core/groupby/grouper.py 98.18% <100%> (+0.01%) ⬆️
pandas/core/groupby/categorical.py 95.23% <100%> (-0.22%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e3b0950...8ec4674. Read the comment docs.

Copy link
Contributor

@jreback jreback left a 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
Copy link
Contributor

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

@jreback jreback added Groupby Categorical Categorical Data Type labels Feb 6, 2019
@gfyoung gfyoung added the Bug label Feb 7, 2019
Copy link
Contributor

@jreback jreback left a 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`)
Copy link
Contributor

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

@WillAyd
Copy link
Member

WillAyd commented Mar 19, 2019

Closing as stale ping if you'd like to continue

@WillAyd WillAyd closed this Mar 19, 2019
kpflugshaupt added a commit to kpflugshaupt/pandas that referenced this pull request Mar 26, 2019
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.
kpflugshaupt added a commit to kpflugshaupt/pandas that referenced this pull request Mar 28, 2019
* 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)
kpflugshaupt added a commit to kpflugshaupt/pandas that referenced this pull request Mar 28, 2019
* 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Categorical Categorical Data Type Groupby
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Groupby with observed=True doesn't sort
4 participants