Repository navigation
Fix nonsensical transform in mixed-mode axes aspect computation. - #14990
timhoffm merged 2 commits into
Conversation
Sorry, something went wrong.
efiring
left a comment
There was a problem hiding this comment.
You might want to either remove the comment in the test, or modify the test so that it tests the zoomed-in case where the requested view limits are within larger data limits. This need not block the PR, though.
Sorry, something went wrong.
| ax.apply_aspect() | ||
| assert ax.get_xlim() == pytest.approx(np.array([1/10, 10]) * np.sqrt(10)) | ||
| # Currently the autoscaler chooses to reduce the x-limits by half a decade | ||
| # on each end, but this may change later. |
There was a problem hiding this comment.
I don't know why it would, unless the test itself is changed. I think the point of confusion is that in this test you are plotting only one point, so apply_aspect is understandably and correctly shrinking an axis instead of expanding one. xlim and ylim pertain to view limits, not data limits. It looks like I made a big mistake, long ago, in calling the adjustable option "datalim", because it is not actually adjusting data limits--they remain determined by the actual plot calls and Artist additions--but rather, view limits. Maybe we should change the option name (with deprecation, in a separate PR) to "viewlim" or "viewlimits"?
Sorry, something went wrong.
There was a problem hiding this comment.
I just removed the comment and let's punt the discussion of what the ideal behavior should be to, well, not this PR :)
Sorry, something went wrong.
PR Summary
The first commit fixes the nonsensical transform mentioned in #14899, while changing the test introduced in #14727 to match the behavior of autoscale which we decided in #14899 to keep at least for now (as refactoring it is going to require more work).
The second commit fixes the fact that I missed, in #14727, that I could just write trf.transform(array) rather than map(trf.transform, array), i.e. Transform.transform works on arrays (and is documented to do so).
attn @efiring
PR Checklist