FIX: clean up re-limiting hysteresis #20168
Conversation
|
... ok this has issues if we use the old epoch, but is fine w/ the new epoch. |
0c0c300 to
b0decc5
Compare
|
OK, I think this is fine... |
So to confirm, you mean if |
import matplotlib.pyplot as plt
fig = plt.figure()
ax = fig.add_subplot()
ax.plot([50, 70], [1, 2])
ax.xaxis.axis_date()
assert ax.get_xlim() == (49, 71)
plt.show()Works as expected. New commit adds this as a test. |
|
This is a one-line fix to a pretty old bug.... |
|
I found an explanation in the base class: matplotlib/lib/matplotlib/axis.py Lines 1007 to 1019 in 08f4629 Does this still do the right thing for units that implement default_limits?
|
|
I think that the "correct" solution is likely a bit more complex and likely involves something related to #17106 (comment). |
|
Well, a failing example |
|
Not a particularly meaningful example, but the first I could come up with: from matplotlib.units import AxisInfo, ConversionInterface, registry
from matplotlib import pyplot as plt
import numpy as np
class Temperature: # Just a made up unit system.
def __init__(self, value):
self.value = value
class TemperatureConverter:
axisinfo = staticmethod(lambda *args: AxisInfo(
default_limits=(0, 100), label="temperature"))
default_units = staticmethod(lambda *args: None)
convert = staticmethod(
lambda obj, unit, axis:
obj if ConversionInterface.is_numlike(obj)
else np.array([o.value for o in obj]) if isinstance(obj, np.ndarray)
else obj.value)
registry[Temperature] = TemperatureConverter()
fig, axs = plt.subplots(2)
# Just checking that the interface works.
axs[0].plot([0, 1], [Temperature(0), Temperature(1)])
# An empty axis set to use Temperature units should default to (0, 100).
axs[1].yaxis.update_units(Temperature(0))
plt.show()Before this PR, "empty" temperature axis default to limits of (0, 100); after this PR they don't anymore. To be clear, I think it may be OK to get rid of that per-unit-custom-autolimits feature (well, again, I don't actually use the units system...), but then it should be done everywhere, whereas this PR special-cases dates to be the only units to respect default_units. |
|
Thanks. Well dates need the special case because they are undefined at datenun=0 for the old epoch because we rely on datetime. For the new epoch that is not the case, but we still need to allow the old epoch. I guess a more general solution of limits=[None, None] would be good. Not sure how that tracks though the rest of the code base. |
|
Just to be clear, my point (and I guess @QuLogic's too) is that the current unit system allows any unit to define whatever default_limits they want. With this PR, default_limits gets ignored for anything but dates. If we really want to go that route (which again, may be the right thing to do), we should just deprecate default_limits and add an internal special-casing for dates -- leaving default_limits present but ignored just seems wrong. |
|
No I understand and agree we can't just break this. The logic in this part of the code is pretty convoluted, and I'd be against making it more so just to fix this hysteresis (ie it's not hard to remember to add your hlines after you add your data). But if there is a straightforward fix that simplifies things we should aim for that. |
5d006ac to
673718b
Compare
| if self.converter is not None: | ||
| info = self.converter.axisinfo(self.units, self) | ||
| if info.default_limits is not None: | ||
| valmin, valmax = info.default_limits | ||
| xmin = self.converter.convert(valmin, self.units, self) | ||
| xmax = self.converter.convert(valmax, self.units, self) | ||
| if not dataMutated: |
There was a problem hiding this comment.
Just because we changed the view limits doesn't mean we should change the data limits, which is what this line does...
There was a problem hiding this comment.
I think this is likely the correct fix, thanks for figuring that out! :-)
6f3bae3 to
3164ce1
Compare
|
... OK new commit rips out different 15 year old code. |
lib/matplotlib/axis.py
Outdated
| viewMutated = self.axes.viewLim.mutatedx() | ||
| if not dataMutated or not viewMutated: | ||
| # only change view if dataLim has not changed: | ||
| if not self.axes.dataLim.mutatedx(): |
There was a problem hiding this comment.
Should this rather be if not self.axes.dataLim.mutatedx() and not self.axes.viewLim.mutatedx():? (i.e. if someone explicitly called set_xlim() before, we should also just respect that).
Likewise below.
There was a problem hiding this comment.
Yeah, if I do this, then
munits.registry[Quantity] = quantity_converter
fig, ax1 = plt.subplots()
ax1.xaxis.update_units(Quantity([10], "miles"))
fig.draw_no_output()
assert ax1.get_xlim() == (0, 100)fails. The xlim is set somehow to (-0.06, 0.06), and the viewLim has been mutated. However, I have no idea where that happens or why.
There was a problem hiding this comment.
The view limits are deliberately mutated by _request_autoscale_view in _unit_change_handler().
The issue here seems to be that we would like to have nonsingular limits due to what scale we choose, but we have a parallel nonsingular limit due to units.
@anntzer you were probably the last to work on this. I don't know how to make it all work together cleanly - it seems like the same concept was developed independently in units and scales, and they are clashing.
There was a problem hiding this comment.
I looked a bit into this, and I guess that the correct patch may be
diff --git i/lib/matplotlib/axes/_base.py w/lib/matplotlib/axes/_base.py
index afba7c8321..05317ac140 100644
--- i/lib/matplotlib/axes/_base.py
+++ w/lib/matplotlib/axes/_base.py
@@ -2857,8 +2857,8 @@ class _AxesBase(martist.Artist):
if self.get_yscale() == 'log':
y_stickies = y_stickies[y_stickies > 0]
- def handle_single_axis(scale, autoscaleon, shared_axes, interval,
- minpos, axis, margin, stickies, set_bound):
+ def handle_single_axis(scale, autoscaleon, shared_axes, name,
+ axis, margin, stickies, set_bound):
if not (scale and autoscaleon):
return # nothing to do...
@@ -2870,12 +2870,15 @@ class _AxesBase(martist.Artist):
x_values = []
minimum_minpos = np.inf
for ax in shared:
- x_values.extend(getattr(ax.dataLim, interval))
+ x_values.extend(getattr(ax.dataLim, f"interval{name}"))
minimum_minpos = min(minimum_minpos,
- getattr(ax.dataLim, minpos))
+ getattr(ax.dataLim, f"minpos{name}"))
x_values = np.extract(np.isfinite(x_values), x_values)
if x_values.size >= 1:
x0, x1 = (x_values.min(), x_values.max())
+ elif getattr(self._viewLim, f"mutated{name}")():
+ # No data but explicit viewLims already set: exit.
+ return
else:
x0, x1 = (-np.inf, np.inf)
# If x0 and x1 are non finite, use the locator to figure out
@@ -2919,11 +2922,11 @@ class _AxesBase(martist.Artist):
# End of definition of internal function 'handle_single_axis'.
handle_single_axis(
- scalex, self._autoscaleXon, self._shared_x_axes, 'intervalx',
- 'minposx', self.xaxis, self._xmargin, x_stickies, self.set_xbound)
+ scalex, self._autoscaleXon, self._shared_x_axes, 'x',
+ self.xaxis, self._xmargin, x_stickies, self.set_xbound)
handle_single_axis(
- scaley, self._autoscaleYon, self._shared_y_axes, 'intervaly',
- 'minposy', self.yaxis, self._ymargin, y_stickies, self.set_ybound)
+ scaley, self._autoscaleYon, self._shared_y_axes, 'y',
+ self.yaxis, self._ymargin, y_stickies, self.set_ybound)
def _get_axis_list(self):
return self.xaxis, self.yaxis
diff --git i/lib/matplotlib/axis.py w/lib/matplotlib/axis.py
index 443dee43f7..6f2ed97858 100644
--- i/lib/matplotlib/axis.py
+++ w/lib/matplotlib/axis.py
@@ -1456,8 +1456,7 @@ class Axis(martist.Artist):
default = self.converter.default_units(data, self)
if default is not None and self.units is None:
self.set_units(default)
-
- if neednew:
+ elif neednew:
self._update_axisinfo()
self.stale = True
return True
@@ -2268,7 +2267,7 @@ class XAxis(Axis):
# docstring inherited
xmin, xmax = 0., 1.
# only change view if dataLim has not changed:
- if not self.axes.dataLim.mutatedx():
+ 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 info.default_limits is not None:(and likewise for y)
The problem is first that update_units called both set_units and _update_axisinfo, but set_units itself already calls _update_axisinfo, which thus ended up being called twice, and the second time mutatedx would be True (see (1)). So first get rid of the duplicate call to _update_axisinfo.
The second problem, then, is that _update_axisinfo triggers unstale_viewLim (the first time), which leads to autoscaling being called, which would set the locator-defined (see (2)) default limits instead (causing (1)) (previously, this was not a problem because the second call to _update_axisinfo would undo that). So fix autoscale_view to respect preset viewLims if there's no data and the viewLims have already been mutated (i.e., by the default_limits).
(2): Currently, both units and locators can define default limits (units do so via default_limits; locators do so via nonsingular()); it may make more sense to unify the two approaches and e.g. only support locator.nonsingular() (saying that units who want to define default limits must do so via a custom locator). But that may be for later...
There was a problem hiding this comment.
Right, so as this stands, we lose the user setting the xlim before changing the units on the axes. I actually think this is acceptable, and I don't see a way around it. What does
ax1.set_xlim(0, 10)
ax1.plot(Quantity(np.arange(3), "miles"))even mean?
There was a problem hiding this comment.
ooops, sorry, cross post...
There was a problem hiding this comment.
Yes, the above seems to work with all the tests I've made and
ax1.set_xlim(0, 10)
ax1.plot(Quantity(np.arange(3), "miles"))af857f7 to
c3a1319
Compare
f5662e8 to
5de931c
Compare
Well, actually, my point was that it was more complicated than a one-line change, and I certainly did not understand it enough to approve yet. |
5de931c to
854606a
Compare
854606a to
353ceb4
Compare
|
The OSX failure seems spurious. |
|
Thanks both! |
|
👍 I'm glad this got sorted without removing |
PR Summary
Closes #7742
This was pretty old code, I'm not sure what it was supposed to do, but it was only being triggered if the units changed on the axis
For date axes we need to set the default to something not 0-1 if the old epoch is being used, otherwise we end up w invalid dates so for the empty axes case we call the old set_default_limits.
PR Checklist
pytestpasses).flake8on changed files to check).flake8-docstringsand runflake8 --docstring-convention=all).doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).