Skip to content

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Jul 27, 2019

x_trf goes from rawdata-space to scaled-space, so it's what should get
applied to datalims, not x_trf.inverted(). So

        x0, x1 = map(x_trf.inverted().transform, dL.intervalx)
        y0, y1 = map(y_trf.inverted().transform, dL.intervaly)

from 87c742b should have been

        x0, x1 = map(x_trf.transform, dL.intervalx)
        y0, y1 = map(y_trf.transform, dL.intervaly)

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

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

@anntzer anntzer force-pushed the apply_aspect_transform branch 2 times, most recently from 33a4d79 to 59a6a2f Compare July 28, 2019 21:44
@QuLogic QuLogic added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Jul 29, 2019
@QuLogic QuLogic requested a review from efiring July 29, 2019 06:08
@efiring
Copy link
Member

efiring commented Jul 29, 2019

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?

@anntzer
Copy link
Contributor Author

anntzer commented Jul 29, 2019

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
plot([1, 2]); margins(0); gca().set(aspect=1, adjustable="datalim")
if you manually lower the window' width to less than its height and then back up again, then the y limits get a 0.05 margin rather than the original 0. (Admittedly this is better than what this patch does, which is to make the limits wider and wider without ever going back.)
Finally, what do you propose to do with #14898?

Perhaps the full solution is to integrate limits adjustment for aspect application together with autoscaling, but that's more surgery...

@anntzer anntzer added this to the v3.2.0 milestone Aug 5, 2019
@efiring
Copy link
Member

efiring commented Aug 5, 2019

@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)))

@efiring
Copy link
Member

efiring commented Aug 5, 2019

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.

@anntzer
Copy link
Contributor Author

anntzer commented Aug 6, 2019

Thanks for the commented algorithm :)

The new algorithm is nonsensical unless the same transform is used for both axes, isn't it?

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.

Actually, I'm not even sure the latter [logit] makes sense unless both axes are somehow pinned such that their x and y spans are symmetric about zero.

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?

@efiring
Copy link
Member

efiring commented Aug 6, 2019

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?

@anntzer
Copy link
Contributor Author

anntzer commented Aug 6, 2019

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.)

test

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.

@efiring
Copy link
Member

efiring commented Aug 6, 2019

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.

@efiring
Copy link
Member

efiring commented Aug 6, 2019

Also, in a stage-2 cleanup the margins need a little more thought.

@anntzer anntzer removed the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Aug 6, 2019
@anntzer
Copy link
Contributor Author

anntzer commented Aug 6, 2019

the release critical part is in #14990.

@anntzer anntzer modified the milestones: v3.2.0, v3.3.0 Aug 6, 2019
@anntzer anntzer added the status: needs comment/discussion needs consensus on next step label Aug 8, 2019
@QuLogic QuLogic modified the milestones: v3.3.0, v3.4.0 May 2, 2020
@jklymak jklymak marked this pull request as draft September 10, 2020 15:34
@QuLogic QuLogic modified the milestones: v3.4.0, v3.5.0 Jan 21, 2021
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).
@anntzer anntzer force-pushed the apply_aspect_transform branch from 59a6a2f to cd70bba Compare May 20, 2021 20:46
@anntzer anntzer changed the title Fix incorrect transform in apply_aspect. Changed axis selection when zooming datalim-adjustable fixed-aspect axes May 20, 2021
@QuLogic QuLogic modified the milestones: v3.5.0, v3.6.0 Aug 23, 2021
@timhoffm timhoffm modified the milestones: v3.6.0, unassigned Apr 30, 2022
@story645 story645 modified the milestones: unassigned, needs sorting Oct 6, 2022
@github-actions
Copy link

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.

@github-actions github-actions bot added the status: inactive Marked by the “Stale” Github Action label Jun 23, 2023
@anntzer
Copy link
Contributor Author

anntzer commented Jun 23, 2023

Let's keep this around until #14898 is fixed.

@anntzer anntzer added the keep Items to be ignored by the “Stale” Github Action label Jun 23, 2023
@rcomer rcomer removed the status: inactive Marked by the “Stale” Github Action label Jun 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
keep Items to be ignored by the “Stale” Github Action status: needs comment/discussion needs consensus on next step status: needs rebase status: needs revision
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"round_numbers" axis limits + axis("equal") sometimes sends artists out of axes limits.
6 participants