Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Only autoscale_view() when needed, not after every plotting call.#13593
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
b476632 toc732394Compare| @@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should these get a comment why we need to draw?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
added comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You've added the comment to another draw 😄. There are three of them in the PR.
Each of them is before aget_path() would it be feasible/make sense to_request_autoscale_view() in this method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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).
Uh oh!
There was an error while loading.Please reload this page.
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Is it intended that this might be swapped depending onswap?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't understand what change caused this tostart working.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 to23fa1a9CompareThis avoids quadratic complexity when accumulating sticky edges.Mostly, this just replaces autoscale_view() calls with setting a flagrequesting that autoscale_view() be called the next time viewLim isaccessed. Note that we cannot just do this in draw as this would breakcommon 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 problemswhen switching from linear to log scales (as that also needs to bedeferred to as late as possible, once the scale is actually known).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
subject to CI pass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM I won't merge yet as this seems kind of a major change and should get executive approval...
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think everything looks fine in this PR; I'm confused about this and changes toticker.py though, are they unrelated or do they need to go in tandem with the autoscaling changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 |
Uh oh!
There was an error while loading.Please reload this page.
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
didnot 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_patchnot 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