-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
CLN: Implement multiindex handling for get_op_result_name #38323
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
Conversation
Can we do this by overriding get_reconciled_name_object and not get_op_result_name? id like to avoid the small-but-frequent additional overhead there |
pls merge master |
…n_get_op_result_name � Conflicts: � pandas/core/indexes/multi.py � pandas/tests/indexes/multi/test_setops.py
Done and Done |
pandas/core/ops/common.py
Outdated
@@ -122,3 +122,30 @@ def _maybe_match_name(a, b): | |||
elif b_has: | |||
return b.name | |||
return None | |||
|
|||
|
|||
def maybe_match_names_multiindex(a, b): |
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 type a, b and the return
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.
Don't need to type this after moving?
@@ -93,7 +93,7 @@ def _maybe_match_name(a, b): | |||
""" |
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.
would rename this to maybe_match_names
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.
Done
pandas/core/ops/common.py
Outdated
""" | ||
Try to find common names to attach to the result of an operation between | ||
a and b. Return a consensus list of names if they match at least partly | ||
or None if they have completely different names. |
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 could just be in padnas/core/indexes/multi.py i think would be fine
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.
Done
LGTM ex @jreback comments |
also pls merge master |
…n_get_op_result_name � Conflicts: � pandas/core/indexes/multi.py
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.
LGTM
looks good. is this user visible in any way? e.g you mention: |
No, we did use rename previously which avoided this. But now we are consistent between the intersection methods |
thanks @phofl |
…#38323) * CLN: Implement multiindex handling for get_op_result_name * Change import order * Override method * Move import * Remove import * Fix merge issue * Move methods
black pandas
git diff upstream/master -u -- "*.py" | flake8 --diff
@jbrockmendel
This would solve the inconsistency in
get_op_result_name
for MultiIndexes, but adds a bit more complexity here.I think this would allow us to remove the now duplicated code in
intersection