| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
I was hoping I could modify the figure and show again, but that does not seem to be the case import matplotlib.pyplot as plt
from matplotlib.figure import Figure
fig = Figure()
ax = fig.subplots()
ax.plot([0, 2])
plt.figure(fig)
plt.show()
ax.set_title('A cool line')
plt.figure(fig)
plt.show()No title is shown 😕 |
Sorry, something went wrong.
It may be fundamentally nice not to have to create the figure though pyplot to be able to use it in pyplot afterwards. You can now do ``` from matplotlib.figure import Figure import matplotlib.pyplot as plt fig = Figure() fig.subplots().plot([1, 3, 2]) plt.figure(fig) # fig is now tracked in pyplot plt.show() ``` This also opens up the possibility to more dynamically track and untrack figures in pyplot, which opens up the road to optimized figure tracking in pyplot (matplotlib#29849)
|
Showing again now works properly. When destroying a figure manager, the figure's canvas is reset to a FigureCanvasBase. Technical questions:
|
Sorry, something went wrong.
|
Note: the failing tests are in test_interactive_backend, which saves the figure to file, closes it, and then saves it to file again, expecting exactly the same output. matplotlib/lib/matplotlib/tests/test_backends_interactive.py Lines 222 to 235 in 011d12f This is now no longer the case, as closing the figure resets the canvas to FigureCanvasBase, i.e. the images are genereated through different canvases - FigureCanvasBase and the backend-specific canvas, which do not necessarily have pixel-identical output. An example diff looks like this:
How should we handle this? I'm inclined to say that the previous expectation is no longer justified, and it makes sense that the canvas is reset. Should we try to fix this with tolerances? Or don't we need this image test at all anymore and instead it is sufficient to test that the figure has again a FigureCanvasBase (which imlies it can be saved with savefig, but we don't have to test the contents? Or are there other approaches? |
Sorry, something went wrong.
|
It's a bit strange that the end png result is not the same, as both should ultimately go through Agg for rasterizing... It would be nice to figure out what is going wrong, but I agree this isn't a blocker. |
Sorry, something went wrong.
|
Failing tests are FAILED lib/matplotlib/tests/test_backends_interactive.py::test_interactive_backend[toolbar2-MPLBACKEND=qtagg-QT_API=PyQt5-BACKEND_DEPS=PyQt5] FAILED lib/matplotlib/tests/test_backends_interactive.py::test_interactive_backend[toolbar2-MPLBACKEND=qtcairo-QT_API=PyQt6-BACKEND_DEPS=PyQt6,cairocffi] FAILED lib/matplotlib/tests/test_backends_interactive.py::test_interactive_backend[toolbar2-MPLBACKEND=qtcairo-QT_API=PySide6-BACKEND_DEPS=PySide6,cairocffi] FAILED lib/matplotlib/tests/test_backends_interactive.py::test_interactive_backend[toolbar2-MPLBACKEND=qtcairo-QT_API=PyQt5-BACKEND_DEPS=PyQt5,cairocffi] FAILED lib/matplotlib/tests/test_backends_interactive.py::test_interactive_backend[toolbar2-MPLBACKEND=qtcairo-QT_API=PySide2-BACKEND_DEPS=PySide2,cairocffi] and the same again for toolmanager, which I left out for simplicity. So all cairocffi tests fail and addtionally the qtagg-PyQt5 test. But the agg-based tests for PyQt6, PySide2 and PySide6 pass. Interestingly, the qtagg-PyQt5 test passes on my local machine. The above diff image was from qtcairo-PyQt5. |
Sorry, something went wrong.
|
At least for the cairo tests this is understandable: if you reset to FigureCanvasBase, then they will now rasterize using agg, whereas they rasterized with cairo prior to that. I guess that's probably fine? |
Sorry, something went wrong.
I prefer adding doing this via super() calls: for the benefit of third-parties, it is relatively easier to document "one needs to call super().destroy() in subclasses" rather than "one needs to do this and that" (restore the default canvas, but who knows what else can change in the future). |
Sorry, something went wrong.
|
My thought was that the specific FigureManager* classes override the canvas in their __init__ and hence they should undo what they did in their destroy(). But one could also argue that resetting to the base canvas is a universal property of destroying any manager. |
Sorry, something went wrong.
When destroying a manager, replace the figure's canvas by a figure canvas base.
There in now machinery for all of the public API that takes a renderer is input to get it from the current canvas on the root figure. Use that machinery instead.
|
I took the liberty of fixing one of the tests. We probably should purge all of the canvas.get_renderer() calls from the tests, but that is a later problem. |
Sorry, something went wrong.
import must be local
| { | ||
| [self->window close]; | ||
| self->window = NULL; | ||
| [super destroy]; |
There was a problem hiding this comment.
I could need a bit of help here from someone with more Objective-C / Cpython knowledge. This is missing the equivalent of super.destroy(). The change here was a rather wild guess from pattern-matching other similar cases in the module, but is apparently not correct, because this now fails the build with
../../src/_macosx.m:689:6: error: use of undeclared identifier 'super'
Sorry, something went wrong.
There was a problem hiding this comment.
Do you need to go into the objective C layer or can you stay at the Python layer (have not looked at the updates here in detail). But, we do have some multiple inheritance going on here, so you may have to explicitly call which super you want to interact with at this place. (I think the super().destroy() currently on these lines goes to the objective C implementation, and you want to go the FigureManagerBase implementation [we want to travel to both it sounds like])
matplotlib/lib/matplotlib/backends/backend_macosx.py
Lines 168 to 174 in 352b419
Sorry, something went wrong.
There was a problem hiding this comment.
super() goes to the next class in MRO, and that covers the whole inheritance tree also in case of multiple inheritance if all classes in the hierarchy call it without explicitly stating the parent. That‘s what I want. I do not want to mess with calling any specific parent.
Sorry, something went wrong.
There was a problem hiding this comment.
My quick suggestion would be to avoid trying to call super() from the C-layer. To avoid the multiple inheritance issue, you can rename the (PyCFunction)FigureManager_destroy to (PyCFunction)FigureManager__destroy or something else (its in a private module) and then call that method explicitly from the Python destroy() side. You can keep the super().destroy() already there which will still travel the tree as you want I think.
Sorry, something went wrong.
There was a problem hiding this comment.
Maybe something along the lines of https://stackoverflow.com/questions/42201881/python-c-api-how-to-call-base-type-method-from-subtype would work?
Sorry, something went wrong.
| // call super().destroy() | ||
| PyObject *super_obj = PyObject_CallFunctionObjArgs( | ||
| (PyObject *)&PySuper_Type, | ||
| (PyObject *)&FigureManagerType, | ||
| self, | ||
| NULL | ||
| ); | ||
| if (super_obj == NULL) { | ||
| return NULL; // error | ||
| } | ||
| PyObject *result = PyObject_CallMethod(super_obj, "destroy", NULL); | ||
| Py_DECREF(super_obj); | ||
| if (result == NULL) { | ||
| return NULL; // error | ||
| } | ||
| Py_DECREF(result); |
There was a problem hiding this comment.
There are two approvals already, but since this super() call is new and I'm not the most experienced C-API developer, I'd like someone to cross check this bit again.
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM, but maybe someone on macOS should double-check that part.
Sorry, something went wrong.
|
I spot-checked this on OSX and it seems work as expected:
|
Sorry, something went wrong.
|
Thank you @timhoffm ! I'm super stoked we landed this as this has been a low-key goal of mine for a while now. I'm also very happy that the actual implementation was way simpler and less invasive than the direction I was going. |
Sorry, something went wrong.
|
🎉😀🍾🥂 |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
It may be fundamentally nice not to have to create the figure though pyplot to be able to use it in pyplot afterwards. You can now do
This also opens up the possibility to more dynamically track and untrack figures in pyplot, which opens up the road to optimized figure tracking in pyplot (#29849)
Anybody, feel free to play around with this and try to break it.