Only autoscale_view() when needed, not after every plotting call. #13593
Conversation
b476632 to
c732394
Compare
| @@ -54,6 +54,7 @@ def test_noise(): | |||
| fig, ax = plt.subplots() | |||
| p1 = ax.plot(x, solid_joinstyle='round', linewidth=2.0) | |||
|
|
|||
| fig.canvas.draw() | |||
There was a problem hiding this comment.
Should these get a comment why we need to draw?
There was a problem hiding this comment.
You've added the comment to another draw 😄. There are three of them in the PR.
Each of them is before a get_path() would it be feasible/make sense to _request_autoscale_view() in this method?
There was a problem hiding this comment.
They've all been commented now.
What really matters is to have it before get_transform(), but IIRC(?) we can't always update there (because the point is to keep TransformedBboxes computed from get_transform() well, not updated until we perform all updates at the same time in a single shot).
| if vmin == vmax: | ||
| vmin = _decade_less(vmin, self._base) | ||
| vmax = _decade_greater(vmax, self._base) | ||
| vmin, vmax = 1, 10 |
There was a problem hiding this comment.
Is it intended that this might be swapped depending on swap?
There was a problem hiding this comment.
Yes (at least it's consistent with the base Locator.nonsingular).
| @@ -4832,9 +4832,7 @@ def hexbin(self, x, y, C=None, gridsize=100, bins=None, | |||
|
|
|||
| corners = ((xmin, ymin), (xmax, ymax)) | |||
| self.update_datalim(corners) | |||
| collection.sticky_edges.x[:] = [xmin, xmax] | |||
There was a problem hiding this comment.
I don't understand what change caused this to start working.
There was a problem hiding this comment.
To be honest I don't know either. I do have some other plans to fiddle with hexbin though (but that won't be in this PR).
a6445a5 to
23fa1a9
Compare
This avoids quadratic complexity when accumulating sticky edges. Mostly, this just replaces autoscale_view() calls with setting a flag requesting that autoscale_view() be called the next time viewLim is accessed. Note that we cannot just do this in draw as this would break common idioms like ``` ax.plot(...) ax.set_xlim(0, None) # keep top limit to what plot() set ``` The main nontrivial changes are - Removal of sticky_edges from hexbin(): Previously, hexbin() actually did *not* respect the sticky_egdes settings for some reason (this can be checked visually); but with this patch it would respect them -- breaking the baseline images. So just don't set sticky_edges instead. - Making LinearLocator.numticks a property: Previously, some code using LinearLocator would only work after the locator has been used once, so that tick_values properly set numticks to not-None; but with this patch, tick_values is no longer called early enough; making numticks a property fixes that. Note that LinearLocator is likely extremely rarely used anyways... - In test_bbox_inches_tight (which uses the old "round_numbers" autolimits mode), the autolimits change depending on whether autoscaling happens before the call to `xticks([])` (old behavior) or after (because there's no notion of "round numbers" anymore. Here we can just force these limits. - test_multi_color_hatch relied on ax.bar() triggering an autoscale but ax.add_patch *not* doing so. Just disable autoscaling then. This patch also prepares towards fixing collections autoscaling problems when switching from linear to log scales (as that also needs to be deferred to as late as possible, once the scale is actually known).
| In some cases, this can result in different limits being reported. If this is | ||
| an issue, consider triggering a draw with `fig.canvas.draw`. | ||
|
|
||
| LogLocator.nonsingular now maintains the orders of its arguments |
There was a problem hiding this comment.
I think everything looks fine in this PR; I'm confused about this and changes to ticker.py though, are they unrelated or do they need to go in tandem with the autoscaling changes?
There was a problem hiding this comment.
They need to go with it; it's all explained in the first message (also the commit message).
|
I reckon 3 approvals is good enough, especially since it's early in the |
Closes the first half of #7413 (you may want to read that first). Closes #12542.
This avoids quadratic complexity when accumulating sticky edges.
Mostly, this just replaces autoscale_view() calls with setting a flag
requesting that autoscale_view() be called the next time viewLim is
accessed. Note that we cannot just do this in draw as this would break
common idioms like
The main nontrivial changes are
did not respect the sticky_egdes settings for some reason (this can
be checked visually); but with this patch it would respect them --
breaking the baseline images. So just don't set sticky_edges instead.
LinearLocator would only work after the locator has been used once,
so that tick_values properly set numticks to not-None; but with this
patch, tick_values is no longer called early enough; making numticks
a property fixes that. Note that LinearLocator is likely extremely
rarely used anyways...
autolimits mode), the autolimits change depending on whether
autoscaling happens before the call to
xticks([])(old behavior) orafter (because there's no notion of "round numbers" anymore. Here we
can just force these limits.
ax.add_patch not doing so. Just disable autoscaling then.
This patch also prepares towards fixing collections autoscaling problems
when switching from linear to log scales (as that also needs to be
deferred to as late as possible, once the scale is actually known).
(i.e., work towards the second half of #7413, not that I have a complete
plan yet.)
On the example in #12542, this version is ~35% faster than on 1.5.3 (which is the last version before quadratic behavior was introduced). Obviously a lot of other things changed since then, though.
PR Summary
PR Checklist