-
-
Notifications
You must be signed in to change notification settings - Fork 18.3k
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
move rename functionality out of internals #21924
Conversation
Codecov Report
@@ Coverage Diff @@
## master #21924 +/- ##
=========================================
+ Coverage 92.04% 92.15% +0.1%
=========================================
Files 169 169
Lines 50782 50698 -84
=========================================
- Hits 46742 46720 -22
+ Misses 4040 3978 -62
Continue to review full report at Codecov.
|
pandas/core/generic.py
Outdated
result._data = result._data.rename_axis(f, axis=baxis, copy=copy, | ||
level=level) | ||
|
||
# TODO: we already did a copy above, can we avoid doing again? |
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 think this makes it more confusing here, reaching into internals to do this.
I would rather move _transform_index and items_overlap_with_suffix from internals to Index.base (or appropriate), these are just renaming / transform helpers for an index.
Then this would be much cleaner.
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 think this makes it more confusing here, reaching into internals to do this.
At least add_suffix and add_prefix are clear wins. I can de-_data
this a little bit by using NDFrame.set_axis
. I'll take a look at transform_index and items_overlap_with_suffix, see if there is a more reasonable place for them.
I would rather [...]
I agree, am working along similar lines.
@jorisvandenbossche gentle ping; the issue that motivated this PR was yours |
Hello @jbrockmendel! Thanks for updating the PR.
|
Updated to get only add_prefix/add_suffix out of internals, leaves |
@jorisvandenbossche this has been scaled back significantly, but still addresses part of the Issue you opened. 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.
No objection to this
pandas/core/internals/managers.py
Outdated
@@ -169,27 +169,17 @@ def set_axis(self, axis, new_labels): | |||
def rename_axis(self, mapper, axis, copy=True, level=None): | |||
""" | |||
Rename one of axes. | |||
|
|||
Parameters | |||
Parameters |
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 you undo this change?
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.
Copy/paste snafu, thanks for pointing this out. Will fix shortly.
pandas/core/internals/managers.py
Outdated
---------- | ||
mapper : unary callable | ||
axis : int | ||
copy : boolean, default True | ||
level : int, default None | ||
|
||
""" | ||
""" |
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.
this one as well
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 will have a look soon
thanks! |
closes CLN: move rename functionality out of internals #16045updateNot anymoregit diff upstream/master -u -- "*.py" | flake8 --diff
@jorisvandenbossche can you confirm this is what you had in mind in that issue?