Skip to content

Improve(?) implementation of secondary_axis. #14463

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.

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Jun 6, 2019

Currently, secondary_xaxis is implemented by adding a child axes with a
physical height set to zero and y position set accordingly relative to
its parent axes (using the axes_locator mechanism).

This patch changes it so that the child axes' extents actually
matches the parent axes, and instead positioning the spines using
Spine.set_position. It also makes sure that the secondary axes patch is
invisible and that the bounds of the "orthogonal" axis (the one that is
not shown) matches the parent bounds.

By doing so, it becomes possible to plot data directly on the secondary
axis as well; e.g. the following now works:

from matplotlib import pyplot as plt
from matplotlib.axes import Axes

fig, ax0 = plt.subplots()
ax1 = ax0.secondary_xaxis(
    "top", functions=(lambda x: x**3, lambda x: x**(1/3)))
Axes.plot(ax1, [.25, .5, .75], [.25, .5, .75], ".-")
plt.show()

(We have to use Axes.plot here instead of ax1.plot just because
SecondaryAxes inherits from _AxesBase, not from Axes, but that doesn't
really matter.)

Another advantage is that one can now use secondary_axis as a
replacement for SubplotZero, a relatively obscure feature of
mpl_toolkits that is only showcased in 3 examples:

https://matplotlib.org/gallery/axisartist/simple_axisline.html
https://matplotlib.org/gallery/axisartist/simple_axisline2.html
https://matplotlib.org/gallery/axisartist/demo_axisline_style.html

whose goal is just to draw a spine at x=0 or y=0.

simple_axisline2 just moves its main spine to y=0, so we can implement
that directly with Spine.set_position (see also simple_axisartist1).

simple_axisline adds additional spines, and I added an equivalent
implementation using secondary_axis, which is fairly transparent.
(This example, as well as test_secondary_xy, show why the axes patch
must be made invisible: otherwise, the patch of later secondary axes
would be drawn over other secondary axes.)

demo_axisline_style doesn't showcase anything that's specifically
related to SubplotZero so I just rewrote it without SubplotZero.

If we agree that secondary_axis is a suitable replacement, SubplotZero
could ultimately be deprecated (in a later PR).

Minor points:

Also delete SecondaryAxis._loc, which is set in a single place and
never used.

PR Summary

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
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@anntzer anntzer requested a review from jklymak June 6, 2019 13:59
Copy link
Member

@jklymak jklymak left a comment

Choose a reason for hiding this comment

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

Intriguing idea. Seems problematic to allow plotting on the secondary axes as we will certainly have folks confused about zorder.

Did you check that the new axes locator behaves under resizing the figure?

@anntzer
Copy link
Contributor Author

anntzer commented Jun 6, 2019

Resizing works as far as I can tell.

Allowing drawing on the secondary axes is mostly just a nice side effect; I'm not suggesting to add a public API there (it would need its own discussion). On the other hand I don't think zorder is that complicated (basically, the children of the secondary axes all get inserted into the main list at the position specified by the secondary axes zorder) -- but again we can discuss this separately.

ax2.set_ylabel("Label Y2")

ax.plot([-2, 3, 2])

Copy link
Member

Choose a reason for hiding this comment

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

OK, this is nice. Can this be added as an example in the main spines section as well? Or moved there, with a link to it here? Putting this second in this section makes it look like this is a deprecated way to do it, whereas I think the goal is for it to be the canonical way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the additions to simple_axisline2 and simple_axisartist1 and just linked them https://matplotlib.org/devdocs/gallery/ticks_and_spines/spine_placement_demo.html#sphx-glr-gallery-ticks-and-spines-spine-placement-demo-py instead.

I think I'll leave the multi-axes example as it is right now because there's no completely equivalent example elsewhere and I don't want to bother about proper explanations for now, but we can always move it there later (e.g. when we kill SubplotZero...).

@ImportanceOfBeingErnest
Copy link
Member

I would argue that those arrows on top of the spines are closely linked to having the axes through (0,0). Essentially my impression is that there are two kind of graphs around: "Math graphs", that go through zero and have directional symbols on them, and "Engeneers graphs" that look like the default graphs in matplotlib (possibly with or without top and right border). My vote would hence be to keep showcasing zero passing axes with arrows.

@anntzer
Copy link
Contributor Author

anntzer commented Jun 7, 2019

It's not really hard to add arrows to "standard" (non-mpltoolkits) matplotlib graphs (it's just a matter to add an annotate() (or arrow()) call in axes coordinates; https://gist.github.com/joferkington/3845684 / https://3diagramsperpage.wordpress.com/2014/05/25/arrowheads-for-axis-in-matplotlib/).
We could certainly think of having a better API for that, but I was hoping to keep that a separate discussion...

@ImportanceOfBeingErnest
Copy link
Member

The canonical order in such cases is: Provide an alternative, then remove the original, not vice versa.

I think the changes to secondary_xaxis have enough merit by themselves, so the SubplotZero stuff can be cut out?

@anntzer
Copy link
Contributor Author

anntzer commented Jun 7, 2019

I'm not removing the old (and currently only) way to do that (I'm no even removing the examples for them!); I'm just discussing that the changes here will later enable further changes.

@efiring
Copy link
Member

efiring commented Jul 8, 2019

On the call today we (@anntzer and I) discussed this. One idea is to more thoroughly unify twinned axes with secondary axes so that the user would need to remember less about their (reduced) differences. For example, if secondary axes were not children then the handling of zorder would be identical between twinned axes and secondary axes.

@jklymak
Copy link
Member

jklymak commented Jul 8, 2019

I think you are just suggesting that a secondary axes is just a twinned axes with a special relationship between the parent and the twin. I guess that’s ok but I don’t like the ambiguity. We have lots of ambiguity with twinned axes, shared axes, etc and it all becomes a bit confusing. Currently a secondary axes is just an annotation on the parent axes. You can’t plot on it. That makes it a lot simpler. I’m not sure why we want to move to something more ambiguous just to give the trivial ability to plot in different units. The user already has the forward and inverse functions so it’s not like it’s hard to plot on the parent axes.

@anntzer
Copy link
Contributor Author

anntzer commented Apr 17, 2020

@ImportanceOfBeingErnest I guess you handled your own concern re "math-style axes" via the solution at #17170? :-)

@ImportanceOfBeingErnest
Copy link
Member

I guess my concern is superseeded by my comment in #17170, that is, keep showcasing axisartist functionality, but in the context of that module.

Currently, secondary_xaxis is implemented by adding a child axes with a
physical height set to zero and y position set accordingly relative to
its parent axes (using the axes_locator mechanism).

This patch changes it so that the child axes' extents actually
*matches* the parent axes, and instead positioning the spines using
Spine.set_position.  It also makes sure that the secondary axes patch is
invisible and that the bounds of the "orthogonal" axis (the one that is
not shown) matches the parent bounds.

By doing so, it becomes possible to plot data directly on the secondary
axis as well; e.g. the following now works:
```
from matplotlib import pyplot as plt
from matplotlib.axes import Axes

fig, ax0 = plt.subplots()
ax1 = ax0.secondary_xaxis(
    "top", functions=(lambda x: x**3, lambda x: x**(1/3)))
Axes.plot(ax1, [.25, .5, .75], [.25, .5, .75], ".-")
plt.show()
```
(We have to use Axes.plot here instead of ax1.plot just because
SecondaryAxes inherits from _AxesBase, not from Axes, but that doesn't
really matter.)

Another advantage is that one can now use secondary_axis as a
replacement for SubplotZero, a relatively obscure feature of
mpl_toolkits that is only showcased in 3 examples:

https://matplotlib.org/gallery/axisartist/simple_axisline.html
https://matplotlib.org/gallery/axisartist/simple_axisline2.html
https://matplotlib.org/gallery/axisartist/demo_axisline_style.html

whose goal is just to draw a spine at x=0 or y=0.

simple_axisline2 just moves its main spine to y=0, so we can implement
that directly with Spine.set_position (see also simple_axisartist1).

simple_axisline adds additional spines, and I added an equivalent
implementation using secondary_axis, which is fairly transparent.
(This example, as well as test_secondary_xy, show why the axes patch
must be made invisible: otherwise, the patch of later secondary axes
would be drawn over other secondary axes.)

demo_axisline_style doesn't showcase anything that's specifically
related to SubplotZero so I just rewrote it without SubplotZero.

If we agree that secondary_axis is a suitable replacement, SubplotZero
could ultimately be deprecated (in a later PR).

Minor points:

Also delete `SecondaryAxis._loc`, which is set in a single place and
never used.
@jklymak
Copy link
Member

jklymak commented Apr 23, 2021

I'm still mildly against this. I think secondary_axis is just an axis, not a full fledged axes you can plot on. Folks should use twin for an axes.

@jklymak jklymak marked this pull request as draft April 23, 2021 15:20
@anntzer
Copy link
Contributor Author

anntzer commented Apr 23, 2021

Added this to the weekly call agenda, should hopefully be relatively quick to discuss and reach a decision.

@jklymak
Copy link
Member

jklymak commented Apr 23, 2021

Sure, but can you re-iterate why it should be an axes versus just using twin? I genuinely don't understand...

@anntzer
Copy link
Contributor Author

anntzer commented Apr 23, 2021

Because this way you can write something like

fig, ax_celsius = plt.subplots()
ax_farenheit = ax0.secondary_yaxis(
    "top", functions=(celsius_to_farenheit, farenheit_to_celsius))
ax_celsius.plot(celsius_data)
Axes.plot(ax_farenheit, farenheit_data)
plt.show()

and have everything just work?

(Note that I would not argue to add that feature if it had to be implemented from scratch; the point is that all the required machinery basically already there so it seems a pity not to make it available.)

@jklymak
Copy link
Member

jklymak commented Apr 23, 2021

Sure, but isn't that what twinx does? Is it keeping the data limits in sync that you are after?

@anntzer
Copy link
Contributor Author

anntzer commented Apr 23, 2021

twinx() doesn't even ensure that the celsius data will be correctly readable on the farenheit axis?

@jklymak
Copy link
Member

jklymak commented Apr 23, 2021

OK, but then should this replace twinx? Should twinx gain this functionality?

My original idea with secondary_axis was for a lightweight annotation artist that didn't confuse people like the existing twin method did; if I could have not had an axes created at all, I'd have done so, but unfortunately an Axis doesn't contain all the artists for some reason.

But if we can make this clean I'm not opposed. We could also come up with another API for the two things.

@anntzer
Copy link
Contributor Author

anntzer commented Apr 23, 2021

OK, but then should this replace twinx? Should twinx gain this functionality?

For me, twinx means you're plotting two unrelated datasets, whose only common point is that they share the independent variable (e.g. temperature vs time and pressure vs time). secondary_axis means that you're plotting datasets that are directly comparable, except that they may be expressed in various units (e.g. celsius vs. farenheit).

But if we can make this clean I'm not opposed. We could also come up with another API for the two things.

Agreed I'm not particularly wedded to the API, again my main point was that this is basically already available as is.

@jklymak
Copy link
Member

jklymak commented Apr 23, 2021

OK, I think that makes sense. Does the drawing happen on the parent or the child? If we passed add_artist to self.parent.add_artist, that would maybe take care of a lot of the confusion of having two axes. OTOH it wouldn't allow two sets of grids, which folks have asked for in the past.

@anntzer
Copy link
Contributor Author

anntzer commented Apr 23, 2021

Right now it happens on the child, but perhaps forwarding add_artist to the parent may indeed be better.

@jklymak
Copy link
Member

jklymak commented Apr 23, 2021

I think this brings up issues of what the cursor should show, etc, but I think by stating one is the parent, and one is the child, we can just say the cursor follows the parent regardless of which method the artist was added with.

I guess I think this should be a different method. We could even consider soft-deprecating secondary_axis in favour of secondary_axes. Its marked "Experimental" so soft deprecating it is not out of the question. I think if people weren't ever confused about which axes gets the picks, or can have a facecolor etc, then this could work as a nice convenience.

@anntzer
Copy link
Contributor Author

anntzer commented Apr 29, 2021

Discussed on call. Agreement was that this would be nice to have, but we need to make sure everything is well lined up. Questions are listed in the summary of the discussion, but among the points I need to look at: 1) does the orthogonal direction get correctly synced? 2) how much implementation can be shared between this and twinx/y? 3) can we pull in parasite_axes functionality, i.e. both axes' artists get z-ordered together, they share the same color cycle? (also legend() works, etc.)

@github-actions
Copy link

Since this Pull Request has not been updated in 60 days, it has been marked "inactive." This does not mean that it will be closed, though it may be moved to a "Draft" state. This helps maintainers prioritize their reviewing efforts. You can pick the PR back up anytime - please ping us if you need a review or guidance to move the PR forward! If you do not plan on continuing the work, please let us know so that we can either find someone to take the PR over, or close it.

@github-actions github-actions bot added the status: inactive Marked by the “Stale” Github Action label Jun 19, 2023
@anntzer anntzer added keep Items to be ignored by the “Stale” Github Action and removed status: inactive Marked by the “Stale” Github Action labels Jun 19, 2023
@anntzer
Copy link
Contributor Author

anntzer commented Jun 19, 2023

Looks like the agreement from the call 2y ago was to try moving this forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
keep Items to be ignored by the “Stale” Github Action status: needs rebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants