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.
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
Convert DataFrame.rename to keyword only; simplify axis validation #29140
Convert DataFrame.rename to keyword only; simplify axis validation #29140
Changes from 17 commits
f09c6df
66acc90
177a440
b66e9e2
2415e46
296a9df
6ba8518
4128780
2776c7d
8e20fa4
cca8eed
1f8e1cf
4dc8bbe
1b261f8
b5e54bd
ab1ee2e
af0b7c5
d5d812c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Change here was to be explicit about what is accepted
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.
+1 to being explicit. The only reason we had *args, **kwargs in the first place was to support the ambiguous case.
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.
Side note - there is only one other use now of this
validate_axis_style_args
function (seeDataFrame.reindex
). The intent here is good, but I think the fact that it mutates the args and kwargs being passed it makes it very difficult to reason about. As a follow up to this would like to replace entirely with something simple that just operates on the axis argumentThere 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.
Is there anything blocking a similar PR for
.reindex
?Is this referring to DataFrame.reindex, or validate_axis_style_args? Ideally we can completely remove
validate_axis_style_args
.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 didn't look before because I was trying to keep this focused, but it might not be too much extra effort. I'll take a look and if it gets out of hand let you know
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.
Ultimately I am trying to get rid of all of the internal
_AXIS_*
functions and mappings in core.generic, which seem like relics of the panel daysIf we buy into just having
index
andcolumns
being the potential axes in that order things can be greatly simplifiedThere 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.
Couldn't seem to get dict unpacking to place nicely with mypy. @simonjayhawkins
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 Series method doesn't completely align with the DataFrame method, but I think this is an easy way to go about it while maintain backwards compat
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 value in accepting
axis
here, if we don't also havemapper
and columns?I don't see an easy way to accept
mapper
. That's essentially fulfilled byindex
.So maybe my question is: should we also accept
columns=
? (Not arguing for this btw. I don't know what's best 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.
Do we validate axis at all, or just ignore?
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.
Here axis gets ignored. Looks like this was implemented as part of #18589
Yea I'm not sure myself. I was thinking ideally it would have been great to align the signatures better but for where we are that would cause some API churn. So I guess best to leave unless there's a ringing need to do something here (?)