Skip to content

Fix unpickling/__setstate__ in TransformNode #4070

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
Closed
wants to merge 1 commit into from
Closed

Fix unpickling/__setstate__ in TransformNode #4070

wants to merge 1 commit into from

Conversation

joergdietrich
Copy link
Contributor

Fix the unpickling error in #4068 and add a test for it.

tacaswell
tacaswell reviewed Feb 6, 2015
View reviewed changes
lib/matplotlib/transforms.py
@@ -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.

@joergdietrich
Copy link
Contributor Author

I'm not sure anymore I understand myself what's going on. This PR definitely fixes the exception during unpickling. However, the unpickled figure cannot be plt.show()n:

/home/joerg/src/matplotlib/lib/python2.7/site-packages/matplotlib-1.5.dev1-py2.7-linux-x86_64.egg/matplotlib/projections/polar.py in get_matrix(self)
    101 
    102     def get_matrix(self):
--> 103         if self._invalid:
    104             limits_scaled = self._limits.transformed(self._scale_transform)
    105             yscale = limits_scaled.ymax - limits_scaled.ymin

AttributeError: 'PolarAffine' object has no attribute '_invalid'

I'll investigate.

@tacaswell tacaswell added this to the v1.5.x milestone Feb 7, 2015
@tacaswell tacaswell modified the milestones: proposed next point release, next point release Feb 19, 2015
@tacaswell
Copy link
Member

@joergdietrich Any progress on this?

@joergdietrich
Copy link
Contributor Author

No, sorry. Unexpectedly got flooded with other things. This PR does not fix the original problem at all. The exception during unpickling in fact does not occur on my devel branch without this PR. I created a bit of a mess with different environments when trying to fix this. This PR should be closed without merging.

It will be a few weeks before I can look into #4068. I'd be happy if somebody else wants to take a look at it.

@tacaswell
Copy link
Member

@joergdietrich No worries. I have great sympathy for both being over-subscribed and python environments fighting back!

@tacaswell tacaswell closed this Mar 17, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v1.5.0
Development

Successfully merging this pull request may close these issues.
4 participants
@joergdietrich @tacaswell @WeatherGod @pelson