I think this is ready for review. Assuming the direction this is moving us is good, still need to decide if we are okay with this being a breaking change in 3.0 (my preference) or if it should be deprecated. If we do go the deprecation route, it will be noisy (many cases where results will be the same but we can't tell so need to warn). The only way I see a deprecation working is if we add an option, e.g.future_groupby_agg so that users can opt in to the new implementation.

cc@jorisvandenbossche@MarcoGorelli@Dr-Irv@mroeschke for any thoughts.

I could generally be OK making this a "breaking bug change" for 3.0. Just 2 points:

1. IsDataFrame.agg already strict like this?
2. ForRaise on anything detected as being a non-scalar, I would be open to still allowing.agg to store a non-scalar, i.e. nested value, as a result element and returndtype=object. But these values should never expand the dimensions of the result when usingagg

I'm not going to review the whole code change - beyond what I understand about how this all works, but I think the example I wrote here should be in the tests:
#33242 (comment)

@mroeschke

1. IsDataFrame.agg already strict like this?

Great question - unfortunately the answer is no. We useDataFrame.apply under the hood. Perhaps if we are going to do this with groupby, we should also do it across the board. That would make me lean more toward introducing something like afuture.agg option.

2. ForRaise on anything detected as being a non-scalar, I would be open to still allowing.agg to store a non-scalar, i.e. nested value, as a result element and returndtype=object. But these values should never expand the dimensions of the result when usingagg

Agreed. I believe that's the case here for a UDF, but not strings (e.g.cumsum). This is#44845 - but I'd like to focus on UDFs here and work on string arguments separately.

@Dr-Irv

I'm not going to review the whole code change - beyond what I understand about how this all works, but I think the example I wrote here should be in the tests:

Certainly - I added this astest_unused_kwargs. In that test, we usenp.sum(np.sum(data)). This way works both on column and frame inputs, and would raise if we pass a frame instead of column-by-column. Is this sufficient?

Certainly - I added this astest_unused_kwargs. In that test, we usenp.sum(np.sum(data)). This way works both on column and frame inputs, and would raise if we pass a frame instead of column-by-column. Is this sufficient?

I'm not sure. In the example I created, you had 2 functions, one with 1 argument, the other with 2 arguments, and what was being passed to those 2 functions was different because of the number of arguments. I don't see how the test you created confirms thatSeries are passed independent of the function declaration.

So maybe you should have a test like this that addresses the particular issue in#33242 :

deftwoargs(x,y):assertisinstance(x,pd.Series)returnx.sum()deftest_two_args(self):df=pd.DataFrame({'a': [1,2,3,4,5,6],'b': [1,1,0,1,1,0],'c': ['x','x','x','z','z','z'],'d': ['s','s','s','d','d','d']})df.groupby('c')[['a','b']].agg(twoargs,0)

I don't see how the test you created confirms thatSeries are passed independent of the function declaration.

We don't ever inspect the UDF to see what arguments it can take - our logic branches on whether additional arguments are passed in the call toagg:.agg(func, 0) vs.agg(func) previously resulted in two different paths, one which passed the DataFrame and one which passed each Seriers.

Still, no opposition to an additional test here. Will add.