-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
ENH: make ax.get_position apply aspect #9855
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
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). |
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 |
0e51daf
to
d4d11f3
Compare
e983130
to
d260cca
Compare
There was a problem hiding this 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.
I'm sticking this in 2.2. Feel free to re-milestone, but its a pretty small change. |
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. |
Now that we are on 3 can this go in? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good
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 callax.apply_aspect()
orfig.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 callingax.get_position(original=True)
.This PR, simply calls
self.apply_aspect()
iforiginal=False
.The doc string was also updated to what I hope is numpydoc compliance.
Closes #9207
PR Checklist