TransformWrapper pickling fixes

[QuLogic]

Calling set_children may or may not work if the child has been unpickled yet, and it actually causes duplicate parents to be set in the times when it does appear to work.

Using the example from #4908 with some additional prints:

import pickle
import matplotlib.transforms as mtransforms

class TestTransform(object):
    def __init__(self):
        self.identity = mtransforms.IdentityTransform()
        self.identity2 = mtransforms.IdentityTransform()
        self.composite = self.identity + self.identity2
        self.wrapper = mtransforms.TransformWrapper(self.composite)
        #self.wrapper = mtransforms.TransformWrapper(self.identity)

c = TestTransform()
print(c.wrapper._child, list(c.wrapper._child._parents.items()))
pkl = pickle.dumps(c)
ret = pickle.loads(pkl)
print(ret.wrapper._child, list(ret.wrapper._child._parents.items()))

If it succeeds, the result is:

$ python3 test_pickle.py 
IdentityTransform() [(139932718695928, TransformWrapper(IdentityTransform()))]
IdentityTransform() [(139932718695928, TransformWrapper(IdentityTransform())), (139932718696656, TransformWrapper(IdentityTransform()))]

You can see that the unpickled child transform has a second fictitious parent.

So the simple fix appears to be to avoid calling set_children...

Fixes #4908.

[QuLogic]

Actually, now that I think about it, is there any reason to override __getstate__ at all for TransformWrapper? I'm not sure it's necessary, but maybe there are edge cases I'm not accounting for...

[tacaswell]

@pelson you have any insight into this?

[tacaswell]

This is a really heavy-handed solution to this problem. I think it would be better to pull the re-usable stuff out into a private function rather than add an extra kwarg. Adding a private function is not an API change, where as adding a kwarg is (granted a back-compatible one). More importantly, removing a private function is not an API where as removing a kwarg definitely is a breaking change.

[QuLogic]

You're probably right but I'm still wondering why TransformWrapper needs its own __getstate__ in the first place and why it can't just defer to its grandparent's TransformNode.__getstate__. It seems to be working, but maybe I've missed something...

[tacaswell]

In cases like this it is usually worth doing the forensics to sort out when that change came into the code base.

[QuLogic]

Normally a good idea, but unfortunately, the blame shows it was added to TransformWrapper in the exact same commit that it was added to TransformNode. I guess we'd need to wait on @pelson for the full reasoning.

[QuLogic]

OK, I see it's a Python 2 thing; I'll go with your suggestion then.

[QuLogic]

I also noticed that since TransformWrapper overrides __[gs]etstate__, it doesn't save and restore its own parents, so I've added that on as well. This updated test looks at both children and parents:

from __future__ import print_function
import pickle
import matplotlib.transforms as mtransforms

class TestTransform(object):
    def __init__(self):
        self.identity = mtransforms.IdentityTransform()
        self.identity2 = mtransforms.IdentityTransform()
        self.composite = self.identity + self.identity2
        self.wrapper = mtransforms.TransformWrapper(self.composite)
        self.wrapper2 = mtransforms.TransformWrapper(self.wrapper)

c = TestTransform()
print(c.wrapper2._child, list(c.wrapper2._child._parents.items()))
print(c.wrapper._child, list(c.wrapper._child._parents.items()))
pkl = pickle.dumps(c)
del c
ret = pickle.loads(pkl)
print(ret.wrapper2._child, list(ret.wrapper2._child._parents.items()))
print(ret.wrapper._child, list(ret.wrapper._child._parents.items()))

[tacaswell]

I suspect that is why the set child is there.

Why would setting the parents do any better that setting children if the
order is random?

On Fri, Aug 14, 2015, 12:01 AM Elliott Sales de Andrade <
notifications@github.com> wrote:

I also noticed that since TransformWrapper overrides [gs]etstate, it
doesn't save and restore its own parents, so I've added that on as well.

—
Reply to this email directly or view it on GitHub
#4915 (comment)
.

[QuLogic]

The pickling failed because of setting parents on the child. The additional commit here sets the parents for itself.

[tacaswell]

It is not clear that this is going to work, are there going to be other refs to these instances of the objects will be held so they won't get immediately gc'd

[QuLogic]

This is the same method used in the grandparent TransformNode, so if it were going to fail, it would have done so a while ago.

There should be a strong reference from parent -> child assuming the transform's implemented correctly.

[tacaswell]

fair enough.

[tacaswell]

This seems reasonable and the tests pass, but this is a bit of the code that I know very little about

[QuLogic]

Unfortunately, I think @pelson is on vacation at the moment.

[tacaswell]

As is @mdboom and @efiring is at sea!

[pelson]

@QuLogic - would you mind adding a unit test? If nothing else, it will make reviewing the change easier.
This stuff gets pretty deep quickly when dealing with weakkey reference recursion... 😄

[QuLogic]

In writing the test, I'm wondering if it matters what the key is in the _parents dictionary? It doesn't look like anything is ever removed using it, so that's fine, probably. But after unpickling, I'd think that id(parent) is not necessarily the same as it was before pickling.

[tacaswell]

@pelson @QuLogic What is the state of this?

I don't think we need to block 1.5 on this.

[QuLogic]

I added the test, but like I said, I'm not sure whether the unpickling is as consistent as it could be. It hasn't really lead to any problems though, so if @pelson's okay with it, this PR could be accepted as is.

[tacaswell]

@pelson Can you please review this again?

[tacaswell]

back ported as cba1283