Use matplotlib backend registry when possible#5141
Use matplotlib backend registry when possible#5141neutrinoceros merged 2 commits intoyt-project:mainfrom
Conversation
chrishavlin
added
enhancement
|
|
||
| module, fig_canvas, fig_manager = BACKEND_SPECS[key] | ||
| if matplotlib.__version_info__ >= (3, 9): | ||
| # once yt has a minimum matplotlib version of 3.9, this branch |
There was a problem hiding this comment.
I would not bother with this comment; to me, it's 100% implied by the code itself. But, NBD if you think it's helpful.
There was a problem hiding this comment.
mainly added it cause BACKEND_SPECS is not defined within the scope of the function. so i'd rather keep it i think? but also don't feel strongly about it.
There was a problem hiding this comment.
Oh yeah, when you put it like that, I agree with leaving it.
| reason="This test requires matplotlib 3.9 or higher.", | ||
| ) | ||
| @pytest.mark.parametrize("backend_key", BACKEND_SPECS.keys()) | ||
| def test_backend_specs(backend_key): |
There was a problem hiding this comment.
Seems like this test lead to the discovery of invalid entries, so it's great. Do you think it's possible to also test that the dictionary is complete ?
There was a problem hiding this comment.
not in a satisfyingly automated way... you can get a list of all backends with:
>>> from matplotlib.backends import backend_registry
>>> backend_registry.list_builtin()
['gtk3agg', 'gtk3cairo', 'gtk4agg', 'gtk4cairo', 'macosx', 'nbagg', 'notebook', 'qtagg', 'qtcairo', 'qt5agg', 'qt5cairo', 'tkagg', 'tkcairo', 'webagg', 'wx', 'wxagg', 'wxcairo', 'agg', 'cairo', 'pdf', 'pgf', 'ps', 'svg', 'template']
but i'm not sure we want the non-interactive backends in BACKEND_SPECS (pdf, svg, etc.). And since the test could only be run with matplotlib 3.9+, it also would not be very future proof. If a new backend shows up in a new matplotlib release, it would cause the completeness test to fail but the new backend would not actually work for the case where BACKEND_SPEC actually gets used. So we'd be back to manually maintaining BACKEND_SPEC until yt bumps to matplotlib >= 3.9. So I'd prefer to leave BACKEND_SPEC alone at this point and encourage folks to upgrade matplotlib instead -- I could add a new try/except to the BACKEND_SPEC branch that suggests upgrading matplotlib if you get a key error from BACKEND_SPEC ?
There was a problem hiding this comment.
Thanks for the additional info. Yes, such a try/except could improve UX, but I now think the PR is good as is, so, up to you !
There was a problem hiding this comment.
ok, i'll leave it off then. given the lack of bug reports and questions i don't think it's a huge priority.
neutrinoceros
removed
the
deprecation
| figure_canvas, figure_manager = self._get_canvas_classes() | ||
| self.canvas = figure_canvas(self.figure) | ||
| if figure_manager is not None: | ||
| # with matplotlib >= 3.9, figure_manager should always be not None |
There was a problem hiding this comment.
This could be expressed directly within the condition: we can only test for None on older matplotlib. This approach would be more greppable.
There was a problem hiding this comment.
ya, I thought about that but we'd still have to set self.manager for newer matplotlib so I thought the if statement became less obvious than the comment. i.e., would need this to be:
if matplotlib.__version_info__ >= (3, 9) or figure_manager is not None:
self.manager = figure_manager(self.canvas, 1)but I wasn't thinking about greppability. which is a good point, so happy to put the above change in and drop the comment.
2 participants
Closes #5138
I also added a test for the existing
BACKEND_SPECS, just checking that the backend keys are valid, which resulted in me realizing that gtk, gtkagg and gtkcairo have all been deprecated for a while (ipython/ipython#14368 , matplotlib/matplotlib#10426) in favor of versioned keys (e.g., GTK3Agg). I removed the deprecated keys.Also worth noting that with this change, for folks using matplotlib>=3.9, yt plotting should now work with all the interactive backends listed at https://matplotlib.org/stable/users/explain/figure/backends.html#interactive-backends , which includes some that were missing from
BACKEND_SPECS, e.g.:GTK4AggandGTK4Cairo.