Skip to content

ENH: Implement dict-like support for rename and set_names in MultiIndex #38126

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

Merged
merged 16 commits into from
Jan 4, 2021

Conversation

phofl
Copy link
Member

@phofl phofl commented Nov 28, 2020

I tried my hand in adding support for dict-like renamings for MultiIndexes. This seemed as the most straight forward way to me, because we do not have to add complex logic as long as we can cast the dict-like objects to regular configurations.

We probably must add a docstring to MultiIndex.rename now?

@@ -1366,6 +1367,12 @@ def set_names(self, names, level=None, inplace: bool = False):
( 'cobra', 2018),
( 'cobra', 2019)],
names=['species', 'year'])
>>> idx.set_names({'kind': 'snake'})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add a comment on this example (skip a line)

Copy link
Member Author

@phofl phofl Nov 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -1385,6 +1398,12 @@ def set_names(self, names, level=None, inplace: bool = False):
idx = self
else:
idx = self._shallow_copy()

if isinstance(self, ABCMultiIndex) and is_dict_like(names):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add a comment on what this condition is doing?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -1385,6 +1398,12 @@ def set_names(self, names, level=None, inplace: bool = False):
idx = self
else:
idx = self._shallow_copy()

if isinstance(self, ABCMultiIndex) and is_dict_like(names):
level = Index(self.names).get_indexer_for(names)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you not just iterate over these to grab the levels? (and use get_level_values)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, thx. Thats way better.

List comprehensions would be an option too, but would have to loop twice. Would prefer ist this way

Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see comments

@@ -1376,6 +1393,12 @@ def set_names(self, names, level=None, inplace: bool = False):
if not is_list_like(names) and level is None and self.nlevels > 1:
raise TypeError("Must pass list-like as `names`.")

if is_dict_like(names) and not isinstance(self, ABCMultiIndex):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all of these conditions can be if/elif

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wanted to be consistent with the existing conditions, changed them to elif


if is_dict_like(names) and level is not None:
raise TypeError("Can not pass level when passing dict-like as `names`.")

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't this hit a dict-like names?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yikes, yes you are right. have not though about dict like names. Removed this

if isinstance(self, ABCMultiIndex) and is_dict_like(names):
# Transform dict to list of new names and corresponding levels
level, names_adjusted = [], []
for i, name in enumerate(self.names):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see my comment above i think this condition needs to be on L1402

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved it up a bit and added level is None

@jreback
Copy link
Contributor

jreback commented Dec 2, 2020

cc @Dr-Irv if you'd have a look. this is api you were expecting (from the OP)?

Copy link
Contributor

@Dr-Irv Dr-Irv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs tests for the basic use case in the original issue.

@Dr-Irv
Copy link
Contributor

Dr-Irv commented Dec 2, 2020

cc @Dr-Irv if you'd have a look. this is api you were expecting (from the OP)?

Yes, that is the API. I didn't check the implementation, but the tests should reflect the "normal" use case of having a MultiIndex with names that are different for each level, and you want to change one or two of the names. Also, should check that behavior is consistent with DataFrame.rename() if a name in the dict argument is not one of the level names - I'm not sure what we do in DataFrame.rename() if that is the case.

@phofl
Copy link
Member Author

phofl commented Dec 2, 2020

@Dr-Irv

Concerning missing names: I have oriented me on df.rename, as you also suggested. df.rename does nothing for missing index elements or column names.

Additionally for duplicates: Same holds true here. Is also done like df.rename. It produces duplicates if we have the same name twice in the original and it also produces duplicates if the targets already exist.

Copy link
Contributor

@Dr-Irv Dr-Irv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Final approval rests with @jreback

…421_before_merge

� Conflicts:
�	doc/source/whatsnew/v1.2.0.rst
…421_before_merge

� Conflicts:
�	doc/source/whatsnew/v1.3.0.rst
@phofl
Copy link
Member Author

phofl commented Jan 3, 2021

ping

If the index is a MultiIndex and names is not dict-like, level(s) to set
(None for all levels). Otherwise level must be None.

.. versionchanged:: 1.2.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1.3

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -1425,16 +1433,37 @@ def set_names(self, names, level=None, inplace: bool = False):
( 'cobra', 2018),
( 'cobra', 2019)],
names=['species', 'year'])

When renaming levels through a dictionary no level can't be passed.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When renaming levels with a dict, levels can not be passed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@jreback jreback added this to the 1.3 milestone Jan 4, 2021
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment, ping on green

Name(s) to set.

.. versionchanged:: 1.2.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change to 1.3

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@phofl
Copy link
Member Author

phofl commented Jan 4, 2021

@jreback green

@jreback jreback merged commit e4614cb into pandas-dev:master Jan 4, 2021
@jreback
Copy link
Contributor

jreback commented Jan 4, 2021

thanks @phofl

@phofl phofl deleted the 20421 branch January 4, 2021 18:56
luckyvs1 pushed a commit to luckyvs1/pandas that referenced this pull request Jan 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

API: Allow MultiIndex.rename() to accept a dict as an argument
3 participants