Fix nonsensical transform in mixed-mode axes aspect computation.#14990
Fix nonsensical transform in mixed-mode axes aspect computation.#14990timhoffm merged 2 commits intomatplotlib:masterfrom
Conversation
anntzer
added
the
Release critical
efiring
left a comment
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.
lib/matplotlib/tests/test_axes.py
Outdated
| 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"?
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 :)
anntzer
force-pushed
the
mixedscaleaspect
branch
from
d3db965 to
6108d49
Compare
3 participants
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 thanmap(trf.transform, array), i.e.Transform.transformworks on arrays (and is documented to do so).attn @efiring
PR Checklist