-
-
Notifications
You must be signed in to change notification settings - Fork 19.4k
BUG: copy np.ndarray inputs to Index constructor by default #63370
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
BUG: copy np.ndarray inputs to Index constructor by default #63370
Conversation
|
We essentially already do a shallow copy of Index inputs in the To clarify my suggestion:
I meant "doing the same for the Index constructor" (and not doing the same for Index input to the Series constructor ..). I see that the above sentence could have been interpreted both ways, sorry ;) So that |
Ah makes sense. No problem, I mimicked the copying we do in the
|
jorisvandenbossche
left a comment
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.
Looks good!
| if isinstance(data, (set, frozenset)): | ||
| data = list(data) | ||
|
|
||
| elif is_ea_or_datetimelike_dtype(data_dtype): |
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.
For datetime64 ndarray, we wouldn't get in the elif block below that now copies, but this is what you mentioned to handle in the Index subclasses in a follow-up?
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.
My comment was referring passing an ndarray to DatetimeIndex et. al., but good find, yes I think I'll have to put this check higher up.
| data=None, | ||
| dtype=None, | ||
| copy: bool = False, | ||
| copy: bool | None = 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.
You can also use the updated description in the Series docstring as a starting point to update the docstring here as well (as the end goal should be to make it similar)
@jorisvandenbossche I went with your suggestion to
in #63306 (comment)