Skip to content

Deprecate smart_bounds handling in Axis and Spine #11004

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

Merged
merged 2 commits into from
Jul 17, 2019

Conversation

efiring
Copy link
Member

@efiring efiring commented Apr 9, 2018

PR Summary

Code related to spines, ticks, and autoscaling needs some simple cleanups, which might be followed by some deeper changes. To begin:

  • Tighten the tolerance for floating point error in deciding which ticks to keep. Prior to this PR it was supposed to be 0.5 pixel, but I don't think this makes sense; it makes the selection backend-dependent, and it is vastly larger than it needs to be to compensate for floating point error.
  • Stop calling Axis._update_ticks with a 'renderer' argument; it doesn't use it.
  • Deprecate all "smart_bounds" handling in Axis and Spine. It modifies the spine behavior by making the spines extend only to the data limits.

Edited, 2019-07-16: Everything but the last item has been superceded by other PRs, and therefore has been stripped out of this one.

PR Checklist

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

@QuLogic
Copy link
Member

QuLogic commented Apr 9, 2018

Try panning around the Axes in that example; it does not work the same with and without smart bounds.

@jklymak
Copy link
Member

jklymak commented Apr 9, 2018

If set_smart_bounds stays it’d be nice if the docstring was a little more informative as to what a smart bound is.

@efiring
Copy link
Member Author

efiring commented Apr 9, 2018

I've only looked at one failing test, but I suspect it will be characteristic. I think the problem is with the old behavior, not the new behavior, but this can be discussed. In specgram_noise, the actual xlims are
(0.20000000000000001, 199.69999999999999).
The old behavior gives a tick at zero but not at 200; the new behavior omits both ticks.

@efiring
Copy link
Member Author

efiring commented Apr 9, 2018

@QuLogic, does the smart_bounds behavior add any real value? If so, what is the desired behavior that it uniquely provides?

@QuLogic
Copy link
Member

QuLogic commented Apr 9, 2018

I don't have any particular attachment to it; just pointing out that it is not at all the same as Axes.autoscale_view.

@efiring
Copy link
Member Author

efiring commented Apr 9, 2018

@QuLogic You are right, of course; I was not looking carefully at what the smart_bounds was doing. The big effect is that it keeps the spines locked to the data as the data are moved, so that the spine spans the part of the data range that fits within the xlim and ylim instead of spanning those limits as they are changed. If anyone wants to test this, the modified excerpt from spine_placement_demo.py will serve:

import numpy as np
import matplotlib.pyplot as plt

plt.rcParams['axes.autolimit_mode'] = 'round_numbers'
plt.rcParams['axes.xmargin'] = 0
plt.rcParams['axes.ymargin'] = 0

for smart in (True, False):
    fig, ax = plt.subplots()
    fig.suptitle('smart is %s' % smart)
    x = np.linspace(-np.pi, np.pi, 100)
    y = 2 * np.sin(x)
    ax.set_title('centered spines')
    ax.plot(x, y)
    ax.spines['left'].set_position('center')
    ax.spines['right'].set_color('none')
    ax.spines['bottom'].set_position('center')
    ax.spines['top'].set_color('none')
    ax.spines['left'].set_smart_bounds(smart)
    ax.spines['bottom'].set_smart_bounds(smart)
    ax.xaxis.set_ticks_position('bottom')
    ax.yaxis.set_ticks_position('left')
plt.show()

@efiring
Copy link
Member Author

efiring commented Apr 9, 2018

@astraw, would you explain the use case for smart_bounds, please?

@jklymak
Copy link
Member

jklymak commented Apr 9, 2018

Seems cute, but I don't see what the data analysis/presentation use-case might be.

@astraw
Copy link
Contributor

astraw commented Apr 9, 2018

I wrote the original "dropped spine" implementation (which John Hunter noted sounded like a medical condition) and this looks like the translated-to-git commit: 43ca13b. The word "smart" is not in that commit nor do I remember any smart bounds feature. So without doing further spelunking, I cannot offer any support of smart_bounds. If you are in favor of removing in your cleanup, please go ahead.

@QuLogic
Copy link
Member

QuLogic commented Apr 10, 2018

There is an example with dropped spines and it does not use this smart bounds feature, so I'm not sure how they were related.

@astraw
Copy link
Contributor

astraw commented Apr 10, 2018

In my original dropped spines implementation I wrote an example which showed the various different interaction modes which I tested with. This was the primary test case that I developed the code for. If that still runs without problems and with each axes behaving differently (there was no redundancy), then all my original use cases are accounted for. (I'm super busy this moment and haven't looked to check what has become of that example.)

@efiring
Copy link
Member Author

efiring commented Apr 10, 2018

@astraw, I appreciate your responding and don't want to burden you. The history as transferred from svn to github includes the following.
You added spines in what is now 43ca13b, and that included the spine_placement_demo.py that we still use: https://matplotlib.org/gallery/ticks_and_spines/spine_placement_demo. You added the smart bounds in a5761e8, including turning them on in the demo. (They are off by default.)
I don't know what the case was originally, but now the main difference the smart bounds make appears only when one zooms or pans. In the example I give in an earlier comment the plots are initially identical with and without the smart bounds. When the example is modified by removing the rcParams entries (which I put in to give a more "classic" autoscaling behavior) the smart bounds make a slight difference: the spines end exactly at the data limits instead of extending to the view limits, which have been expanded by the margins--a relatively new feature.

@astraw
Copy link
Contributor

astraw commented Apr 10, 2018

Thanks for the summary. What you write is consistent with my memory in that I did spend some effort in tuning interactive behavior and also tuning the length of the spines to end at data limits. So if "smart bounds" is indeed the name of the feature to cause the spines to end at the data limits, I would say this is a useful feature, and not just for interactive mode. In any case, I haven't followed MPL development closely enough to have an informed opinion about this and would defer to the devs' preference on this. I certainly can understand if a simple code base is seen to be more important than obscure features.

@efiring
Copy link
Member Author

efiring commented Sep 4, 2018

Rebased and updated.
I think that this can go in as-is, provided there is agreement about deprecating the "smart bounds".

@jklymak
Copy link
Member

jklymak commented Sep 11, 2018

This needs a few test images updated...

@efiring
Copy link
Member Author

efiring commented Sep 11, 2018

Thanks. I had it all staged, just needed to commit and push. Unfortunately, I think these test images are much larger than they need to be for their real purpose. They all lose an initial tick with this PR because their first x data point is a small positive number, not zero.

@jklymak jklymak self-requested a review September 11, 2018 00:43
@tacaswell tacaswell added this to the v3.1 milestone Sep 11, 2018
@anntzer
Copy link
Contributor

anntzer commented Sep 11, 2018

Tighten the tolerance for floating point error in deciding which ticks to keep. Prior to this PR it was supposed to be 0.5 pixel, but I don't think this makes sense; it makes the selection backend-dependent, and it is vastly larger than it needs to be to compensate for floating point error.

Is that clear? <0.5px makes sense in that it means that if e.g. you draw without antialiasing, then the tick will be aligned with the edge (well... perhaps, depending on where you fall exactly with the pixel edges).

In general shouldn't this really (as usual) be handled by the locators deciding exactly what ticks to show? (Per the argument made again and again...) The locators already have their own tolerance mechanism for floating point error, cf the Base class that you fiddled with as well IIRC. Also I did some quick profiling a while ago and IIRC this _get_pixel_distance_along_axis is actually contributing >~1% to the total runtime, due to the need to construct, well, a bunch of transforms along the way. So it'd be nice to see it go away.

Note that I'm not blocking this PR on that change, I guess it's still an improvement, just making sure I understand what's going on.

@jklymak
Copy link
Member

jklymak commented Sep 11, 2018

@efiring, why don't we trim the ticks in tick_values? If I add the code below the results seem good to me. A bunch of ticker tests fail, but thats because they were returning ticks that are out of bounds.

def tick_values(self, vmin, vmax):
        if self._symmetric:
            vmax = max(abs(vmin), abs(vmax))
            vmin = -vmax
        vmin, vmax = mtransforms.nonsingular(
            vmin, vmax, expander=1e-13, tiny=1e-14)
        locs = self._raw_ticks(vmin, vmax)
        # make sure ticks are inside vmin, vmax within numerical tolerance:
        rtol = np.abs(vmax - vmin) * 1e-12
        locs = locs[locs < vmax + rtol]
        locs = locs[locs > vmin - rtol]

        prune = self._prune
        if prune == 'lower':
            locs = locs[1:]
        elif prune == 'upper':
            locs = locs[:-1]
        elif prune == 'both':
            locs = locs[1:-1]
        return self.raise_if_exceeds(locs)

@jklymak
Copy link
Member

jklymak commented Feb 26, 2019

I think this was superceded by #12158. Closing, but please do re-open if I'm mistaken...

@jklymak jklymak closed this Feb 26, 2019
@efiring
Copy link
Member Author

efiring commented Feb 26, 2019

No, I think this is mostly unrelated to #12158.

@efiring efiring reopened this Feb 26, 2019
@jklymak
Copy link
Member

jklymak commented Feb 26, 2019

Sorry... OK to punt it to 3.2 or are you hoping to come back to it?

@efiring efiring modified the milestones: v3.1.0, v3.2.0 Feb 26, 2019
@efiring
Copy link
Member Author

efiring commented Feb 26, 2019

Punted.

@efiring efiring force-pushed the spine_axis_cleanup branch from 860831c to ec1c382 Compare July 16, 2019 20:50
@efiring
Copy link
Member Author

efiring commented Jul 16, 2019

This still needs an api_changes entry. This PR is highly modified from the original, so that now it's sole purpose is to deprecate the smart_bounds feature. I could turn it into a new PR and simply refer from there to the extensive comments here about that feature.

Copy link
Member

@jklymak jklymak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I'll change the PR title if thats OK...

@jklymak jklymak changed the title Spine and Axis cleanup Deprecate smart_bounds handling in Axis and Spine Jul 16, 2019
@efiring
Copy link
Member Author

efiring commented Jul 16, 2019

How are deprecations added to the "what's new" documentation? Is this done at release time by the release manager? It doesn't appear to be done via adding an rst file to the doc/api/api_changes directory.

@jklymak
Copy link
Member

jklymak commented Jul 16, 2019

You put an entry in doc/api/next_api_changes

@tacaswell tacaswell merged commit b2c7190 into matplotlib:master Jul 17, 2019
@efiring efiring deleted the spine_axis_cleanup branch July 17, 2019 03:11
@moorepants
Copy link

I just wanted to note that the plotting module in SymPy relies on these smart_bounds calls and we will need an alternative if they are removed from matplotlib.

@tacaswell
Copy link
Member

Could you give us some context about how your are using smart bounds?

@efiring
Copy link
Member Author

efiring commented Apr 2, 2020

From the SymPy 18870 referenced above, it appears that there is exactly one line that was setting smart_bounds for a spine to True, hence activating that logic path. Based on the change in plots in https://docs.sympy.org/dev/modules/plotting.html, it appears that this was somehow controlling the ylim. I don't know why this can't be done either with normal mpl autoscaling or by explicitly setting ylim as desired within SymPy.

@moorepants
Copy link

I've dug a little deeper and I don't think the smart_bounds was the direct cause of the output changes in SymPy's plotting functions. I've opened #17004 that points to a regression in mpl 3.2 from 3.1 that is causing the effect.

@moorepants
Copy link

Given that, we will need a workaround for the removal of smart_bounds in SymPy. Trying to figure out what that was is clouded by the possible unrelated regression.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants