BUG: Fix to GH34422 SeriesGroupBy works only with 'func' now

[gurukiran07]

• closes BUG: SeriesGroupBy works with any column name specified in NamedAgg #34422
• tests added / passed
• passes black pandas
• whatsnew entry

This PR tries to fix the bug pointed in #34422 where SeriesGroupBy.agg works with any given column name in NamedAgg.

After discussing with @TomAugspurger and @MarcoGorelli in another issue #34380 regarding this issue in the comments, We came to a solution that

So for SeriesGroupBy.agg, if there are any tuples or NamedAgg present in kwargs then I think we should raise.
Disallowing NamedAgg and tuples with SeriesGroupBy.

Before fix:

s = pd.Series([1,1,2,2,3,3,4,5])
s.groupby(s.values).agg(one = pd.NamedAgg(column='anything',aggfunc='sum'))
  one
1    2
2    4
3    6
4    4
5    5

s.groupby(s.values).agg(one=('something','sum'))
   one
1    2
2    4
3    6
4    4
5    5

After fix:

s = pd.Series([1,1,2,2,3,3,4,5])
s.groupby(s.values).agg(one = pd.NamedAgg(column='anything',aggfunc='sum'))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "d:\#gh34422\pandas\pandas\core\groupby\generic.py", line 243, in aggregate
    raise TypeError(tuple_given_message.format(type(kwargs[col]).__name__))
TypeError: 'func' is expected but recieved NamedAgg in **kwargs.

s.groupby(s.values).agg(one=('something','sum'))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "d:\#gh34422\pandas\pandas\core\groupby\generic.py", line 243, in aggregate
    raise TypeError(tuple_given_message.format(type(kwargs[col]).__name__))
TypeError: 'func' is expected but recieved tuple in **kwargs.

[pep8speaks]

Hello @gurukiran07! 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 2020-06-03 19:37:24 UTC

[MarcoGorelli]

Thanks for the PR

Can you add a test as well to show how it would close the linked issue?

[gurukiran07]

@MarcoGorelli I'm new to open source development. I'm going through writing-tests mentioned in contributing.rst . I'll add tests by tomorrow.

It's currently showing failed in 9 checks and 2 successful checks. How and where can I fix those 9 failing checks?

[TomAugspurger]

If you click through on the "Details" you should see the failures. e.g. https://dev.azure.com/pandas-dev/pandas/_build/results?buildId=36338&view=logs&j=bef1c175-2c1b-51ae-044a-2437c76fc339&t=770e7bb1-09f5-5ebf-b63b-578d2906aac9&l=105

[jreback]

always add tests first

[gurukiran07]

@jreback and @MarcoGorelli Added tests in test_aggregate.py.

[TomAugspurger]

Our documentation has contributing guidelines. Check those at let me know if you still have issues.

…

On May 29, 2020, at 05:12, guru kiran ***@***.***> wrote:  @TomAugspurger Thank you. I still have 1 failing check in Linting. How can I fix it? Please excuse me for asking so many doubts. I'm 18 years old with no prior experience in open source development. Thank you. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[gurukiran07]

@TomAugspurger Thank you. Now, all checks have been passed. Should have gone through "Coding style" part of contribution guidelines before.

[MarcoGorelli]

Thanks @gurukiran07 !

[MarcoGorelli]

Can msg be more specific?

[gurukiran07]

Yes, Can be extend msg = "func is expected but recieved NamedAgg" in this test, while using tuple msg = "func is expected but recieved tuple" and while using list msg = "func is expected but recieved list". Is this fine?

[TomAugspurger]

No, strings aren't callable. So it'd need to be a callable or str? Perhaps we already have something to check that.

…

On Mon, Jun 1, 2020 at 3:25 PM guru kiran ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pandas/core/groupby/generic.py <#34435 (comment)>: > if relabeling: columns = list(kwargs) - func = [kwargs[col] for col in columns] + func = [] + for col in columns: + if isinstance(kwargs[col], (list, tuple)): @TomAugspurger <https://github.com/TomAugspurger> aggfunc can be str sometimes right. s.groupby(...).agg(one='sum',two='max) Will callable return True the above example? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#34435 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAKAOITWLMYV7XWCPRMW6NLRUQFDNANCNFSM4NNKSIPQ> .

[gurukiran07]

@TomAugspurger

No, strings aren't callable. So it'd need to be a callable or str? Perhaps we already have something to check that.

str func is handled using getattr in the below lines may be we can use hasattr(self,col_func)?

[gurukiran07]

@TomAugspurger

if isinstance(func,str):
    return getattr(self,func)(*args,**kwargs)

These lines are hit when str is passed. I guess we can use hasattr or callable.

[jreback]

looks good. can you add a whatsnew note, groupby section of bug fixes for 1.1. ping on green.

[gurukiran07]

@jreback all 14 checks passed. And updated whatsnew v1.1.0.rst under groupby bug fixes.

[jreback]

minor comments, ping on green.

[gurukiran07]

@jreback All checks passed. Updated with mentioned changes.

[jreback]

thanks @gurukiran07 very nice!

[gurukiran07]

@jreback Thank you. It was a huge learning experience, got to learn lot of new stuff. Hope to contribute more in future.