-
-
Notifications
You must be signed in to change notification settings - Fork 18.5k
BUG: retain ordered Categorical dtype in SeriesGroupBy aggregations #41147
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
result = type(values)._from_sequence(result.ravel(), dtype=values.dtype) | ||
# Note this will have result.dtype == dtype from above | ||
|
||
elif ( |
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.
in conjunction with #41086, cast_agg_result becomes a 1-liner and can be cleanly tacked onto the end of py_fallback
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, just a question regarding possible future work
) | ||
objvals = obj._values | ||
|
||
if isinstance(objvals, Categorical): |
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.
Does it make sense to try to separate this out further upstream in the future, so that _cython_agg_general isn't calling pure-python methods?
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 we'll get this case handled within WrappedCythonOp._ea_wrap_cython_operation before too long, but im still troubleshooting that
@jbrockmendel lgtm. was there a precursor to merge first? |
already got #41111 |
ATM we correctly wrap them for DataFrameGroupBy but not for SeriesGroupBy. Doing this correctly for SeriesGroupBy has the benefit of avoiding the need to special-case it in DataFrameGroupBy