Skip to content
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

ENH: make ax.get_position apply aspect #9855

Merged
merged 3 commits into from Apr 10, 2018

Conversation

jklymak
Copy link
Member

@jklymak jklymak commented Nov 25, 2017

PR Summary

Currently, ax.get_position(original=False) doesn't tell us the position of an Axes if its aspect ratio has been set, unless we call ax.apply_aspect() or fig.draw() beforehand. This is a bit annoying, because its hard to imagine why we would want to ask for the axes position without the proper aspect ratio already having being applied, particularly if we can get the original position by calling ax.get_position(original=True).

This PR, simply calls self.apply_aspect() if original=False.

The doc string was also updated to what I hope is numpydoc compliance.

Closes #9207

PR Checklist

  • Has Pytest style unit tests
  • Code is PEP 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

@jklymak jklymak changed the title ENH: make ax.get_position apply aspect WIP: ENH: make ax.get_position apply aspect Nov 25, 2017
@tacaswell tacaswell added this to the v2.2 milestone Nov 25, 2017
@tacaswell
Copy link
Member

Nominally 👍.

Share @efiring 's concern about performance, but this seems a good a place as any in the code to force application of the aspect. There is still a path confusion (create axes, set aspect, ask position, change limits) but is a more limited path that the current one (create axes, set aspect, set limits, ask position, draw).

@jklymak
Copy link
Member Author

jklymak commented Nov 25, 2017

I'm less concerned about performance. I think if the user asks for the position, and they specify "original=False" they should get what they ask for. If its a performance issue for them, then they can figure things out, and then hardcode.

A thornier question, however, is if the default should be "original=True", since thats what most users were getting when calling get_position(). That is if they didn't know the trick of calling ax.apply_aspect(). But for all I can tell that trick would only be discernible from reading the source code. I would never have thought about calling that, and I got pretty heavily into the source code in #9082.

@jklymak jklymak force-pushed the enh-update-aspect branch 2 times, most recently from 0e51daf to d4d11f3 Compare November 25, 2017 23:44
@jklymak jklymak changed the title WIP: ENH: make ax.get_position apply aspect ENH: make ax.get_position apply aspect Nov 25, 2017
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.

From the standpoint of internals, the api_changes is a little bit misleading; with original=False it was returning the active position, which at that point might or might not be identical to the original, and which might or might not be the position after the draw. The latter is still true, but this PR makes it more likely that get_position(False) returns what the user might reasonably expect. So I think it is a net gain, and worth trying.

@jklymak jklymak modified the milestones: needs sorting, v2.2.0 Mar 1, 2018
@jklymak
Copy link
Member Author

jklymak commented Mar 1, 2018

I'm sticking this in 2.2. Feel free to re-milestone, but its a pretty small change.

@dstansby
Copy link
Member

dstansby commented Mar 1, 2018

I'd be in favour of putting this in 3.0 since it's a change that should probably go out in an rc before the final release.

@jklymak jklymak modified the milestones: v2.2.0, v3.0 Mar 1, 2018
@jklymak
Copy link
Member Author

jklymak commented Mar 18, 2018

Now that we are on 3 can this go in?

Copy link
Member

@phobson phobson left a comment

Choose a reason for hiding this comment

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

Looks good

@phobson phobson merged commit 83507dc into matplotlib:master Apr 10, 2018
@jklymak jklymak deleted the enh-update-aspect branch March 5, 2019 16:10
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.

axes has no method to return new position after box is adjusted due to aspect ratio...
6 participants