CLN: trim unreachable groupby paths #41361
Conversation
jbrockmendel
commented
May 6, 2021
jbrockmendel
commented
- closes #xxxx
- tests added / passed
- Ensure all linting tests pass, see here for how to run them
- whatsnew entry
| # "Optional[List[str]]", variable has type "Index") | ||
| ret.columns = columns # type: ignore[assignment] | ||
| return ret | ||
|
|
There was a problem hiding this comment.
you could outdent (and remove the else) here to make it more obvious
There was a problem hiding this comment.
considered that, but this mirrors the layout of the DataFrameGroupBy method and im holding out hope of sharing more of that
| @@ -1000,6 +998,11 @@ def aggregate(self, func=None, *args, engine=None, engine_kwargs=None, **kwargs) | |||
| result = op.agg() | |||
| if not is_dict_like(func) and result is not None: | |||
| return result | |||
| elif relabeling and result is not None: | |||
| # this should be the only (non-raising) case with relabeling | |||
There was a problem hiding this comment.
@rhshadrach i wrote this comment a few years ago and now need help figuring it out. in particular, can re rule out relabeling and result is None? Or is that a shortcoming of the test suite?
There was a problem hiding this comment.
Yes - it is not possible that relabeling is True and result is None. If relabeling is True, the func returned from reconstruct_func must be the result from normalize_keyword_aggregation which is a dictionary. In the case of a dict-like, the result is always computed in Apply.agg via Apply.agg_dict_like.
There was a problem hiding this comment.
so this condition can be replaced with elif relabeling:?
There was a problem hiding this comment.
Yes. Also, as an aside, I think it was a misstep to try to reuse the code in core.apply for groupby.agg. I don't have any concrete ideas for cleaning this up yet, but it's on my radar.
