BUG: Lambda function returns KeyError in DataFrameGroupBy.agg

[charlesdong1991]

• closes Multiple lambdas for the same column return KeyError in DataFrameGroupBy.agg with named aggregation #27519
• tests added / passed
• passes black pandas
• passes git diff upstream/master -u -- "*.py" | flake8 --diff
• whatsnew entry

[jreback]

List of columns indices

[TomAugspurger]

This is growing to be pretty complex. I wonder: is it possible to pass order into maybe_mangle_lamdas? Then when we iterate over elements of func we might update the corresponding name in order?

[charlesdong1991]

emm, not sure, could take a look, but probably will change few functions as well. This looks indeed a bit complex, but I found out there are quite few issues around it:

1. allow lambda in groupby.agg
2. correctly order different lambda functions and dataset which involves different lambdas on same column, mixed lambda and aggfunc.
3. potentially could remove duplication warning, right now, if gb.agg(a=(col, 'min'), b=(col, 'min')) would raise an error, but gb.agg(a=(col, 'min'), b=(col, lambda x: np.min(x))) won't. Users might actually want to do it, so that they could use b column for other ops while based on np.min result. There is also an error test in test_aggregate.py which could be potentially removed.
   But I will take a look tomorrow to see if I can come up with a simpler version. @TomAugspurger

[pep8speaks]

Hello @charlesdong1991! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-08-26 18:03:11 UTC

[charlesdong1991]

Hi, @TomAugspurger I simplified the code quite a lot, but still in _normalize_keyword_aggregation because i feel it is better than changing _maybe_mangle_lambdas. Right now, hopefully it wont look too complex. And I also add more complex test cases to prove the lambda works in different scenarios and order of columns is correct. It looks all good locally so feel free to take a look. ^^

[TomAugspurger]

Looks better, thanks.

[TomAugspurger]

Can you add a few unit tests that directly test _uniquify_aggfunc?

[TomAugspurger]

Can you verify that the output example in the docstring still passes?

Can you also add a docstring example were this new code is hit in https://github.com/pandas-dev/pandas/pull/27921/files#diff-bfee1ba9e7cb79839776fac1a57ed940R1742?

[charlesdong1991]

yeah, verified. And added tests for _make_unique function, all tests pass locally @TomAugspurger

[charlesdong1991]

wanna give a follow-up review? @TomAugspurger

[TomAugspurger]

Thanks!