Support args in groupby apply
#10682
Conversation
brandon-b-miller
added
feature request
Codecov Report
@@ Coverage Diff @@
## branch-22.06 #10682 +/- ##
================================================
+ Coverage 86.35% 86.38% +0.02%
================================================
Files 142 142
Lines 22335 22337 +2
================================================
+ Hits 19287 19295 +8
+ Misses 3048 3042 -6
Continue to review full report at Codecov.
|
| @@ -516,7 +516,7 @@ def pipe(self, func, *args, **kwargs): | |||
| """ | |||
| return cudf.core.common.pipe(self, func, *args, **kwargs) | |||
|
|
|||
| def apply(self, function): | |||
| def apply(self, function, *args): | |||
There was a problem hiding this comment.
Is there a reason we can't support **kwargs in the same way?
There was a problem hiding this comment.
Good question. I considered this, and the reason I did not add it was really roadmap considerations. I know we want to rethink how this API works in general, and if we start supporting this now, we could back ourselves into a corner where users are reliant upon it and we're limited in the avenues we can pursue to replace this pipeline without complicating things at the outset. The same thing makes me a little uneasy about even adding *args, but at least there's a direct feature request for that and precedent for it elsewhere (DataFrame.apply and Series.apply both support args=).
As you have observed, it would probably not take much work to add so I'm happy to add it if there's strong motivation to do so.
There was a problem hiding this comment.
The motivation is to make this API align with pandas. https://pandas.pydata.org/docs/reference/api/pandas.core.groupby.GroupBy.apply.html#pandas.core.groupby.GroupBy.apply
What are we wanting to rethink? The public API, or the internal implementation? I would say that *args and **kwargs go together for this feature and we should implement both or neither -- not just *args.
There was a problem hiding this comment.
Similarly, we should enable **kwargs for Series.apply and DataFrame.apply if the numba machinery is no more complicated than it is here. Parameters-by-keyword and parameters-by-position serve the same purpose and I don't see a fundamental distinction in complexity between them.
There was a problem hiding this comment.
It's the internal implementation that I expect to change, since this implementation does a loop over chunks which is what makes it potentially slow. Right now this API isn't backed by Numba at all, which is why we can pass through args and theoretically kwargs rather easily.
kwargs in a numba implementation is not trivial. For context, numba does not support kwargs at all in compiled functions. This is why we don't have it in the other apply APIs. Since we could end up exploring some kind of JIT compiled implementation of this API as well, it might be closing the door on some solutions for us that could otherwise cover a broad swath of use cases.
I would say that
*argsand**kwargsgo together for this feature and we should implement both or neither -- not just*args.
I empathize with the feeling of asymmetry that comes along with having *args and not **kwargs, but not with the notion that we should not have *args unless we can also have **kwargs. For reasons similar to what you noted about there being little fundamental distinction, supporting *args unlocks a space of functions including many logical equivalents to **kwargs functions while still allowing us to tiptoe around this API a bit.
What do you think, @bdice ?
There was a problem hiding this comment.
Okay, that's the context I was missing. 😄 Let's move forward with this as-is, then. I'll review the rest of the PR.
| @@ -291,6 +291,14 @@ def foo(df): | |||
| got = got_grpby.apply(foo) | |||
| assert_groupby_results_equal(expect, got) | |||
|
|
|||
| def foo_args(df, k): | |||
There was a problem hiding this comment.
I would prefer to see this in a separate test function rather than combined with tests for functions that lack *args. Maybe the data generation can be shared via a helper function (a fixture could work but would be evaluated at collection time, which is undesirable). Similarly for the other tests.
There was a problem hiding this comment.
tbh I agree with you here. Let me rework these a bit.
There was a problem hiding this comment.
What do you think of doing it kind of like 7b27cc5 ?
|
@gpucibot merge |
Closes #10295