-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
MNT: set default canvas when un-pickling #16189
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
Conversation
@@ -1999,7 +1999,7 @@ def __setstate__(self, state): | |||
|
|||
# re-initialise some of the unstored state information | |||
self._axobservers = [] | |||
self.canvas = None | |||
FigureCanvasBase(self) # Set self.canvas. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it would be vaguely nicer to set state["canvas"] (and also state["_axobservers"], state["_layoutbox"]) to the right values in __getstate__
rather than popping them out of the dict and having to restore them here, but I'm not going to insist on that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The types in these values tend to not be things that pickle well. canvas
can be a Qt object and the observers can be lambdas (and yes, we could use cloud pickle and go down the route that dask does to pickle/unpickle almost anything, but that is probably not worth the complexity or something we want to encourage).
The other nice thing about not directly pickling the canvas objects is that when re-hydrated they will come up in the correct framework of the importer. This fix only matters if you are unpickling a Figure
that was unknown to pyplot
when it was pickled (and inline
de-registers the figures after they show) then this code path will matter.
From a philosophical I think that the canvas object is not part of the identity of the Figure
. A given Figure
can only we associated with one canvas at a time (due to the circular references) but we can (and do!) switch the canvas out "live", hence even if we could, it probably should not be included verbatim in the pickle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I know you can't pickle Qt canvasses (and agree they're not really part of figure state); I meant to pickle a FigureCanvasBase instead (I guess you only need to do so if restore_to_pylab is false). Not a really big deal, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor things that should be fixed, but the idea looks right.
In 380c531 / matplotlib#12450 we set the default canvas on Figure `__init__` to FigureCanvasBase but were still setting it to `None` in `__setstate__`.
6c64ea7
to
d20ea49
Compare
…189-on-v3.2.x Backport PR #16189 on branch v3.2.x (MNT: set default canvas when un-pickling)
In 380c531 / #12450 we set the
default canvas on Figure
__init__
to FigureCanvasBase but were stillsetting it to
None
in__setstate__
.Found this while investigating #8291. After inline displays itself it removes itself from the list of active figures according to pyplot so on un-pickling we don't set the canvas. This causes the figure to fail to display (due to
fig.canvas
beingNone
).