-
-
Notifications
You must be signed in to change notification settings - Fork 19.3k
REF: Make deep keyword only in Block(Manager).copy #62969
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
| new_axes = [copy_func(ax) for ax in self.axes] | ||
| else: | ||
| new_axes = [ax.view() for ax in self.axes] | ||
| new_axes = [ax.view() for ax in self.axes] |
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.
Reason that this is removed?
I know that an Index is generally immutable so the difference between view or actual copy is not relevant when it comes to the data, but we still some mutable attributes
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.
deep="all" is not passed anymore AFAICT, so copy_func also becomes [ax.view() for ax in self.axes]. Should I change the view to copy to be safe?
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.
Ah, sorry, missed the fact that the copy() call was only done for deep="all" ..
Yes, I would expect that we actually copy the indices in case of a deep=True
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.
Hmm, it seems we have some tests that verify the shallow-ness of the copy (checking identity)
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.
Ah darn. I'll leave a TODO comment here and just swap back to the existing behavior since this PR is just meant to not change behavior.
A change to make it more explicit internally when we need to deep vs shallow copy a Block or Manager. Keeps the current behavior