-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
BUG: passing str to GroupBy.apply #42021
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
760f8fd
10bfb8c
04bcb28
736ad9f
e14af63
6cedf1d
0b1e549
09c70d2
dc83cff
6f29251
4dbc5ce
b70f706
2445055
3c004a2
1a97534
27e63be
c0742ae
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1245,6 +1245,16 @@ def f(g): | |
"func must be a callable if args or kwargs are supplied" | ||
) | ||
else: | ||
if isinstance(func, str): | ||
if hasattr(self, func): | ||
res = getattr(self, func) | ||
if inspect.isfunction(res) or inspect.ismethod(res): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if |
||
return res() | ||
return res | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why are you returning? does any test actually hit this? should just fall thru no? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, several of the test_empty_groupby tests get here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we only raise, is there any issue? I.e. to just do
Or perhaps there is some benefit to adding the call to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you mean return There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not quite - I put my suggestion (that doesn't work) below in case you're curious, but see now this PR isn't solely fixing the case of an invalid str argument, but also addressing the
|
||
|
||
else: | ||
raise TypeError(f"apply func should be callable, not '{func}'") | ||
|
||
f = func | ||
|
||
# ignore SettingWithCopy here in case the user mutates | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this be an elif
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated +greenish