-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
ENH: pd.NamedAgg forwards *args and **kwargs to aggfunc #58852
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
Closed
Closed
Changes from all commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
14d4973
Named agg allow kwargs
tomytp 8ba87ac
fixed NamedAgg initialization
tomytp 816aa17
Included NammedAgg changes in whatsnew
tomytp ed6b650
simplified NamedAgg class
tomytp 31611a2
changed to callsuper
tomytp 3ecf296
fix typo in whatsnew
tomytp 240cc96
Fix __new__ error in NamedAgg
tomytp 7f752d5
fix attribute error in NamedAgg
tomytp ae11c8e
fix NamedAgg docstring
tomytp f30f13d
fix docstring NamedAgg
tomytp 1c06491
Fix whatsnew formatting
tomytp 7202de9
rollback changes from doc
tomytp a563ec1
removed unnecessary dunder implementation
tomytp File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
What's the reason for introducing a new class here?
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.
That's because you cannot override new when inheriting from NamedTuple directly.
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.
So the most elegant way I found to override it while maintaining the NamedTuple properties was to Introduce a new class. Do you have a suggestion for another method to do it?
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.
the reference error for when I tried to inherit directly from NamedTuple is:
AttributeError: Cannot overwrite NamedTuple attribute __new__
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.
Ah that's unfortunate. This seems kind of hacky as a result though - maybe we can just drop the NamedTuple subclass? I think that was mostly developed that way as a convenience rather than an intentional design decision - I am not sure why NamedAgg would need to be "tuple-ish"
@rhshadrach might have more thoughts
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.
python/cpython#77258 has some relevant information, especially the linked discussion there to the issue in
python/typing
.In any case, it looks to me like this implementation just ignores
args
/kwargs
when the providedaggfunc
is not a callable? If that's the case, then I'm -1 on this approach anyways.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.
@rhshadrach I'll rethink the approach to include the function names passed as string.
I'm not sure I understand your thoughts on the necessity of keeping NamedArgs with tuple behavior. Could you clarify that?
TY
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.
I don't believe I shared any thoughts in this regard, just a link to some background information on why Python raises when you try to overwrite
__new__
.