twinx / twiny inherit autoscale behavior for shared axis #7904

naoyak · 2017-01-21T08:46:09Z

Creating twinx() or twiny() should inherit the autoscaling behavior from the parent Axes instance, but only for the shared axis. Previously, twinx() would autoscale on the x-axis regardless of the setting on the original Axes, meaning that plotting on the twin would look incorrect with the (visible) original x-axis.

story645 · 2017-01-23T22:50:20Z

lib/matplotlib/axes/_base.py

        self.yaxis.tick_left()
        ax2.xaxis.set_visible(False)
+        ax2.set


what does this line do?

oops, made a goof there. removed.

story645 · 2017-01-23T22:52:15Z

lib/matplotlib/tests/test_axes.py

@@ -145,6 +145,24 @@ def test_twinx_cla():
    assert ax.yaxis.get_visible()


+@cleanup
+def test_twin_inherit_autoscale():


As much as I dislike image tests, do you have one of those, or some other inspection of the axis to check that the ticks are acting the way you expect?

Yeah, I could create something motivated by the example in #6860 (i.e. that the plotted points should reflect that the original (visible) axis and twinned (invisible) axis share the same autoscaling behavior.

added a simple image comparison.

story645 · 2017-01-23T22:53:41Z

Also, how does this intersect with #7528?

naoyak · 2017-01-23T23:11:02Z

mm, i have to check out #7528. I think I need to understand a bit better what _make_twin_axes() is doing...

naoyak · 2017-01-24T04:00:05Z

I don't think this PR conflicts directly with #7528 in terms of functionality, but maybe testing can be consolidated. Seems like there's some confusion on whether the shared axis should be the same axis or a copy (whose attributes can be changed independently), both from the API perspective and in terms of the internals.

NelleV · 2017-01-25T17:33:05Z

This looks good to me. I'd just add a bit of documentation on the twinx and twiny method to explain this behaviour.

naoyak · 2017-01-25T18:54:55Z

@NelleV thanks, added a note about the autoscale setting to the docstrings.

NelleV · 2017-01-25T19:05:20Z

Thanks! It looks good to me.

story645 · 2017-01-26T04:40:01Z

lib/matplotlib/axes/_base.py

@@ -3902,18 +3902,18 @@ def _make_twin_axes(self, *kl, **kwargs):

    def twinx(self):
        """
-        Create a twin Axes sharing the xaxis
+        Create a twin Axes instance sharing the x-axis with self


not clear what self is here? (Basically not sure this is any clearer than the original wording)

story645 · 2017-01-26T04:47:10Z

lib/matplotlib/axes/_base.py

-        ticks on left and the returned axes will have ticks on the
-        right. To ensure tick marks of both axis align, see
-        `~matplotlib.ticker.LinearLocator`
+        Create a twin Axes instance for generating a plot with a shared


also not sure this is any clearer. docs support images, so maybe that's the way to go here?
https://github.com/matplotlib/matplotlib/blob/master/lib/matplotlib/axes/_axes.py#L1660

An image would be a good idea. These docs feel like they are about implementation rather than the use case - I think what we want to say is "call this method if you want to create a plot with 1 x-axis and 2 y-axes". To me the extra xaxis object is an artifact which is hidden, but apparently there is another use case...

Since these are the dev docs, they kinda have to stay in implementation land. Your string is great, but more appropriate for a gallery example. Honestly, I'd just prefer the original docstring. I think with a small image, they're clear

If the docstring is going to focus on implementation, it still needs to be revised to reflect what's actually going on behind the scenes. twinx/twiny doesn't really "share" an axis in the sense of sharing the Axis object, but instead creates a 2nd, invisible one..I can try to encapsulate this aspect and revise.

story645 · 2017-01-26T04:48:40Z

lib/matplotlib/axes/_base.py


        Notes
-        ------
+        -----
        For those who are 'picking' artists while using twiny, pick
        events are only called for the artists in the top-most axes.


top-most relative to what?

I think this means the "original" Axes.

Actually I'm not really familiar with the Artist/picker API, I'm not really sure what top-most is in this context..

The Axes are rendered in some order (determined by the z-order and sorted in the Figure draw method). Which ever one draws last (has highest z-order) is the one that gets the pick event.

If I remember correctly, the new axis ends up on top. I remember having some issues with legends getting obscured by gridlines because of this.

story645 · 2017-01-26T21:40:43Z

lib/matplotlib/axes/_base.py

@@ -3902,18 +3902,18 @@ def _make_twin_axes(self, *kl, **kwargs):

    def twinx(self):
        """
-        Create a twin Axes sharing the xaxis
+        Create a twin Axes instance sharing the x-axis with the parent 


with the parent what?

is it that unclear? maybe "parent" isn't the exact terminology, but after all twinx() is a method called on an existing Axes. I am going to revert this part of the docstring edit.

story645 · 2017-01-26T21:40:53Z

lib/matplotlib/axes/_base.py

        self.yaxis.tick_left()
        ax2.xaxis.set_visible(False)
        ax2.patch.set_visible(False)
        return ax2

    def twiny(self):
        """
-        Create a twin Axes sharing the yaxis
+        Create a twin Axes instance sharing the y-axis with the parent


same as above

naoyak · 2017-01-27T04:28:40Z

OK, I've revised the twinx and twiny docstrings to reflect the changes in this PR and clarified the wording to the best of my ability.

However this makes me wonder if the implementation is flawed in that the scales of the twinned axis will diverge if autoscale is turned on, since plotting is done on either the original or the new Axes. Would it make more sense to share the same Axis object?

tacaswell · 2017-01-27T04:46:38Z

lib/matplotlib/axes/_base.py

        `~matplotlib.ticker.LinearLocator`

        Returns
        -------
-        Axis


wow, good catch!

story645 · 2017-01-27T07:19:25Z

This is making my head hurt. I'm wondering, and you likely know better than me at this point, but what I'm getting from you is that one y is hooked into the displayed x and the other is y hooked into the hidden x? if so, I agree that they they should just both be hooked into a single x, but I wonder if that's possible...

naoyak · 2017-01-27T08:11:43Z

Actually, sorry it seems to work fine with the separate xaxis instances - the autoscaling does stay in sync between the "original" and the "twinned" x-axes. So while the implementation is still confusing to me, I think this will work OK.

import numpy as np
import matplotlib.pyplot as plt

fig, ax1 = plt.subplots()
t = np.arange(0.01, 10.0, 0.01)
s1 = np.exp(t)
ax1.plot(t, s1, 'b-')
ax1.set_xlabel('original x-axis')
ax1.set_ylabel('exp', color='b')
ax1.tick_params('y', colors='b')
ax1.tick_params('x', colors='b')

ax2 = ax1.twinx()
ax2.set_ylabel('sin', color='r')
ax2.tick_params('y', colors='r')
ax2.tick_params('x', colors='r')

# Manually set the twinned x-axis visible and shift it down
ax2.xaxis.set_visible(True)
ax2.spines['bottom'].set_position(('axes', -0.2))
ax2.set_xlabel('twinned x-axis')
t2 = np.arange(0.01, 20.0, 0.01)
s2 = np.sin(2 * np.pi * t2)
ax2.plot(t2, s2, 'r.')

plt.show()

story645 · 2017-01-27T08:48:08Z

Don't apologize! There are loads of moving pieces here and it gets confusing quickly. Ok, then far as I can tell, the goal of #7376 is to treat original and twin x as two semi-independent axis, in that they should have the same min & max but can be labeled individually. And so making a shared x-axis would probably make that functionality impossible. @madphysicist wanna chime in?

naoyak · 2017-01-27T09:08:12Z

Yep, making a shared xAxis instance would seem to preclude the functionality in #7376. Fortunately we don't have to go down that road to resolve the relevant issues here 😄

This should be ready now - if it gets merged @madphysicist will have to sort a merge conflict in the docstring, but nothing functionally impactful, I think.

story645 · 2017-01-27T09:38:00Z

Looks good to me, just waiting on gitter.im for how to deal with the appveyor failure that has nothing to do with your test.

madphysicist · 2017-01-27T14:57:14Z

lib/matplotlib/axes/_base.py

        `~matplotlib.ticker.LinearLocator`

        Returns
        -------
-        Axis
-            The newly created axis
+        Axes


I think that this should be lowercase.

madphysicist · 2017-01-27T14:58:59Z

lib/matplotlib/axes/_base.py

+        x-axis positioned opposite to the original one (i.e. at top). The
+        y-axis autoscale setting will be inherited from the original Axes.
+        To ensure that the tick marks of both x-axes align, see
+        `~matplotlib.ticker.LinearLocator`


I am not sure that I understand the last sentence. Specifically, how does looking at the documentation for LinearLocator ensure tick alignment? :)

This is the original wording - do you have any suggestions for a better sentence? (or you could incorporate it in your PR since that deals with tickers)

I will try to figure out what that sentence means exactly and attempt to improve it.

madphysicist · 2017-01-27T14:59:11Z

lib/matplotlib/axes/_base.py


        Returns
        -------
-        Axis
-            The newly created axis
+        Axes


… Axes

madphysicist · 2017-01-27T22:23:30Z

@naoyak Until fairly recently, I thought that twinning actually meant sharing the axis. Since that is not the case, your earlier comment makes me question this whole PR:

However this makes me wonder if the implementation is flawed in that the scales of the twinned axis will diverge if autoscale is turned on, since plotting is done on either the original or the new Axes. Would it make more sense to share the same Axis object?

I don't think that sharing the axis object is an option. I think there may need to be a special provision in autoscaling that sets the widest possible bounds for the entire chain of twinned axes. Inheriting autoscaling is definitely a necessary part of this, but autoscaling itself may need to be redefined to special-case axes with twins. I think that fixing that issue that way will also address the concerns about my PR and autoscaling that @efiring brought up.

naoyak · 2017-01-27T22:36:09Z

@madphysicist

I think there may need to be a special provision in autoscaling that sets the widest possible bounds for the entire chain of twinned axes.

I think that's what the current implementation does, actually - I am not sure how this is accomplished under the hood, though. Per discussion above I realized that sharing the Axis object is unnecessary so that is no longer an issue.

At this point this PR only sets the autoscale setting equal for the twinned axis instances and doesn't touch anything else. I think it's safe to merge without messing with your PR, what do you think?

madphysicist · 2017-01-28T01:08:15Z

I think that you are safe to merge. I am working on some autoscaling tests right now. I am pretty sure there will be no conflict but it will be interesting to see how our PRs play together.

QuLogic · 2017-01-29T07:32:19Z

#6860 wants a backport.

naoyak closed this Jan 21, 2017

naoyak reopened this Jan 21, 2017

story645 reviewed Jan 23, 2017

View reviewed changes

naoyak force-pushed the twin-axes-autoscale branch from ed9b984 to b01c31d Compare January 23, 2017 22:57

naoyak closed this Jan 24, 2017

naoyak reopened this Jan 24, 2017

naoyak mentioned this pull request Jan 24, 2017

ENH: Added share_tickers parameter to axes._AxesBase.twinx/y #7528

Closed

naoyak force-pushed the twin-axes-autoscale branch from b01c31d to fcf88d4 Compare January 24, 2017 21:08

naoyak changed the title ~~twinx / twiny inherit autoscale behavior for shared axis~~ [MRG] twinx / twiny inherit autoscale behavior for shared axis Jan 25, 2017

naoyak force-pushed the twin-axes-autoscale branch from 423bf47 to c5ea558 Compare January 25, 2017 18:52

NelleV approved these changes Jan 25, 2017

View reviewed changes

NelleV changed the title ~~[MRG] twinx / twiny inherit autoscale behavior for shared axis~~ [MRG+1] twinx / twiny inherit autoscale behavior for shared axis Jan 25, 2017

story645 reviewed Jan 26, 2017

View reviewed changes

naoyak force-pushed the twin-axes-autoscale branch from c5ea558 to 3a14787 Compare January 26, 2017 05:34

story645 reviewed Jan 26, 2017

View reviewed changes

naoyak force-pushed the twin-axes-autoscale branch 2 times, most recently from ead4817 to 1492d30 Compare January 26, 2017 21:58

tacaswell reviewed Jan 27, 2017

View reviewed changes

lib/matplotlib/axes/_base.py

`~matplotlib.ticker.LinearLocator`

Returns

-------

Axis

Copy link

Member

tacaswell Jan 27, 2017

Choose a reason for hiding this comment

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

wow, good catch!

madphysicist reviewed Jan 27, 2017

View reviewed changes

twinx and twiny inherit autoscale setting for shared axis from parent…

0c0c3d6

… Axes

naoyak force-pushed the twin-axes-autoscale branch from 1492d30 to 0c0c3d6 Compare January 27, 2017 19:03

story645 approved these changes Jan 29, 2017

View reviewed changes

story645 changed the title ~~[MRG+1] twinx / twiny inherit autoscale behavior for shared axis~~ [MRG+2] twinx / twiny inherit autoscale behavior for shared axis Jan 29, 2017

NelleV merged commit 85d76be into matplotlib:master Jan 29, 2017

naoyak deleted the twin-axes-autoscale branch January 29, 2017 04:39

QuLogic changed the title ~~[MRG+2] twinx / twiny inherit autoscale behavior for shared axis~~ \twinx / twiny inherit autoscale behavior for shared axis Jan 29, 2017

QuLogic changed the title ~~\twinx / twiny inherit autoscale behavior for shared axis~~ twinx / twiny inherit autoscale behavior for shared axis Jan 29, 2017

QuLogic added this to the v2.1 milestone Aug 12, 2021

twinx / twiny inherit autoscale behavior for shared axis #7904

twinx / twiny inherit autoscale behavior for shared axis #7904

Conversation

naoyak commented Jan 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

story645 commented Jan 23, 2017 • edited Loading

naoyak commented Jan 23, 2017

naoyak commented Jan 24, 2017

NelleV commented Jan 25, 2017

naoyak commented Jan 25, 2017

NelleV commented Jan 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naoyak commented Jan 27, 2017 • edited Loading

Choose a reason for hiding this comment

story645 commented Jan 27, 2017

naoyak commented Jan 27, 2017 • edited Loading

story645 commented Jan 27, 2017

naoyak commented Jan 27, 2017

story645 commented Jan 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

madphysicist commented Jan 27, 2017

naoyak commented Jan 27, 2017

madphysicist commented Jan 28, 2017

QuLogic commented Jan 29, 2017

story645 commented Jan 23, 2017 •

edited

Loading

naoyak commented Jan 27, 2017 •

edited

Loading

naoyak commented Jan 27, 2017 •

edited

Loading