Skip to content

Fix interaction with unpickled 3d plots. #16220

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

Merged
merged 2 commits into from
Dec 15, 2020
Merged

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Jan 14, 2020

PR Summary

In order to reset the mouse interaction callbacks on unpickled 3d plots,
the callback registry really needs to be on the Figure object rather
than the Canvas, because the canvas doesn't exist yet when the 3d axes
is being unpickled (it is only set on the figure at the very end of
unpickling).

So move the callback registry to the figure (with a proxy property on
the canvas).

Then, add a private mechanism to pickle select callbacks, and combine
everything together. (Bound methods of picklable objects are picklable.)

Test with e.g.

import matplotlib.pyplot as plt
import pickle

fig = plt.figure()
fig.add_subplot(111, projection='3d')

p = pickle.dumps(fig)
plt.close("all")

pickle.loads(p)

plt.show()

Closes #10843.

Goes on top of #15855. Would also be nice to get #16219 in first and rebase on top of it, but not compulsory.

The same mechanism would likely be usable e.g. for the "units finalize" callbacks (grep for this in axes/_base.py) which are likewise dropped on pickling/unpickling, but probably no one ever tried doing funky things with unit changes and pickling so no one noticed...

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@tacaswell
Copy link
Member

I like the idea, but I have two concerns:

  • if the callback hang off of the Figure object, then if the canvas puts any callback on itself and then the canvas is changed the callbacks will continue to fire on the no-longer attached callback
  • I would rather see a _try_to_pickle kwarg on subscribe rather than having a private set we reach in and update.

@anntzer
Copy link
Contributor Author

anntzer commented Mar 10, 2020

if the callback hang off of the Figure object, then if the canvas puts any callback on itself and then the canvas is changed the callbacks will continue to fire on the no-longer attached callback

I actually think that's saner behavior (do you have actual examples of wanting to lose the callbacks when swapping canvases?).

I would rather see a _try_to_pickle kwarg on subscribe rather than having a private set we reach in and update.

I agree the API is not so nice, but _try_to_pickle would effectively become (semi)public API or at least need to be exposed both in connect() and mpl_connect(). I don't feel strongly about this, though. (And it should be do_pickle, not try_to_pickle, i.e. we should just error out when pickling fails.)

@tacaswell tacaswell modified the milestones: v3.3.0, v3.4.0 May 25, 2020
@tacaswell
Copy link
Member

Moved to 3.4 as this needs a bit more thought.

@anntzer
Copy link
Contributor Author

anntzer commented Sep 15, 2020

See also https://discourse.matplotlib.org/t/mpl-iteractions-with-gui-outside-of-jupyter-notebooks/21523/6 for a case where attaching callbacks to the figure rather than the canvas makes more sense(?)

@tacaswell
Copy link
Member

On more consideration, I am convinced this is the right course.

I suspect we want to make a much bigger deal of this, re-write the docs and start pushing people towards directly registering callbacks on the figures rather than silently changing this under the hood.

@anntzer
Copy link
Contributor Author

anntzer commented Sep 15, 2020

That's fine with me, but do you want to take care of the docs rewrite? ;)

In order to reset the mouse interaction callbacks on unpickled 3d plots,
the callback registry really needs to be on the Figure object rather
than the Canvas, because the canvas doesn't exist yet when the 3d axes
is being unpickled (it is only set on the figure at the very end of
unpickling).

So move the callback registry to the figure (with a proxy property on
the canvas).

Then, add a private mechanism to pickle select callbacks, and combine
everything together.

Test with e.g.
```
import matplotlib.pyplot as plt
import pickle

fig = plt.figure()
fig.add_subplot(111, projection='3d')

p = pickle.dumps(fig)
plt.close("all")

pickle.loads(p)

plt.show()
```
@anntzer
Copy link
Contributor Author

anntzer commented Oct 28, 2020

I think this (the callbacks pickling part) would be nice to have for #16931 (comment) as well.
@tacaswell do you want to actually "expose" the API change (i.e. the fact that callbacks moved to the figure)? It should be fairly transparent to the end user whether they're on the figure or the canvas...

@tacaswell
Copy link
Member

I do have a commit in this PR, but it is the deprecation note.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to interact with pickled 3D plot
3 participants