Synchronize units change in Axis.set_units for shared axis

[l-johnston]

PR Summary

Closes #10304
Closes #18311

PR Checklist

• Has Pytest style unit tests
• Code is Flake 8 compliant
• New features are documented, with examples if plot related
• Documentation is sphinx and numpydoc compliant

[timhoffm]

Thanks for looking into this!

I'm not particularly fond of replicating all Axis.set_foo methods to Axes.set_x/yfoo. I favor the paradigm ax.xaxis.set_foo() over ax.set_xfoo for most but the simplest cases (e.g. label).

Having more functionality via emit might be seen as a justification, but conversely I argue that functionality should not whether I set it via the Axes or via the Axis. emit in set_x/ylim is basically a crude hack for synchronizing the Axises without running into recursive updates.

The proper solution would be a concept in which an Axis can communicate changes to it's shared siblings. Making the higher level Axes responsible for synchronization is fragile. Of course, that's a much harder task.

From the API and logic side I'm -0.9 on this. But I acknowledge that we likely won't get the proper solution any time soon. So this might be bearable as a workaround to get the desired functionality.

[anntzer]

Would something along the lines of

diff --git i/lib/matplotlib/axis.py w/lib/matplotlib/axis.py
index 6d28fae6c..d889785c5 100644
--- i/lib/matplotlib/axis.py
+++ w/lib/matplotlib/axis.py
@@ -1533,11 +1533,22 @@ class Axis(martist.Artist):
         """
         if u == self.units:
             return
-        self.units = u
-        self._update_axisinfo()
-        self.callbacks.process('units')
-        self.callbacks.process('units finalize')
-        self.stale = True
+        if self is self.axes.xaxis:
+            shared = [
+                ax.xaxis
+                for ax in self.axes.get_shared_x_axes().get_siblings(self)]
+        elif self is self.axes.yaxis:
+            shared = [
+                ax.yaxis
+                for ax in self.axes.get_shared_y_axes().get_siblings(self)]
+        else:
+            shared = [self]
+        for axis in shared:
+            axis.units = u
+            axis._update_axisinfo()
+            axis.callbacks.process('units')
+            axis.callbacks.process('units finalize')
+            axis.stale = True
 
     def get_units(self):
         """Return the units for axis."""

work? (untested...)

[timhoffm]

Looks like that could work. Mildly worried that the last five lines are too low level to be called across Axises and that that should be a possibly private single function of Axis, but 🤷 .

@l-johnston would you be interested in trying out @anntzer's suggestion?

[jklymak]

I would need to check, but I think the axis unit setting gets called when data are added. So now the user plots floats on the first axis and then plots datetime on the shared one. What do we want to happen? Its not clear to me that we want the first axis to suddenly be changed to datetime units. I'd like to see a few more tests to see how this really behaves if the user does something incorrect.

[l-johnston]

yes, @anntzer 's approach works with slight mod.

[l-johnston]

@jklymak in the case of an actual unit system implementation such as unyt, attempting to set conflicting units on a shared axis raises a unit conversion error. In the case of attempting to set the shared axis to a non-unit data type, like a date, produces a plot, but of course doesn't make any sense.

To prove all of this in the tests, I would need a complete unit system implementation. Is it acceptable to import unyt?

[l-johnston]

@timhoffm I switched to @anntzer 's approach. Is there anything else to do?

[jklymak]

#18311 seems like a reasonable test.

[jklymak]

This seems right, but I'm somewhat leery given how sharex and sharey are poorly defined and how units are still poorly defined. I really can't tell if adding more magic is good or bad. a) it would be good if sharex and sharey kwargs could be updated with info that says this would be happening. b) if there is a sharing tutorial it should be updated, or if not, one could be added. c) other basic units we use for conversion should be explicitly tested to show they do the right thing. i.e. datetime and categorical.

I almost preferred the previous version where the new feature was largely in new explicit methods that could document this. Adding automatic magic seems riskier to me. Further, I disagree somewhat with @timhoffm's conclusion that things should be pushed down to the axis objects. I don't think axis objects are well enough documented and have far too much low-level things that are almost private that we can expect users to find methods there. That is largely a documentation objection though.

[l-johnston]

@jklymak The only default ConversionInterface instantiated in matplotlib is DecimalConverter that converts decimal.Decimal to floating point and it does not do anything with the units parameter. I have tested decimal.Decimal and the result is the same as before. datetime also performs the same as before.

In the current commit, I added a Notes section to the docstring explaining that set_units will update shared axis.

As a user, I prefer working with Axes rather than Axis. Also, there is precedence with set_x/y_lim and as a user, set_x/y_units would seem natural. Also, adding the emit feature would give an opt-out.

[anntzer]

I don't have a really strong opinion about how units and sharing should interact (mostly because I still have no idea how units are supposed to behave anywhere 🤷), but I'll just comment on the following:

Further, I disagree somewhat with @timhoffm's conclusion that things should be pushed down to the axis objects.

In my view, if we do add Axes.set_xunits/Axes.set_yunits, it would be a really annoying trap to have ax.set_xunits(foo) behave subtly differently from ax.xaxis.set_units(foo) (especially given that they will behave the same in most cases, and only cause issues when there are shared axes).

[jklymak]

it would be a really annoying trap to have ax.set_xunits(foo) behave subtly differently from ax.xaxis.set_units(foo)

I guess that depends on how low-level we think xaxis.set_units is and whether we think an axis should know anything about the sharing of its parent axes. There are strong arguments for just having all the sharing at the axis level, but that is not how things are set up now. Usually sharing is instantiated at the axes level, so its hard as a user to know where to go to control the sharing if we put knobs down on the axis level.

[dopplershift]

Sharing axes shares limits and ticks, so it makes sense to me that units would follow suit. Just a minor comment to address from me.

[timhoffm]

This looks like the right solution to me. Same behavior between Axis and Axes is to be preferred if possible (and here it is).

@jklymak leaving open, to give you the chance of a final word. Whether pulling up wrappers to Axes or accessing the lower level Axis is the preferred way, may need a final say but the dev call today showed a tendency for using Axis. I agree that Axis docs could use improvement.
Leaving open, to give @jklymak a chance for

[jklymak]

This seems fine. As a todo we should document everything that sharex/y means somewhere in the docs where they are used.