From 12f407839234b123a494429e8d51e8320e164d52 Mon Sep 17 00:00:00 2001 From: Kyle Sunden Date: Fri, 11 Oct 2024 13:29:39 -0500 Subject: [PATCH 1/4] Add explicit converter setting to Axis Closes #19229 --- lib/matplotlib/axis.py | 28 +++++++++++++++++++++++++--- lib/matplotlib/tests/test_units.py | 28 +++++++++++++++++++++++++++- 2 files changed, 52 insertions(+), 4 deletions(-) diff --git a/lib/matplotlib/axis.py b/lib/matplotlib/axis.py index 8e612bd8c702..3b5de64bbcec 100644 --- a/lib/matplotlib/axis.py +++ b/lib/matplotlib/axis.py @@ -657,6 +657,7 @@ def __init__(self, axes, *, pickradius=15, clear=True): self.clear() else: self.converter = None + self._explicit_converter = False self.units = None self._autoscale_on = True @@ -887,6 +888,7 @@ def clear(self): self.reset_ticks() self.converter = None + self._explicit_converter = False self.units = None self.stale = True @@ -1741,12 +1743,16 @@ def update_units(self, data): ``axis.converter`` instance if necessary. Return *True* if *data* is registered for unit conversion. """ - converter = munits.registry.get_converter(data) + if not self._explicit_converter: + converter = munits.registry.get_converter(data) + else: + converter = self.converter + if converter is None: return False neednew = self.converter != converter - self.converter = converter + self._set_converter(converter) default = self.converter.default_units(data, self) if default is not None and self.units is None: self.set_units(default) @@ -1799,7 +1805,7 @@ def convert_units(self, x): return x if self.converter is None: - self.converter = munits.registry.get_converter(x) + self._set_converter(munits.registry.get_converter(x)) if self.converter is None: return x @@ -1810,6 +1816,22 @@ def convert_units(self, x): f'units: {x!r}') from e return ret + def set_converter(self, converter): + self._set_converter(converter) + self._explicit_converter = True + + def _set_converter(self, converter): + if self.converter == converter: + return + if self._explicit_converter: + raise RuntimeError("Axis already has an explicit converter set") + elif self.converter is not None: + _api.warn_external( + "This axis already has an converter set and " + "is updating to a potentially incompatible converter" + ) + self.converter = converter + def set_units(self, u): """ Set the units for axis. diff --git a/lib/matplotlib/tests/test_units.py b/lib/matplotlib/tests/test_units.py index ae6372fea1e1..370b5804c485 100644 --- a/lib/matplotlib/tests/test_units.py +++ b/lib/matplotlib/tests/test_units.py @@ -5,7 +5,7 @@ import matplotlib.pyplot as plt from matplotlib.testing.decorators import check_figures_equal, image_comparison import matplotlib.units as munits -from matplotlib.category import UnitData +from matplotlib.category import StrCategoryConverter, UnitData import numpy as np import pytest @@ -236,6 +236,32 @@ def test_shared_axis_categorical(): assert "c" in ax2.xaxis.get_units()._mapping.keys() +def test_explicit_converter(): + d1 = {"a": 1, "b": 2} + str_cat_converter = StrCategoryConverter() + str_cat_converter_2 = StrCategoryConverter() + + # Explicit is set + fig1, ax1 = plt.subplots() + ax1.xaxis.set_converter(str_cat_converter) + assert ax1.xaxis.converter == str_cat_converter + # Explicit not overridden by implicit + ax1.plot(d1.keys(), d1.values()) + assert ax1.xaxis.converter == str_cat_converter + # No error when called twice with equivalent input + ax1.xaxis.set_converter(str_cat_converter) + # Error when explicit called twice + with pytest.raises(RuntimeError): + ax1.xaxis.set_converter(str_cat_converter_2) + + # Warn when implicit overridden + fig2, ax2 = plt.subplots() + ax2.plot(d1.keys(), d1.values()) + + with pytest.warns(): + ax2.xaxis.set_converter(str_cat_converter) + + def test_empty_default_limits(quantity_converter): munits.registry[Quantity] = quantity_converter fig, ax1 = plt.subplots() From 4bb8c8d4b4c40114a271f7c016356083bb7ebe6a Mon Sep 17 00:00:00 2001 From: Kyle Sunden Date: Wed, 23 Oct 2024 14:01:29 -0500 Subject: [PATCH 2/4] Review comments --- lib/matplotlib/axis.py | 27 ++++++++++++++++++++++----- lib/matplotlib/axis.pyi | 3 +++ 2 files changed, 25 insertions(+), 5 deletions(-) diff --git a/lib/matplotlib/axis.py b/lib/matplotlib/axis.py index 3b5de64bbcec..51296a7bbc9e 100644 --- a/lib/matplotlib/axis.py +++ b/lib/matplotlib/axis.py @@ -657,7 +657,7 @@ def __init__(self, axes, *, pickradius=15, clear=True): self.clear() else: self.converter = None - self._explicit_converter = False + self._converter_is_explicit = False self.units = None self._autoscale_on = True @@ -888,7 +888,7 @@ def clear(self): self.reset_ticks() self.converter = None - self._explicit_converter = False + self._converter_is_explicit = False self.units = None self.stale = True @@ -1743,7 +1743,7 @@ def update_units(self, data): ``axis.converter`` instance if necessary. Return *True* if *data* is registered for unit conversion. """ - if not self._explicit_converter: + if not self._converter_is_explicit: converter = munits.registry.get_converter(data) else: converter = self.converter @@ -1816,14 +1816,31 @@ def convert_units(self, x): f'units: {x!r}') from e return ret + def get_converter(self): + """ + Get the unit converter for axis. + + Returns + ------- + `~matplotlib.units.ConversionInterface` or None + """ + return self.converter + def set_converter(self, converter): + """ + Set the unit converter for axis. + + Parameters + ---------- + converter : `~matplotlib.dates.ConversionInterface` + """ self._set_converter(converter) - self._explicit_converter = True + self._converter_is_explicit = True def _set_converter(self, converter): if self.converter == converter: return - if self._explicit_converter: + if self._converter_is_explicit: raise RuntimeError("Axis already has an explicit converter set") elif self.converter is not None: _api.warn_external( diff --git a/lib/matplotlib/axis.pyi b/lib/matplotlib/axis.pyi index 8f7b213c51e3..f2c5b1fc586d 100644 --- a/lib/matplotlib/axis.pyi +++ b/lib/matplotlib/axis.pyi @@ -15,6 +15,7 @@ from matplotlib.text import Text from matplotlib.ticker import Locator, Formatter from matplotlib.transforms import Transform, Bbox from matplotlib.typing import ColorType +from matplotlib.units import ConversionInterface GRIDLINE_INTERPOLATION_STEPS: int @@ -207,6 +208,8 @@ class Axis(martist.Artist): def update_units(self, data): ... def have_units(self) -> bool: ... def convert_units(self, x): ... + def get_converter(self) -> ConversionInterface | None: ... + def set_converter(self, converter: ConversionInterface) -> None: ... def set_units(self, u) -> None: ... def get_units(self): ... def set_label_text( From 493fbc6ce5bb725eceae4a308713be09b0bfce74 Mon Sep 17 00:00:00 2001 From: Kyle Sunden Date: Thu, 31 Oct 2024 01:00:49 -0500 Subject: [PATCH 3/4] Privatize axis.converter attribute The replacement is the get/set_converter method. This includes changes to use the getter and the private setter in the qt figure options dialog menu. The choice to use the private setter was a defensive one as the public setter prevents being called multiple times (though does short circuit if an identical input is provided, which I think is actually true here, therefore the public one is probably functional (and a no-op).) It is not clear to me on analysis how the unit information is or was lost. A quick test commenting out the two lines which reset converter/units displayed no obvious detrimental effect to removing those, suggesting that even if once they were necessary, they may no longer be. These lines were last touched in #24141, though that really only generalized the code into a loop rather than copy/pasted x and y behavior. The original inclusion of resetting was in #4909, which indicated that the dialog reset unit info. AFAICT, that is no longer true, though I have not rigorously proved that. --- lib/matplotlib/axis.py | 46 ++++++++++--------- .../backends/qt_editor/figureoptions.py | 6 +-- lib/matplotlib/tests/test_dates.py | 6 ++- lib/matplotlib/tests/test_units.py | 4 +- 4 files changed, 34 insertions(+), 28 deletions(-) diff --git a/lib/matplotlib/axis.py b/lib/matplotlib/axis.py index 51296a7bbc9e..aa202b71b366 100644 --- a/lib/matplotlib/axis.py +++ b/lib/matplotlib/axis.py @@ -600,6 +600,10 @@ class Axis(martist.Artist): # The class used in _get_tick() to create tick instances. Must either be # overwritten in subclasses, or subclasses must reimplement _get_tick(). _tick_class = None + converter = _api.deprecate_privatize_attribute( + "3.10", + alternative="get_converter and set_converter methods" + ) def __str__(self): return "{}({},{})".format( @@ -656,7 +660,7 @@ def __init__(self, axes, *, pickradius=15, clear=True): if clear: self.clear() else: - self.converter = None + self._converter = None self._converter_is_explicit = False self.units = None @@ -887,7 +891,7 @@ def clear(self): mpl.rcParams['axes.grid.which'] in ('both', 'minor')) self.reset_ticks() - self.converter = None + self._converter = None self._converter_is_explicit = False self.units = None self.stale = True @@ -1740,20 +1744,20 @@ def grid(self, visible=None, which='major', **kwargs): def update_units(self, data): """ Introspect *data* for units converter and update the - ``axis.converter`` instance if necessary. Return *True* + ``axis.get_converter`` instance if necessary. Return *True* if *data* is registered for unit conversion. """ if not self._converter_is_explicit: converter = munits.registry.get_converter(data) else: - converter = self.converter + converter = self._converter if converter is None: return False - neednew = self.converter != converter + neednew = self._converter != converter self._set_converter(converter) - default = self.converter.default_units(data, self) + default = self._converter.default_units(data, self) if default is not None and self.units is None: self.set_units(default) @@ -1767,10 +1771,10 @@ def _update_axisinfo(self): Check the axis converter for the stored units to see if the axis info needs to be updated. """ - if self.converter is None: + if self._converter is None: return - info = self.converter.axisinfo(self.units, self) + info = self._converter.axisinfo(self.units, self) if info is None: return @@ -1797,20 +1801,20 @@ def _update_axisinfo(self): self.set_default_intervals() def have_units(self): - return self.converter is not None or self.units is not None + return self._converter is not None or self.units is not None def convert_units(self, x): # If x is natively supported by Matplotlib, doesn't need converting if munits._is_natively_supported(x): return x - if self.converter is None: + if self._converter is None: self._set_converter(munits.registry.get_converter(x)) - if self.converter is None: + if self._converter is None: return x try: - ret = self.converter.convert(x, self.units, self) + ret = self._converter.convert(x, self.units, self) except Exception as e: raise munits.ConversionError('Failed to convert value(s) to axis ' f'units: {x!r}') from e @@ -1824,7 +1828,7 @@ def get_converter(self): ------- `~matplotlib.units.ConversionInterface` or None """ - return self.converter + return self._converter def set_converter(self, converter): """ @@ -1838,16 +1842,16 @@ def set_converter(self, converter): self._converter_is_explicit = True def _set_converter(self, converter): - if self.converter == converter: + if self._converter == converter: return if self._converter_is_explicit: raise RuntimeError("Axis already has an explicit converter set") - elif self.converter is not None: + elif self._converter is not None: _api.warn_external( - "This axis already has an converter set and " + "This axis already has a converter set and " "is updating to a potentially incompatible converter" ) - self.converter = converter + self._converter = converter def set_units(self, u): """ @@ -2568,8 +2572,8 @@ def set_default_intervals(self): # not changed the view: if (not self.axes.dataLim.mutatedx() and not self.axes.viewLim.mutatedx()): - if self.converter is not None: - info = self.converter.axisinfo(self.units, self) + if self._converter is not None: + info = self._converter.axisinfo(self.units, self) if info.default_limits is not None: xmin, xmax = self.convert_units(info.default_limits) self.axes.viewLim.intervalx = xmin, xmax @@ -2798,8 +2802,8 @@ def set_default_intervals(self): # not changed the view: if (not self.axes.dataLim.mutatedy() and not self.axes.viewLim.mutatedy()): - if self.converter is not None: - info = self.converter.axisinfo(self.units, self) + if self._converter is not None: + info = self._converter.axisinfo(self.units, self) if info.default_limits is not None: ymin, ymax = self.convert_units(info.default_limits) self.axes.viewLim.intervaly = ymin, ymax diff --git a/lib/matplotlib/backends/qt_editor/figureoptions.py b/lib/matplotlib/backends/qt_editor/figureoptions.py index b025ef3e056e..2809e63c4e8c 100644 --- a/lib/matplotlib/backends/qt_editor/figureoptions.py +++ b/lib/matplotlib/backends/qt_editor/figureoptions.py @@ -42,7 +42,7 @@ def convert_limits(lim, converter): axis_map = axes._axis_map axis_limits = { name: tuple(convert_limits( - getattr(axes, f'get_{name}lim')(), axis.converter + getattr(axes, f'get_{name}lim')(), axis.get_converter() )) for name, axis in axis_map.items() } @@ -66,7 +66,7 @@ def convert_limits(lim, converter): # Save the converter and unit data axis_converter = { - name: axis.converter + name: axis.get_converter() for name, axis in axis_map.items() } axis_units = { @@ -209,7 +209,7 @@ def apply_callback(data): axis.set_label_text(axis_label) # Restore the unit data - axis.converter = axis_converter[name] + axis._set_converter(axis_converter[name]) axis.set_units(axis_units[name]) # Set / Curves diff --git a/lib/matplotlib/tests/test_dates.py b/lib/matplotlib/tests/test_dates.py index 2d60e3525b2a..73f10cec52aa 100644 --- a/lib/matplotlib/tests/test_dates.py +++ b/lib/matplotlib/tests/test_dates.py @@ -668,10 +668,12 @@ def test_concise_converter_stays(): fig, ax = plt.subplots() ax.plot(x, y) # Bypass Switchable date converter - ax.xaxis.converter = conv = mdates.ConciseDateConverter() + conv = mdates.ConciseDateConverter() + with pytest.warns(UserWarning, match="already has a converter"): + ax.xaxis.set_converter(conv) assert ax.xaxis.units is None ax.set_xlim(*x) - assert ax.xaxis.converter == conv + assert ax.xaxis.get_converter() == conv def test_offset_changes(): diff --git a/lib/matplotlib/tests/test_units.py b/lib/matplotlib/tests/test_units.py index 370b5804c485..a8735e180bf0 100644 --- a/lib/matplotlib/tests/test_units.py +++ b/lib/matplotlib/tests/test_units.py @@ -244,10 +244,10 @@ def test_explicit_converter(): # Explicit is set fig1, ax1 = plt.subplots() ax1.xaxis.set_converter(str_cat_converter) - assert ax1.xaxis.converter == str_cat_converter + assert ax1.xaxis.get_converter() == str_cat_converter # Explicit not overridden by implicit ax1.plot(d1.keys(), d1.values()) - assert ax1.xaxis.converter == str_cat_converter + assert ax1.xaxis.get_converter() == str_cat_converter # No error when called twice with equivalent input ax1.xaxis.set_converter(str_cat_converter) # Error when explicit called twice From 6f97b1c37628fff59eb895a83100bcd61bfac596 Mon Sep 17 00:00:00 2001 From: Kyle Sunden Date: Thu, 31 Oct 2024 13:11:29 -0500 Subject: [PATCH 4/4] [doc] fix references to new API --- doc/api/axis_api.rst | 2 ++ lib/matplotlib/axis.py | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/doc/api/axis_api.rst b/doc/api/axis_api.rst index 0870d38c9439..85ba990ffece 100644 --- a/doc/api/axis_api.rst +++ b/doc/api/axis_api.rst @@ -169,6 +169,8 @@ Units Axis.convert_units Axis.set_units Axis.get_units + Axis.set_converter + Axis.get_converter Axis.update_units diff --git a/lib/matplotlib/axis.py b/lib/matplotlib/axis.py index aa202b71b366..0c095e9c4e0a 100644 --- a/lib/matplotlib/axis.py +++ b/lib/matplotlib/axis.py @@ -1836,7 +1836,7 @@ def set_converter(self, converter): Parameters ---------- - converter : `~matplotlib.dates.ConversionInterface` + converter : `~matplotlib.units.ConversionInterface` """ self._set_converter(converter) self._converter_is_explicit = True