-
-
Notifications
You must be signed in to change notification settings - Fork 19.4k
CLN: simplify MultiIndex._shallow_copy #32772
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
CLN: simplify MultiIndex._shallow_copy #32772
Conversation
713db7c to
fb4f8ec
Compare
| if values is not None: | ||
| names = kwargs.pop("names", kwargs.pop("name", self.names)) | ||
| # discards freq | ||
| kwargs.pop("freq", 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.
Dropping this silently makes no sense, freq isn't accepted by any MultiIndex constructor. Just let it be passed in and raise a TypeError if encountered.
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
|
LGTM |
| @Appender(Index._shallow_copy.__doc__) | ||
| def _shallow_copy(self, values=None, **kwargs): | ||
| if values is not None: | ||
| names = kwargs.pop("names", kwargs.pop("name", self.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.
do we need to worry about both name and names being passed?
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.
MultiIndex.from_tuples only accepts names, and Mi uses name in some locations, so yes (but it a part of the ickyness of MultiIndex, so should ideally be refactored away).
|
thanks @topper-123 |
Minor simplification of
MultiIndex._shallow_copy.