Turn autoscale into a contextmanager. #5538

anntzer · 2015-11-21T05:01:24Z

Also clarify a bit the implementation of autoscale, and fix an issue about the
lack of effect of set_autoscalez_on for 3D axes, which was previously visible
by running::

from pylab import *
from mpl_toolkits import mplot3d

gcf().add_subplot(111, projection="3d")
plot([0, 1], [0, 1], [0, 1])
gca().set_autoscalex_on(False)
gca().set_autoscalez_on(False) # has no effect
plot([2, 3], [2, 3], [2, 3])
show()

tacaswell · 2015-11-22T18:34:49Z

lib/matplotlib/axes/_base.py

+
+        or::
+
+            @axes.autoscale(enable)


I would not suggest this in the documentation as this binds the definition of this function to a specific instance.

tacaswell · 2015-11-22T18:37:34Z

This will need documentation in both whats_new and api_changes I think.

tacaswell · 2015-11-22T18:47:36Z

Can you also add a test of the context manager?

anntzer · 2015-11-22T22:52:25Z

A quick grep suggests that there isn't even any testing done for the regular autoscale function (or am I missing something?). If someone writes one I could build on top of it...

tacaswell · 2015-11-22T23:07:55Z

Then you can kill two birds with one stone!

This doesn't need to be image tests, you can just check what the limits are.

On Sun, Nov 22, 2015, 17:52 Antony Lee notifications@github.com wrote:

A quick grep suggests that there isn't even any testing done for the
regular autoscale function (or am I missing something?). If someone
writes one I could build on top of it...

—
Reply to this email directly or view it on GitHub
#5538 (comment)
.

anntzer · 2015-11-23T01:58:46Z

Does this look good enough?

mdboom · 2015-11-23T13:40:46Z

👍 from me, but I agree with @tacaswell that the decorator approach, being tied to a specific Axes object, shouldn't be recommended.

Looks like this needs a rebase.

See matplotlib#5510. Also clarify a bit the implementation of autoscale, and fix an issue about the lack of effect of set_autoscalez_on for 3D axes, which was previously visible by running:: from pylab import * from mpl_toolkits import mplot3d gcf().add_subplot(111, projection="3d") plot([0, 1], [0, 1], [0, 1]) gca().set_autoscalex_on(False) gca().set_autoscalez_on(False) # has no effect plot([2, 3], [2, 3], [2, 3]) show()

anntzer · 2015-11-23T15:48:48Z

Done.

WeatherGod · 2015-11-23T15:58:16Z

lib/matplotlib/axes/_base.py

+            try:
+                yield
+            finally:
+                self._autoscaleXon, self._autoscaleYon = orig_autoscale


should we mark the axes object as stale?

(I have no idea of what you're talking about so I'll leave the discussion to the specialists.)

I don't think this needs to set stale because changing this internal state does not invalidate the drawn figure (that is the next call to draw will not be different)....unless I am miss understanding how this works.

tacaswell · 2015-11-23T19:39:28Z

For future reference, please see the directions in the readme at https://github.com/matplotlib/matplotlib/tree/master/doc/users/whats_new . By putting the whats_new / api-changes into separate files we avoid a lot of un-needed rebases.

tacaswell · 2015-11-24T02:52:49Z

lib/matplotlib/axes/_base.py

            if axis in ['y', 'both']:
                self._autoscaleYon = bool(enable)
-                scaley = self._autoscaleYon
-        self.autoscale_view(tight=tight, scalex=scalex, scaley=scaley)
+        self.autoscale_view(


This does not behave the same if enable is None. Previously, ax.autoscale(None) would autoscale both axis, but leave the _autoscale*on state unchanged, now it scale the axis dependent on the current values of _autoscale*on.

OK, so I misunderstood the code (I never used autoscale(None) so I had no idea what it did). I still don't understand the actual behavior: autoscaling (in autoscale_view) is protected by the conditional if scalex and self._autoscaleXon, so I thought that just passing scalex=True would not perform an autoscale as long as self._autoscaleXon is False.
Where did I go wrong?

I was just reading this code locally and sorting through what inputs are going to the autoscale_view, you are correct this does not actually change the behaviour. Sorry for the noise.

anntzer · 2016-04-07T02:35:06Z

I don't really know what to do with this PR: on the one hand it's in a working state, on the other hand I feel like there should be a better model for autoscaling: rather than being a switch on the axes, I'd rather it be a switch on the artists themselves, marking, for each artist, whether it should be considered for autoscaling computations. This would allow dynamically adding artists in whatever order is convenient while maintaining fine control over autoscaling.

Thoughts?

tacaswell · 2016-04-07T05:30:34Z

There is some recent precedence for that sort of thing as artist can report if they want tight bounds or note in the auto scaling. Just not participating seems like a reasonable option to add.

anntzer · 2016-04-07T05:53:54Z

Uh, through what mechanism?

Also, looking at it again, it's going to be a bit awkward to keep backcompatibility, as usual... This is how I envision it:
There's a field participates_in_autoscale in each artist, defaulting to True. When relim is called, only artists that want to participate are taken into account. Calling autoscale(False) switches the default value to False. The problem is that from a strict backcompatibility POV, calling autoscale(True) should not only revert the default value back to True, but also switch the value for all other artists back to True too. While this is technically not too hard (e.g. there could be a third value, None, which just means "use the default value"), this would kind of defeat the purpose of it. More likely, you need either to change the behavior of autoscale(True), or deprecate it and have another switcher which simply sets the default value of the flag without modifying its value for already created artists.
Thoughts?

tacaswell · 2016-04-07T05:58:24Z

See #5583 for the margin machinery.

anntzer · 2016-04-07T07:20:24Z

OK, so I guess I should replace the values of the margin attribute by a tristate "ignore/no_margin/want_margin"? Also I feel like this attribute should be private...
Any thoughts re. backwards compatibility?

tacaswell · 2016-09-24T19:52:48Z

doc/api/api_changes.rst

+Changes in 2.1.0
+================
+
+Code changes


Can you put these is a file in api_changes/*.rst. This helps to prevent the sort of conflicts this branch has.

I'd rather close this PR and make margin a tristate as discussed above. In fact, making autoscale a contextmanager is a bit misleading given that at the exit of the contextmanager (assuming it temporarily disabled autoscaling), all the artists, including those created while the cm was active, will participate in the next autoscaling... which defeats the purpose of the cm IMO. Thoughts?

The fix for z autoscaling should go in its own PR.

@anntzer That sounds like a good plan to me.

Wish we were 3 only and could use enums 😞

In this case I think the best is to use strings everywhere (IMO the problem of using, say, True, False and a third value is that it is too easy to accidentally test for the boolean value and forget about the third possibility). Say "default" (current True), "remove" (current False) and "ignore"?

attn @mdboom as the author of #5583.

anntzer · 2017-04-16T06:23:16Z

This can't really be made to work with the current machinery. What would be needed is to union the bboxes at draw time, while checking for a hypothetical "participates_in_autoscale" flag. See #7413 (which is not strictly needed, but related).

tacaswell added the status: needs review label Nov 21, 2015

tacaswell reviewed Nov 22, 2015
View reviewed changes

tacaswell added this to the proposed next point release (2.1) milestone Nov 22, 2015

anntzer added 2 commits November 23, 2015 07:38

Add basic test for switching autoscale status.

4f30cce

anntzer force-pushed the autoscale-cm branch from 6df9101 to 90397f9 Compare November 23, 2015 15:48

Documentation for the autoscale contextmanager.

90397f9

WeatherGod reviewed Nov 23, 2015
View reviewed changes

tacaswell reviewed Nov 24, 2015
View reviewed changes

tacaswell added status: needs revision and removed status: needs review labels Nov 24, 2015

WIP: Fix autoscale(None)?

498166d

anntzer mentioned this pull request Jun 12, 2016

Questionable margin-cancellation logic #6565

Closed

tacaswell reviewed Sep 24, 2016

View reviewed changes

anntzer mentioned this pull request Sep 25, 2016

new margin system doesn't handle negative values in bars #7175

Closed

anntzer mentioned this pull request Jan 17, 2017

Fix typo in Axes3D.set_autoscalez_on. #7859

Merged

anntzer closed this Apr 16, 2017

anntzer mentioned this pull request Apr 16, 2017

autoscale context manager #5510

Closed

anntzer deleted the autoscale-cm branch June 23, 2017 09:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Turn autoscale into a contextmanager. #5538

Turn autoscale into a contextmanager. #5538

anntzer commented Nov 21, 2015

tacaswell Nov 22, 2015

tacaswell commented Nov 22, 2015

tacaswell commented Nov 22, 2015

anntzer commented Nov 22, 2015

tacaswell commented Nov 22, 2015

anntzer commented Nov 23, 2015

mdboom commented Nov 23, 2015

anntzer commented Nov 23, 2015

WeatherGod Nov 23, 2015

anntzer Nov 23, 2015

tacaswell Nov 23, 2015

tacaswell commented Nov 23, 2015

tacaswell Nov 24, 2015

anntzer Nov 24, 2015

tacaswell Nov 30, 2015

anntzer commented Apr 7, 2016

tacaswell commented Apr 7, 2016

anntzer commented Apr 7, 2016

tacaswell commented Apr 7, 2016

anntzer commented Apr 7, 2016

tacaswell Sep 24, 2016

anntzer Sep 24, 2016

tacaswell Sep 24, 2016

anntzer Sep 24, 2016 •

edited

Loading

anntzer commented Apr 16, 2017

Turn autoscale into a contextmanager. #5538

Turn autoscale into a contextmanager. #5538

Conversation

anntzer commented Nov 21, 2015

Choose a reason for hiding this comment

tacaswell commented Nov 22, 2015

tacaswell commented Nov 22, 2015

anntzer commented Nov 22, 2015

tacaswell commented Nov 22, 2015

anntzer commented Nov 23, 2015

mdboom commented Nov 23, 2015

anntzer commented Nov 23, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tacaswell commented Nov 23, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anntzer commented Apr 7, 2016

tacaswell commented Apr 7, 2016

anntzer commented Apr 7, 2016

tacaswell commented Apr 7, 2016

anntzer commented Apr 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anntzer Sep 24, 2016 • edited Loading

Choose a reason for hiding this comment

anntzer commented Apr 16, 2017

anntzer Sep 24, 2016 •

edited

Loading