Skip to content
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

Jump to bottom

Fix unpickling/__setstate__ in TransformNode #4070

Closed
wants to merge 1 commit into from
Closed

Fix unpickling/__setstate__ in TransformNode #4070

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
11 changes: 11 additions & 0 deletions lib/matplotlib/tests/test_transforms.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
from __future__ import (absolute_import, division, print_function,
unicode_literals)

import pickle
import six
from six.moves import xrange, zip

Expand Down Expand Up @@ -502,6 +503,16 @@ def test_log_transform():
ax.transData.transform((1,1))


@cleanup
def test_unpickling_transformnode():
# Before matplotlib 1.4.2 unpickling a polar plot would throw an
# AttributeError
ax = plt.subplot(111, polar=True)
fig = plt.gcf()
pfig = pickle.dumps(fig)
fig2 = pickle.loads(pfig)


if __name__=='__main__':
import nose
nose.runmodule(argv=['-s','--with-doctest'], exit=False)
2 changes: 1 addition & 1 deletion lib/matplotlib/transforms.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ def __getstate__(self):
def __setstate__(self, data_dict):
self.__dict__ = data_dict
# turn the normal dictionary back into a WeakValueDictionary
self._parents = WeakValueDictionary(self._parents)
self._parents = WeakValueDictionary(self.__dict__['_parents'])
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain why this changes anything?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The weak reference is destroyed during pickling, thus self._parents does not exist anymore at this point.__getstate__ however saved it in data_dict['_parents'] from which it is now restored.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is doing what you thing it is. Previously we were restoring the _parents from the stored data dictionary, now this is restoring parents from self, which hasn't yet been initialised. Hence, _parents will simply be empty (https://github.com/joergdietrich/matplotlib/blob/%234068/lib/matplotlib/transforms.py#L97).

Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is doing what you thing it is.

Scrap that. I think I'm talking rubbish. (Friday afternoon!)

Copy link
Member

Choose a reason for hiding this comment

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

I think you've found a bug with the setstate implementation, and I don't think it has previously been tested correctly.

How about simply doing data_dict['_parents'] though. It may be more clear what is going on that way.

Would you also mind adding a test that self._parents actually is accessible after being restored?

Copy link
Member

Choose a reason for hiding this comment

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

I still don't understand... shouldn't self._parents be equivalent to
self.__dict__['_parents'], since the object dictionary would be where one
accesses attributes of the object?

On Fri, Feb 6, 2015 at 11:16 AM, Phil Elson notifications@github.com
wrote:

In lib/matplotlib/transforms.py
#4070 (comment):

@@ -115,7 +115,7 @@ def getstate(self):
def setstate(self, data_dict):
self.dict = data_dict
# turn the normal dictionary back into a WeakValueDictionary

  •    self._parents = WeakValueDictionary(self._parents)

  •    self._parents = WeakValueDictionary(self.__dict__['_parents'])

I don't think this is doing what you thing it is.

Scrap that. I think I'm talking rubbish. (Friday afternoon!)

—
Reply to this email directly or view it on GitHub
https://github.com/matplotlib/matplotlib/pull/4070/files#r24250906.


def __copy__(self, *args):
raise NotImplementedError(
Expand Down