-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Changed axis selection when zooming datalim-adjustable fixed-aspect axes #14899
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
base: main
Are you sure you want to change the base?
Conversation
33a4d79
to
59a6a2f
Compare
This sounds to me like a substantial loss of perfectly good functionality. Why not keep the logic that handled the expand/shrink decision? It makes changes reversible. If you always expand, then sequential changes to the aspect ratio will cause the data region to be a progressively smaller and smaller part of the view limits, won't they? |
You point about reversibility is well taken, but can you explain the old algorithm in plain words? I honestly do not understand its logic. Also, it appears that the old approach is also not fully reversible, when then margins are set to less than 0.05: e.g., after Perhaps the full solution is to integrate limits adjustment for aspect application together with autoscaling, but that's more surgery... |
@anntzer Here is the current version with the minimal fix, and with a bunch of quickly-written and informal comments inserted to try to explain how the algorithm works. I'm sure that both the algorithm implementation and the comments could be made much clearer. The variable names were never great, but they are even worse now as the code has evolved. In any case, see if the comments added here are sufficient for you to at least understand the algorithm. def apply_aspect(self, position=None):
"""
Adjust the Axes for a specified data aspect ratio.
Depending on `.get_adjustable` this will modify either the Axes box
(position) or the view limits. In the former case, `.get_anchor`
will affect the position.
Notes
-----
This is called automatically when each Axes is drawn. You may need
to call it yourself if you need to update the Axes position and/or
view limits before the Figure is drawn.
See Also
--------
matplotlib.axes.Axes.set_aspect
for a description of aspect ratio handling.
matplotlib.axes.Axes.set_adjustable
defining the parameter to adjust in order to meet the required
aspect.
matplotlib.axes.Axes.set_anchor
defining the position in case of extra space.
"""
if position is None:
position = self.get_position(original=True)
aspect = self.get_aspect()
if aspect == 'auto':
self._set_position(position, which='active')
return
if aspect == 'equal':
aspect = 1
fig_width, fig_height = self.get_figure().get_size_inches()
fig_aspect = fig_height / fig_width
if self._adjustable == 'box':
if self in self._twinned_axes:
raise RuntimeError("Adjustable 'box' is not allowed in a "
"twinned Axes; use 'datalim' instead")
box_aspect = aspect * self.get_data_ratio()
pb = position.frozen()
pb1 = pb.shrunk_to_aspect(box_aspect, pb, fig_aspect)
self._set_position(pb1.anchored(self.get_anchor(), pb), 'active')
return
# self._adjustable == 'datalim'
# reset active to original in case it had been changed by prior use
# of 'box'
self._set_position(position, which='active')
x_trf = self.xaxis.get_transform()
y_trf = self.yaxis.get_transform()
# xmin etc are in data units
xmin, xmax = map(x_trf.transform, self.get_xbound())
ymin, ymax = map(y_trf.transform, self.get_ybound())
# xsize, ysize are the initial screen-unit spans from viewLim
xsize = max(abs(xmax - xmin), 1e-30)
ysize = max(abs(ymax - ymin), 1e-30)
l, b, w, h = position.bounds
box_aspect = fig_aspect * (h / w)
data_ratio = box_aspect / aspect # data ratio to match aspect with box
# It will be used to set the ratio
# *after* transformation to screen
# units, so "data_ratio" is
# perhaps a poor name now that
# transforms are used.
y_expander = data_ratio * xsize / ysize - 1
# If y_expander > 0, the dy/dx viewLim ratio needs to increase.
# y_expander == 0 means the desired data_ratio matches ysize/xsize
# > 0 means ysize needs to increase, or xsize needs to
# decrease
if abs(y_expander) < 0.005:
return
dL = self.dataLim
x0, x1 = map(x_trf.transform, dL.intervalx)
y0, y1 = map(y_trf.transform, dL.intervaly)
xr = 1.05 * (x1 - x0)
yr = 1.05 * (y1 - y0)
# These are like xsize and ysize, but they are taken from dataLim,
# not viewLim. Presumably they never change unless an Artist is
# added or removed. I think the hardwired margins are a detail;
# we can investigate whether they should be replaced by actual
# margins. They might be playing an important role, though, in
# switching between two sets of criteria for deciding whether to
# adjust y or x.
# Now compare the sizes from transformed viewLim to those from
# transformed dataLim *with* hardcoded margins...:
xmarg = xsize - xr # Positive if viewLim is larger than dataLim
ymarg = ysize - yr
# If we keep xsize fixed by changing ysize (in viewLim), what would
# ysize have to be? That is Ysize.
Ysize = data_ratio * xsize
# And Xsize is what xsize would have to be if ysize is held constant.
# All of this is still dealing with sizes transformed to screen units.
Xsize = ysize / data_ratio
# If we hold ysize constant, how much will the new Xsize differ from
# its dataLim counterpart, xr? Will we have extra room available?
Xmarg = Xsize - xr # Positive means x viewLim span exceeds dataLim.
Ymarg = Ysize - yr
# Setting these targets to, e.g., 0.05*xr does not seem to help.
xm = 0
ym = 0
shared_x = self in self._shared_x_axes
shared_y = self in self._shared_y_axes
# Not sure whether we need this check:
if shared_x and shared_y:
raise RuntimeError("adjustable='datalim' is not allowed when both "
"axes are shared")
# If y is shared, then we are only allowed to change x, etc.
if shared_y:
adjust_y = False
else:
if xmarg > xm and ymarg > ym:
# We have room to spare in both viewLims.
# Line 1: y_expander < 0 so y needs to shrink relative to x.
# Shrink y if we still have extra room after doing so.
# Line 2: y_expander > 0 so y needs to expand relative to x.
# Expand y if the alternative, shrinking x, would
# leave us with not enough room for the x dataLim
adjy = ((Ymarg > 0 and y_expander < 0) or
(Xmarg < 0 and y_expander > 0))
else:
# This is the usual case once we have zoomed in, because then
# we normally have at least one axis where we are viewing
# less than the full data range. If y viewLim needs to
# expand, that is the one we adjust; otherwise x. In other
# words, the choice is always expansion of one axis *until*
# we get to the point where we have extra room for *both*
# axes, so something needs to shrink.
adjy = y_expander > 0
adjust_y = shared_x or adjy # (Ymarg > xmarg)
if adjust_y:
yc = 0.5 * (ymin + ymax)
y0 = yc - Ysize / 2.0
y1 = yc + Ysize / 2.0
self.set_ybound(*map(y_trf.inverted().transform, (y0, y1)))
else:
xc = 0.5 * (xmin + xmax)
x0 = xc - Xsize / 2.0
x1 = xc + Xsize / 2.0
self.set_xbound(*map(x_trf.inverted().transform, (x0, x1))) |
The new algorithm is nonsensical unless the same transform is used for both axes, isn't it? Instead of leaving in the test that uses a combination of logit and log, I think we should raise an exception if the scale types are not the same. The test case should exercise only potentially meaningful use cases including log-log and logit-logit. Actually, I'm not even sure the latter makes sense unless both axes are somehow pinned such that their x and y spans are symmetric about zero. |
Thanks for the commented algorithm :)
There's an extra, spurious inverted() that makes the thing completely wrong as described above and should certainly be fixed, but other than that, why do you consider it nonsensical? I don't think requesting "one decade on the x log scale = one unit on the y linear scale" is pointless (or indeed, put any fixed aspect ratio between them), but I believe you do. That seems to be the crux of the matter.
But then does it mean that if I start with symmetric spans, I cannot manually zoom into a side region of my plot while maintaining the aspect ratio? |
OK, I think I can visualize your semilog example. The nice thing about log is that one decade maps to the same screen distance as another. But logit still throws me. Suppose you specify a ratio of 1. There is no analog of the decade. Or consider symlog. What is analogous to a decade for any span that includes part or all of the central linear transition? I guess it is just whatever the transform does. Is this a real use case in which someone will specify a ratio and be pleased by the result? |
This plots the quantiles of two independent normal distributions against one another in logit-logit, with aspect=1. (Yes, I know, a pp-plot or qq-plot would be more suitable, that's just the first thing I could think of.) Certainly it makes sense to zoom to one area of that plot while maintaining the 45°-ness of the x=y? Strictly speaking the grid is not square here (because the equivalent of the decades would be at 1/101 (1:100 odds), 1/11 (1:10 odds), 1/2 (1:1 odds), 10/11 (10:1 odds), 100/101 (100:101 odds) but I guess drawing the ticks at powers of ten is just conventional. Symlog is of course a bit of a desperate case and (in agreement with @jklymak who extensively argued against it elsewhere) I think is too obscure/easy to misuse to be provided by default, but we're stuck with it right now. |
OK, that helps, thank you. In the commented version of apply_aspect that I included above, I think I removed the spurious inversions. Is that correct? And were the added comments sufficient to clarify what it is doing and how? Perhaps the key point is that under normal zooming conditions, the adjustment picks the axis that needs to be enlarged relative to its initial limits. It is only when there is extra room--the initial view limits exceed the data limits by a margin--that an axis may be trimmed from its initial span. If you would like to make the minimal fix, including the test, I would be willing to try (in a subsequent PR) to make the code a little more readable with some combination of comments, better variable names, and probably some re-arrangement. Or you are welcome to do it. But I think in either case it should done separately from the minimum fix. |
Also, in a stage-2 cleanup the margins need a little more thought. |
the release critical part is in #14990. |
When aspect is set and adjustable="datalim", should we change the x limits or the y limits to get the correct aspect? The old code used some complex conditions, which I actually haven't managed to fully understand, to either expand or shrink one of the axises. Instead, just choose to always expand (rather than shrink) one of the axises, which will avoid sending artists out-of-bounds. (The sole exception is in care of shared axes, which we do not touch as explained in the comment.) This patch caused a change in the autolimiting of test_axes.py::test_pie_frame_grid which was buggy anyways, I forced the old behavior by setting x/ylims manually (after checking that the default is to expand the limits).
59a6a2f
to
cd70bba
Compare
Since this Pull Request has not been updated in 60 days, it has been marked "inactive." This does not mean that it will be closed, though it may be moved to a "Draft" state. This helps maintainers prioritize their reviewing efforts. You can pick the PR back up anytime - please ping us if you need a review or guidance to move the PR forward! If you do not plan on continuing the work, please let us know so that we can either find someone to take the PR over, or close it. |
Let's keep this around until #14898 is fixed. |
x_trf goes from rawdata-space to scaled-space, so it's what should get
applied to datalims, not x_trf.inverted(). So
from 87c742b should have been
Edit: This is getting fixed in #14990, what remains is possibly a revisit of the choice of axis to resize, described below.
However, fixing that triggered a failure for
test_aspect_nonlinear_adjustable_datalim
which had been added in that commit, and fixing that unraveled more
issues.
The basic question is, when aspect is set and adjustable="datalim",
should we change the x limits or the y limits to get the correct aspect?
The old code used some complex conditions, which I actually haven't
managed to fully understand, to either expand or shrink one of the
axises. Instead, just choose to always expand (rather than shrink) one
of the axises, which will avoid sending artists out-of-bounds. (The
sole exception is in care of shared axes, which we do not touch as
explained in the comment.)
This patch caused a change in the autolimiting of
test_axes.py::test_pie_frame_grid which was buggy anyways, I forced the
old behavior by setting x/ylims manually (after checking that the
default is to expand the limits).
Closes #14898.
PR Summary
PR Checklist