API: GroupBy.agg() numeric_only deprecation with custom function

[jorisvandenbossche]

Case: using groupby().agg() with a custom numpy funtion object (np.mean) instead of a built-in function string name ("mean").

When using pandas 1.5, we get a future warning about numeric_only going to change (#46072), and that you can specify the keyword:

>>> df = pd.DataFrame({"key": ['a', 'b', 'a'], "col1": ['a', 'b', 'c'], "col2": [1, 2, 3]})
>>> df
  key col1  col2
0   a    a     1
1   b    b     2
2   a    c     3
>>> df.groupby("key").agg(np.mean)
FutureWarning: The default value of numeric_only in DataFrameGroupBy.mean is deprecated. 
In a future version, numeric_only will default to False. Either specify numeric_only or select 
only columns which should be valid for the function.
     col2
key      
a     2.0
b     2.0

However, if you then try to specify the numeric_only keyword as suggested, you get an error:

In [10]: df.groupby("key").agg(np.mean, numeric_only=True)
...
File ~/miniconda3/envs/pandas15/lib/python3.10/site-packages/pandas/core/groupby/generic.py:981, in DataFrameGroupBy._aggregate_frame(self, func, *args, **kwargs)
    978 if self.axis == 0:
    979     # test_pass_args_kwargs_duplicate_columns gets here with non-unique columns
    980     for name, data in self.grouper.get_iterator(obj, self.axis):
--> 981         fres = func(data, *args, **kwargs)
    982         result[name] = fres
    983 else:
    984     # we get here in a number of test_multilevel tests

File <__array_function__ internals>:198, in mean(*args, **kwargs)

TypeError: mean() got an unexpected keyword argument 'numeric_only'

I know this is because when passing a custom function to agg, we pass through any kwargs, so numeric_only is also passed to np.mean, which of course doesn't know that keyword.

But this seems a bit confusing with the current warning message. So I am wondering if we should do at least either of both:

• If the current behaviour is intentional (numeric_only only working for built-in functions specified by string name), then the warning message could be updated to be clearer
• Could we actually support numeric_only=True for generic numpy functions / UDFs as well? (was this ever discussed before? Only searched briefly, but didn't directly find something)

cc @rhshadrach @jbrockmendel

[jorisvandenbossche]

If I turn the numpy function into an actual UDF by using a small wrapper:

def myfunc(group):
    return np.mean(group)

then we actually already do get a different (and clearer) deprecation warning:

>>> df.groupby("key").agg(myfunc)
FutureWarning: ['col1'] did not aggregate successfully. If any error is raised this will raise 
in a future version of pandas. Drop these columns/ops to avoid this warning.

     col2
key      
a     2.0
b     2.0

So at least we should update the warning for a numpy function to match that?

Although, what's the reason the numpy functions are taking a different code path? I seem to remember that numpy functions were actually mapped to our built-in functions, but if that's the case, then it should be easy to support numeric_only for those as well?

[jorisvandenbossche]

To illustrate, with this small (and very ugly) patch special casing the numeric_only keyword for deciding when a custom function with args/kwargs was passed (and thus to allow np.mean to be mapped to "mean", also when passing numeric_only=True) in Apply.__init__ and Apply.agg:

--- a/pandas/core/apply.py
+++ b/pandas/core/apply.py
@@ -122,7 +122,7 @@ class Apply(metaclass=abc.ABCMeta):
 
         # curry if needed
         if (
-            (kwargs or args)
+            ((kwargs and not (len(kwargs) == 1 and "numeric_only" in kwargs)) or args)
             and not isinstance(func, (np.ufunc, str))
             and not is_list_like(func)
         ):
@@ -165,8 +167,9 @@ class Apply(metaclass=abc.ABCMeta):
 
         if callable(arg):
             f = com.get_cython_func(arg)
-            if f and not args and not kwargs:
-                return getattr(obj, f)()
+            
+            if f and not args and (not kwargs or (len(kwargs) == 1 and "numeric_only" in kwargs)):
+                return getattr(obj, f)(**kwargs)
 
         # caller can react
         return None

then the following works (as expected?):

>>> df.groupby("key").agg(np.mean, numeric_only=True)
     col2
key      
a     2.0
b     2.0

[rhshadrach]

• If the current behaviour is intentional (numeric_only only working for built-in functions specified by string name), then the warning message could be updated to be clearer
• Could we actually support numeric_only=True for generic numpy functions / UDFs as well? (was this ever discussed before? Only searched briefly, but didn't directly find something)

The intention was to pass through kwargs so that groupby(...).agg('mean', ...) behaves the same as groupby(...).mean(...).

I seem to remember that numpy functions were actually mapped to our built-in functions, but if that's the case, then it should be easy to support numeric_only for those as well?

Only in some code paths do we use pandas.core.common._cython_table; in particular, if agg is computed via the code pandas.core.apply we will replace the NumPy method with the pandas method. Once kwargs are supplied, we change up code paths. As a side note, I don't think we should be doing this - if the user wants numpy.mean then they should get numpy.mean.

Related: #49534. There I advocated for special logic to apply the numeric_only argument (when provided) in the case of string aliases because the user shouldn't have to know that internally DataFrame.agg('mean') is calling Series.agg('mean').

I don't see any other options than those given in the OP.

Support numeric_only directly

To support numeric_only in agg, and I think we'd need to do apply and transform too, I would not recommend popping numeric_only from kwargs when supplied as it would be surprising behavior for anyone using a UDF with a numeric_only argument. Instead, I would prefer to add numeric_only to the method signature. The signature of groupby's aggregate would become:

def aggregate(self, func=None, *args, numeric_only=False, engine=None, engine_kwargs=None, **kwargs):

Doing this will break anyone's code that is supplying a UDF with a numeric_only argument. I think the implementation should be straightforward; the downsides to me are (a) it's technically a breaking change and (b) supporting more arguments in these methods.

Fix the warning

For this approach, we'd need to special case string arguments as in #49534 and fix the warning here. This is not a breaking change and doesn't involving adding to the API. The implementation is slightly tricky, but I believe doable. The real downside is it feels like playing whack-a-mole and I suspect there may be more moles to whack.

[jbrockmendel]

I vaguely remember being frustrated by this a while back, clearly didn't find a good solution at the time.

and I think we'd need to do apply and transform too

Agreed that consistency is a priority.

I'd lean towards not supporting numeric_only in agg etc at all. More trouble than its worth.

[jorisvandenbossche]

we will replace the NumPy method with the pandas method. .... As a side note, I don't think we should be doing this - if the user wants numpy.mean then they should get numpy.mean.

Yep, I agree, that's confusing (the numpy function might have different algorithmic / floating point characteristics, or even different default parameter values, for example compare agg(np.std) (replace with "std") vs agg(lambda x: np.std(x)), food for thought for a different issue (and unfortunately also a breaking change).

Related: #49534. There I advocated for special logic to apply the numeric_only argument (when provided) in the case of string aliases because the user shouldn't have to know that internally DataFrame.agg('mean') is calling Series.agg('mean').

That's indeed a related issue, thanks for the link.
I would say that if we special case string aliases or lists of string aliases, we should also do the same for numpy functions if those are mapped to string aliases.

Support numeric_only directly
...
Doing this will break anyone's code that is supplying a UDF with a numeric_only argument. I think the implementation should be straightforward; the downsides to me are (a) it's technically a breaking change and (b) supporting more arguments in these methods.

We could initially inspect the UDF and raise a (deprecation) warning if they have numeric_only in their UDF, warning that in the future this will not be forwarded to the UDF anymore.

It's indeed an additional keyword that would be added, but to be honest, in my mind we already had this keyword, since you could specify it in the common case .. (not realizing this only worked because .agg("mean", **kwargs) was actually calling .mean(**kwargs))

---

I don't have a very strong opinion on whether we should support numeric_only generally or not.

To describe the context where I encountered this: in geopandas we have a dissolve(key, aggfunc, ...) function which basically calls groupby(key).agg(aggfunc) but does a bunch of specific handling of the geometry data. Here, it would be nice that the user can also pass numeric_only=True to dissolve, but because we call the generic agg, this only works if the aggregation function the user specified is a string.
We could of course still select the numeric columns ourselves before calling agg to mimic this, but that gets a bit complicated with also taking into account the key columns etc.

[rhshadrach]

I would say that if we special case string aliases or lists of string aliases, we should also do the same for numpy functions if those are mapped to string aliases.

I believe this is already the case within a given code path. The issue here is that when no kwargs are provided we take the code path that maps to the pandas method, and when kwargs are provided we take the code path that doesn't map to the pandas method. Because of this inconsistency, I don't think there is a way to fix this where I would be comfortable to put it in a patch release. However I'm definitely open to suggestions here.

We could initially inspect the UDF and raise a (deprecation) warning if they have numeric_only in their UDF, warning that in the future this will not be forwarded to the UDF anymore.

This would not catch the case that a user provides a UDF accepting kwargs - but perhaps that is a small enough issue that it's okay.

It's indeed an additional keyword that would be added, but to be honest, in my mind we already had this keyword, since you could specify it in the common case .. (not realizing this only worked because .agg("mean", **kwargs) was actually calling .mean(**kwargs))

In your example we are just treating "mean" like the .mean function and passing through. If a user supplies a UDF that does not accept the numeric_only argument, then passing it will fail. On the other had, if we add numeric_only to the signature, we would then be implementing it for all arguments and subset the frame to numeric dtypes prior to doing any computation.

In short, I think these are significant enough behavior differences that I would not agree we already have this keyword (but that is very much an opinion).

I don't have a very strong opinion on whether we should support numeric_only generally or not.

I'm leaning toward not adding numeric_only to agg et al. I'll to put up a PR by the weekend implementing the fix to the warning (the issue here) and also resolving #49352 to see what this looks like.

[rhshadrach]

For #49352, I failed to realize that when passed a list, agg ignores all args / kwargs. This is a bug I plan to fix in 2.x, but it makes my solution above not viable. Instead, we can follow a similar pattern of rewording the warning that is raised.

Edit: Actually, the warning message for #49352 is already okay; I think it can be closed without fixing. I made a comment there.

[jorisvandenbossche]

The issue here is that when no kwargs are provided we take the code path that maps to the pandas method, and when kwargs are provided we take the code path that doesn't map to the pandas method. Because of this inconsistency, I don't think there is a way to fix this where I would be comfortable to put it in a patch release. However I'm definitely open to suggestions here.

We should still try to fix this inconsistency, though. It is very strange that depending on passing an argument or not, you get a different function that is applied in practice.

But long term we probably want to remove this special casing altogether? (do you know if there is already an issue about this?)

This would not catch the case that a user provides a UDF accepting kwargs - but perhaps that is a small enough issue that it's okay.

We could also check if the UDF accepts kwargs based on the signature ..

But yeah, I can certainly live with a decision to not generally support numeric_only. However, we should maybe look into giving some other ways to easily achieve this purpose?
In the end, under the hood it is "just" a ._get_numeric_data() call on the object on which to apply the aggregations. For normal dataframes, a user can do select_dtypes("numeric") to achieve the same through public APIs. Do we want to add this method to groupby().<..> as well? (you generally can't call it before the groupby, since your group key column might not be numeric)

[rhshadrach]

We should still try to fix this inconsistency, though. It is very strange that depending on passing an argument or not, you get a different function that is applied in practice.

But long term we probably want to remove this special casing altogether? (do you know if there is already an issue about this?)

Agreed - consolidating the paths through groupby is on my roadmap (has been for a while), but it's admittedly been slow going. There are many issues about inconsistencies in output from a user perspective, but not about specific internal paths.

We could also check if the UDF accepts kwargs based on the signature ..

That isn't sufficient though - we'd need to know if it is somehow utilizing numeric_only via kwargs, right?

But yeah, I can certainly live with a decision to not generally support numeric_only. However, we should maybe look into giving some other ways to easily achieve this purpose?

I don't have much opposition or support for it. Some thoughts

• I'm -1 on adding the keyword in 1.5.x (too big a change for a patch release)
• As far as I know no one has asked for it
• The more keyword arguments we add to groupby(...).agg, the more this can start interfering with what UDFs users can use. Maybe this isn't too important, I'm not certain. But it also makes me wonder if this is where a signature like def agg(func, args: list, kwargs: dict) would be better because we can then add kwargs with interfering (related: API: Signature of UDF methods #40112).

Do we want to add this method to groupby().<..> as well? (you generally can't call it before the groupby, since your group key column might not be numeric)

Similar to the above, I'm +0. It seems rather straightforward to support but I don't see it enhancing the API much either. One can do df[["B", "C"]].groupby(df["A"]).sum() though there are some issues with this (e.g. as_index won't work the same - which I'm planning to fix after deprecation ref: #49519)