Deprecate smart_bounds handling in Axis and Spine #11004
Conversation
|
Try panning around the Axes in that example; it does not work the same with and without smart bounds. |
|
If set_smart_bounds stays it’d be nice if the docstring was a little more informative as to what a smart bound is. |
|
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 |
|
@QuLogic, does the smart_bounds behavior add any real value? If so, what is the desired behavior that it uniquely provides? |
|
I don't have any particular attachment to it; just pointing out that it is not at all the same as |
|
@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() |
|
@astraw, would you explain the use case for smart_bounds, please? |
|
Seems cute, but I don't see what the data analysis/presentation use-case might be. |
|
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. |
|
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. |
|
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.) |
|
@astraw, I appreciate your responding and don't want to burden you. The history as transferred from svn to github includes the following. |
|
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. |
e8d9850 to
058288a
Compare
|
Rebased and updated. |
|
This needs a few test images updated... |
|
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. |
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 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. |
|
@efiring, why don't we trim the ticks in 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) |
|
I think this was superceded by #12158. Closing, but please do re-open if I'm mistaken... |
|
No, I think this is mostly unrelated to #12158. |
|
Sorry... OK to punt it to 3.2 or are you hoping to come back to it? |
|
Punted. |
860831c to
ec1c382
Compare
|
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. |
|
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. |
|
You put an entry in |
|
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. |
|
Could you give us some context about how your are using smart bounds? |
|
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. |
|
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. |
|
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. |
PR Summary
Code related to spines, ticks, and autoscaling needs some simple cleanups, which might be followed by some deeper changes. To begin:
Axis._update_tickswith a 'renderer' argument; it doesn't use it.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