Skip to content

FIX: Remove the deepcopy override from transforms #21597

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.

Sign up for GitHub

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 1 commit into from
Feb 2, 2022
Merged

FIX: Remove the deepcopy override from transforms #21597

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions lib/matplotlib/tests/test_figure.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import copy
from datetime import datetime
import io
from pathlib import Path
Expand Down Expand Up @@ -1261,3 +1262,28 @@ def test_kwargs_pass():

assert fig.get_label() == 'whole Figure'
assert sub_fig.get_label() == 'sub figure'


def test_deepcopy():
fig1, ax = plt.subplots()
ax.plot([0, 1], [2, 3])
ax.set_yscale('log')

fig2 = copy.deepcopy(fig1)

# Make sure it is a new object
assert fig2.axes[0] is not ax
# And that the axis scale got propagated
assert fig2.axes[0].get_yscale() == 'log'
# Update the deepcopy and check the original isn't modified
fig2.axes[0].set_yscale('linear')
assert ax.get_yscale() == 'log'

# And test the limits of the axes don't get propagated
ax.set_xlim(1e-1, 1e2)
# Draw these to make sure limits are updated
fig1.draw_without_rendering()
fig2.draw_without_rendering()

assert ax.get_xlim() == (1e-1, 1e2)
assert fig2.axes[0].get_xlim() == (0, 1)
16 changes: 0 additions & 16 deletions lib/matplotlib/transforms.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,22 +151,6 @@ def __copy__(self):
other.set_children(val) # val == getattr(other, key)
return other

def __deepcopy__(self, memo):
# We could deepcopy the entire transform tree, but nothing except
# `self` is accessible publicly, so we may as well just freeze `self`.
other = self.frozen()
if other is not self:
return other
# Some classes implement frozen() as returning self, which is not
# acceptable for deepcopying, so we need to handle them separately.
other = copy.deepcopy(super(), memo)
# If `c = a + b; a1 = copy(a)`, then modifications to `a1` do not
# propagate back to `c`, i.e. we need to clear the parents of `a1`.
other._parents = {}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is the relevant problem: you don't want changes to the copy to invalidate dependents of the original instance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think your previous tests captured that case?

matplotlib/lib/matplotlib/tests/test_transforms.py

Line 709 in b528d5c

def test_deepcopy():

I left all of the previous tests in there. The override of the __copy__() method is still required to keep the parent/child references intact.

Copy link
Contributor

@anntzer anntzer Nov 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Oh, I see. This is because the default deepcopy is also going to deepcopy _parents, creating new dependents (that don't actually exist anywhere except as _parents entries, so they should in fact even get gc'ed out at some point?); in any case this means that we indeed don't need to manually clear out _parents like I previously did.

I haven't thought about the whether the intended figure deepcopy makes sense, but at least, from the pov of deepcopying transforms, I'm happy with the change here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 I think my take on deepcopy is that it should be the same as if I were to pickle dump and then load, which we are already testing, so it should be feasible to achieve I think. Something like this:

__deepcopy__(self):
    return pickle.loads(pickle.dumps(self))

But, we don't even need to do that in this case because the standard deepcopy (with this update) works on the figure object. Thats what I'm trying to get at in the added test and just show that we get new objects with the proper scales set from the base object.

# If `c = a + b; c1 = copy(c)`, this creates a separate tree
# (`c1 = a1 + b1`) so nothing needs to be done.
return other

def invalidate(self):
"""
Invalidate this `TransformNode` and triggers an invalidation of its
Expand Down