-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Attach a FigureCanvasBase by default to Figures. #12450
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
First thought: Does this involve any costly setup logic? We are already quite slow in creating figures. Second thought: Is
just for semantic reasons? Third thought: Do we now get more strange behavior or error message when we try things that would before fail due to the missing canvas? |
|
No just looked at the code 😄
Doesn't sound well thought through. I haven't either, but I'm hesitant to introduce a code change with an effect that we do not overlook. |
I actually really think it's not going to cause any problems, but as usual things are tied up in so many knots in mpl that it's hard to know if we don't try. |
1e6b5e5
to
b2a1b5d
Compare
"rotation disabled. Set canvas then call " | ||
"Axes3D.mouse_init().") | ||
|
||
self._cids = [ |
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.
Concerned that if we are hitting this code path with the base canvas and then it gets replaced the subscriptions will not work.
On the other hand, you are no worse than you are now.
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.
So I'll leave it as it is until someone complains :)
This code was last touched in b0807ae which was primarily reverting #1125 The original I suspect looking at why #1125 failed would be a good guide to if we are missing something here. From a very quick look, that PR defaulted to getting the current backend from pyplot (more-or-less) where as this is is going with the base class which has just enough logic to fall-back on save. Another option is to make |
This is particularly useful for headless cases, where one just wants to take advantage of `print_figure`, which is defined on the base class. For example, one can now do for <loop>: figure = Figure() # do some plotting figure.savefig(...) and let `figure` get gc'd at the next loop iteration; a pyplot-based solution would instead have to explicitly close() the figure or reuse it after clf()'ing it. Also simplifies various places in the codebase where we had to handle the case of `.canvas = None` previously.
b2a1b5d
to
380c531
Compare
Looking at #1457 (comment) it looks like #1125 failed because not all Canvas classes have the same constructor signature, but that's not a problem here. In both cases I think using FigureCanvasBase avoids these problems. |
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.
This seems reasonable to me, if for no other reason than the number of checks for None
it removes.
In 380c531 / matplotlib#12450 we set the default canvas on Figure `__init__` to FigureCanvasBase but were still setting it to `None` in `__setstate__`.
In 380c531 / matplotlib#12450 we set the default canvas on Figure `__init__` to FigureCanvasBase but were still setting it to `None` in `__setstate__`.
This is particularly useful for headless cases, where one just wants to
take advantage of
print_figure
, which is defined on the base class.For example, one can now do
and let
figure
get gc'd at the next loop iteration; a pyplot-basedsolution would instead have to explicitly close() the figure or reuse it
after clf()'ing it.
Also simplifies various places in the codebase where we had to handle
the case of
.canvas = None
previously.xref #11657, #12447.
PR Summary
PR Checklist