Skip to content

Allow equal aspect box on shared axes #10961

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

Conversation

ImportanceOfBeingErnest
Copy link
Member

@ImportanceOfBeingErnest ImportanceOfBeingErnest commented Apr 5, 2018

PR Summary

This would allow to set an equal aspect box on a shared axes.

Until now the following code

from matplotlib import pyplot as plt

fig, ax1 = plt.subplots()
ax2 = plt.twiny() 

ax1.imshow([[0,1],[1,0]], extent=(0,5,10,20))

ax2.set_aspect("equal", adjustable="box", share=True)
ax2.axis([36,41,10,20])  

plt.show()

would produce an error. Turning the error into a warning lets this code produce the desired output

image

PR Checklist

  • Has Pytest style unit tests (would it need one?)
  • Code is PEP 8 compliant

@ImportanceOfBeingErnest
Copy link
Member Author

"0% of diff hit (target 50%)" == complete mystery to me

@jklymak
Copy link
Member

jklymak commented Apr 5, 2018

  1. the codecov means this code path has no test.
  2. I think this error is in there for good reason... ping @efiring

@efiring
Copy link
Member

efiring commented Apr 5, 2018

The problem is that this only gives the desired result because you have carefully chosen your limits to be consistent. I think that allowing this in mpl is bad policy; hence the error. Couldn't you achieve your desired result without sharing?

@ImportanceOfBeingErnest
Copy link
Member Author

I guess I could get the same output with something like

from matplotlib import pyplot as plt

fig = plt.figure()
ax1 = fig.add_subplot(111, label="a")
ax2 = fig.add_subplot(111, label="b")

ax1.imshow([[0,1],[1,0]], extent=(0,5,10,20))

ax2.tick_params(left=False, bottom=False, top=True, 
                labeltop=True, labelbottom=False,
                labelleft=False)
ax2.set_aspect("equal", adjustable="box")
ax2.patch.set_visible(False)
ax2.axis([36,41,10,20])  

plt.show()

But isn't that restriction a bit artificial?
I mean, if you do not set the limits correctly, you will still get a valid plot, but it may look bad in the sense of overlapping axes. So you always get what you ask for; and the warning tells you that this may not necessarily coincide with what you were hoping to get at.
Or is there anything that breaks down and would justify the error?

@efiring
Copy link
Member

efiring commented Apr 5, 2018

I don't think it is artificial; it is preventing an inconsistency. Twinning requires the boxes to be identical, and it locks together the ylim of the two Axes (in the twiny case). Setting the aspect ratio on both means that at draw time, for each Axes in sequence, either its data limits or its box is adjusted. Since the xlims are independent, in your example, this means there is a fundamental conflict between the need to adjust the boxes and the requirement that they be identical. I don't think our API should allow this, even though you can set the xlims so that the boxes would in fact be the same.

@ImportanceOfBeingErnest
Copy link
Member Author

there is a fundamental conflict between the need to adjust the boxes and the requirement that they be identical.

That's probably the main point: Where exactly does this requirement come from?
More precisely, I would say the requirement for a twiny axes is that the boxes are identical in height, for a twinx axes that they are idential in width. As far as I can see this is still preserved in the code base.

@efiring
Copy link
Member

efiring commented Apr 6, 2018

Well, I suppose you could superimpose boxes with the same height but different widths, but chances are it would look pretty bad unless you remove the frames. I think it goes against the essence of twinx and twiny, for which a reasonable expectation is that the Axes position boxes coincide..

@jklymak
Copy link
Member

jklymak commented Apr 6, 2018

It seems the fundamental desire is to use twiny is to provide a second x-spine for the original axes that will have different locators and formatters (an alternate scale), rather than create a second axes that will be plotted on. Is there any other way to achieve that right now?

@efiring
Copy link
Member

efiring commented Apr 6, 2018

I agree that this is desirable, and at present it not easy in any obvious way that I know of. Everything is so tightly tied to the Axes object, and the Axes object has only a single Locator and Formatter per Axis. Given that, the solution is probably to make a second Axes with everything turned off except for the desired spine, and use callbacks to lock the position and the desired spine limits to those from the master Axes in which the actual plotting is done. The callback for the limits could then transform those numbers (and the entire scale) from centigrade to fahrenheit, for example, or from frequency to period (which would be my use case), and the second Axes Locator and Formatter for the spine would be able to make appropriate ticks and labels.
We have callbacks for set_xlim and set_ylim; we lack a callback for set_position.

@jklymak
Copy link
Member

jklymak commented Apr 6, 2018

maybe this is off-topic and I've misunderstood the root use-case, in which case we can take this elsewhere. Note I'm pretty sure that is the use case in #10960. But...

Could we just have a list of secondary_xaxis and secondary_yaxis objects that are just subclasses of xaxis objects but also have the "unit" conversion, have an offset of some sort, and are slave to their parent x/yaxis? These could simply be updated every time the parent axes is updated, but would have a known parent (the backwards compatible Axes.xaxis)? I say a list, because if we allow one extra spine, why not more.

@efiring
Copy link
Member

efiring commented Apr 6, 2018

That might work. It looks like the only use of Axes.transData in the Axis is for an optional tweak, so the subclass could disable it.

@efiring
Copy link
Member

efiring commented Apr 6, 2018

The normal Axis gets its view_interval from its Axes.dataLim. You would change methods in the subclass so that some transformation function would be applied to Axes.dataLim in setting the slave view_interval?
I think this approach might involve more code than my suggestion of adding a callback for set_position, and providing a helper that makes the mostly-blank slave Axes.
I'm not sure which is better, or whether there is another approach entirely that should be pursued.

@ImportanceOfBeingErnest
Copy link
Member Author

Of course any general solution to producing secondary axes for images is preferrable compared to this attempted workaround.

A meta thing: What you are discussing now goes a bit beyond of the purpose of this PR. If I understand correctly my desired change is rejected. So would it make sense to close this PR and continue general discussion about secondary axes elsewhere, in an issue where it can later be found and used by people implementing those features?

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.

@efiring may veto this, but I actually think warning here is fine. The stated use case is completly appropriate, and it appears there is a community out there that uses twinx/y for this purpose. Hopefully no users are going to complain when things look wonky after getting an explicit warning that things may look wonky....

@jklymak
Copy link
Member

jklymak commented Apr 6, 2018

Ooops, approval should be modulo a test checking that the warning is emitted...

Copy link
Member

@efiring efiring left a comment

Choose a reason for hiding this comment

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

I still consider this to be bad policy, and would prefer to see the PR closed.

@WeatherGod
Copy link
Member

WeatherGod commented Apr 7, 2018 via email

@efiring
Copy link
Member

efiring commented Apr 7, 2018

@WeatherGod, try this, if it is analogous to your use case:

fig, ax = plt.subplots(ncols=2, sharex=True, sharey=True)
ax[0].imshow(np.random.randn(15, 12))
ax[1].imshow(np.random.randn(15, 12))

There is no more box-forced; box works in this case.

@WeatherGod
Copy link
Member

WeatherGod commented Apr 7, 2018 via email

@efiring
Copy link
Member

efiring commented Apr 7, 2018

It was in #10033.

@ImportanceOfBeingErnest
Copy link
Member Author

What's the status here? Should this be closed? Should it be left as a place for discussion?

@efiring
Copy link
Member

efiring commented Apr 16, 2018

I still recommend that it be closed.

@ImportanceOfBeingErnest
Copy link
Member Author

OK, closing this and counting on some other approach to allow for equal aspect doubly sided axes soon.

@ImportanceOfBeingErnest ImportanceOfBeingErnest deleted the allow-shared-box-equal-aspect branch April 16, 2018 21:12
@jklymak
Copy link
Member

jklymak commented Apr 16, 2018

#11026 is heading that way, though I am planning to stop where I am with that and will add the second slave axes as a different PR once #11026 is in (if its deemed acceptable). Its a little bit orthogonal, with the only relevant addition being the ability to add an axes child...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants