ENH: Groupby agg support multiple funcs numba #53486
Conversation
| if is_groupby: | ||
| engine = self.kwargs.get("engine", None) | ||
| engine_kwargs = self.kwargs.get("engine_kwargs", None) | ||
| kwargs = {"engine": engine, "engine_kwargs": engine_kwargs} |
There was a problem hiding this comment.
Can we update the method signatures with by adding engine and engine_kwargs params instead of mangling kwargs?
There was a problem hiding this comment.
obj here can be a Series/DF/Resampler/ too, since the code is shared with there, so it wouldn't be possible to add engine/engine_kwargs there (unless we supported numba there too), since agg there is part of the public API.
There was a problem hiding this comment.
obj here can be a Series/DF/Resampler/ too, since the code is shared with there, so it wouldn't be possible to add engine/engine_kwargs there (unless we supported numba there too), since agg there is part of the public API.
There was a problem hiding this comment.
The more we evolve groupby, the more I realize trying to share this code between Series/DataFrame and groupby was a mistake of mine. I've been thinking we should refactor and separate off groupby entirely (but it can still live in core.apply). I'm okay with having a bit of a kludge here and will separate/cleanup after.
| if is_groupby: | ||
| engine = self.kwargs.get("engine", None) | ||
| engine_kwargs = self.kwargs.get("engine_kwargs", None) | ||
| kwargs = {"engine": engine, "engine_kwargs": engine_kwargs} |
There was a problem hiding this comment.
The more we evolve groupby, the more I realize trying to share this code between Series/DataFrame and groupby was a mistake of mine. I've been thinking we should refactor and separate off groupby entirely (but it can still live in core.apply). I'm okay with having a bit of a kludge here and will separate/cleanup after.
| @@ -1416,6 +1424,17 @@ def aggregate(self, func=None, *args, engine=None, engine_kwargs=None, **kwargs) | |||
| result.columns = columns # type: ignore[assignment] | |||
|
|
|||
| if result is None: | |||
| # Remove the kwargs we inserted | |||
There was a problem hiding this comment.
So we need to go through result = op.agg() first instead of just using self._aggregate_with_numba in the if if maybe_use_numba(engine) branch above?
There was a problem hiding this comment.
Yep, that checks for str/list/dict aggregations, and call back into the agg function itself.
It'll return None, if a callable is passed, though (what this block handles).
|
Looks pretty good. Just a merge conflict |
|
Thanks @lithomas1 |
* ENH: Groupby agg support multiple funcs numba * address code review * remove old TODO
* ENH: Groupby agg support multiple funcs numba * address code review * remove old TODO
doc/source/whatsnew/vX.X.X.rstfile if fixing a bug or adding a new feature.Only thing missing for now is Groupby.agg with multiple UDFs. This'll require changes to the shared apply code, so I've left that out of this PR.
(I've added a commented out example test case to address this).