Skip to content

Use matplotlib backend registry when possible #5141

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.

Sign up for GitHub

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
neutrinoceros merged 2 commits into from
Mar 21, 2025
Merged

Use matplotlib backend registry when possible #5141

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bebe8ed
use matplotlib.backends.backend_registry when possible
chrishavlin Mar 19, 2025
eb5e396
add test for BACKEND_SPECS, remove deprecated backend keys
chrishavlin Mar 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions nose_ignores.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
--ignore-file=test_add_field\.py
--ignore-file=test_ambiguous_fields\.py
--ignore-file=test_base_plot_types\.py
--ignore-file=test_callable_grids\.py
--ignore-file=test_callbacks_geographic\.py
--ignore-file=test_commons\.py
Expand Down
1 change: 1 addition & 0 deletions tests/tests.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,7 @@ other_tests:
- "--ignore=test_outputs_pytest"
- "--ignore-file=test_add_field\\.py"
- "--ignore-file=test_ambiguous_fields\\.py"
- "--ignore-file=test_base_plot_types\\.py"
- "--ignore-file=test_callable_grids\\.py"
- "--ignore-file=test_callbacks_geographic\\.py"
- "--ignore-file=test_commons\\.py"
Expand Down
18 changes: 13 additions & 5 deletions yt/visualization/base_plot_types.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,6 @@ class FormatKwargs(TypedDict):


BACKEND_SPECS = {
"gtk": ["backend_gtk", "FigureCanvasGTK", "FigureManagerGTK"],
"gtkagg": ["backend_gtkagg", "FigureCanvasGTKAgg", None],
"gtkcairo": ["backend_gtkcairo", "FigureCanvasGTKCairo", None],
"macosx": ["backend_macosx", "FigureCanvasMac", "FigureManagerMac"],
"qt5agg": ["backend_qt5agg", "FigureCanvasQTAgg", None],
"qtagg": ["backend_qtagg", "FigureCanvasQTAgg", None],
Expand Down Expand Up @@ -138,6 +135,8 @@ def __init__(
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
Copy link
Member

@neutrinoceros neutrinoceros Mar 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be expressed directly within the condition: we can only test for None on older matplotlib. This approach would be more greppable.

Copy link
Contributor Author

@chrishavlin chrishavlin Mar 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

# see _get_canvas_classes for details.
self.manager = figure_manager(self.canvas, 1)

self.axes.tick_params(
Expand All @@ -151,11 +150,20 @@ def _create_axes(self, axrect: tuple[float, float, float, float]) -> None:

def _get_canvas_classes(self):
if self.interactivity:
key = str(matplotlib.get_backend()).lower()
key = str(matplotlib.get_backend())
else:
key = "agg"

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
Copy link
Member

@neutrinoceros neutrinoceros Mar 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@chrishavlin chrishavlin Mar 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@neutrinoceros neutrinoceros Mar 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah, when you put it like that, I agree with leaving it.

# can replace the rest of this function and BACKEND_SPECS can
# be removed. See https://github.com/yt-project/yt/issues/5138
from matplotlib.backends import backend_registry

mod = backend_registry.load_backend_module(key)
return mod.FigureCanvas, mod.FigureManager

module, fig_canvas, fig_manager = BACKEND_SPECS[key.lower()]

mod = __import__(
"matplotlib.backends",
Expand Down
19 changes: 19 additions & 0 deletions yt/visualization/tests/test_base_plot_types.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import matplotlib
import pytest

from yt.visualization.base_plot_types import BACKEND_SPECS


@pytest.mark.skipif(
matplotlib.__version_info__ < (3, 9),
reason="This test requires matplotlib 3.9 or higher.",
)
@pytest.mark.parametrize("backend_key", BACKEND_SPECS.keys())
def test_backend_specs(backend_key):
Copy link
Member

@neutrinoceros neutrinoceros Mar 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Contributor Author

@chrishavlin chrishavlin Mar 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Member

@neutrinoceros neutrinoceros Mar 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 !

Copy link
Contributor Author

@chrishavlin chrishavlin Mar 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, i'll leave it off then. given the lack of bug reports and questions i don't think it's a huge priority.

# note: while this test itself requires matplotlib 3.9, it is
# testing functionality in yt that can be removed once yt has
# a minimal matplotlib version of 3.9,
# see https://github.com/yt-project/yt/issues/5138
from matplotlib.backends import backend_registry

assert backend_registry.is_valid_backend(backend_key)
Loading