ENH: Switch to a private, simpler AxesStack. #7377

anntzer · 2016-11-02T01:00:51Z

The current implementation of AxesStack subclasses cbook.Stack, which
requires hashable keys, which leads to additional complexity on the
caller's side (_make_key).

Instead, switch to using two lists (keys and axes) and relying on
list.index, which makes the implementation much simpler.

Also make the new class private and deprecate the previous one.

(The original idea was to make AxesStack private to avoid having to deal with issues such as #7194. I originally thought that we could just replace it by an OrderedDict, which is still an option, but you need a really need a two-way dict to implement remove-by-value (instead of by key), so I ended up dropping that idea -- the benefit of dropping _make_key looked more enticing.)

NelleV · 2016-11-02T01:48:19Z

If you want a simpler PR at first, you can make one that deprecates the public AxesStack and "just" creates the private one. We can go from there in simplifing the API.
(else you'll have to fix the failing tests :) )

I'd wait on having the deprecation of the AxesStack vetted by others before moving forward. I think make AxesStack private is a good idea: it is only used in the figure object right now, in a private attribute, and probably aimed at being private from the start.
attn @efiring @tacaswell

tacaswell · 2016-11-02T01:11:02Z

lib/matplotlib/figure.py

@@ -157,6 +158,60 @@ def __contains__(self, a):
        return a in self.as_list()


+class _AxesStack:


needs to inherit from (object) (for now).

tacaswell · 2016-11-02T04:15:39Z

👍 in principle but we should try to support as much of the old API as possible.

I know I have written SO answers which reached in and touched the AxesStack....

The balance against making everything private is that if you do not give users enough public knobs to get done what they need to get done, they will find the private knobs and just do it. Once enough user are using a private API it become defacto public and we are as bad off or worse than we would have been other wise.

anntzer · 2016-11-02T05:40:59Z

@NelleV Hopefully everything should be fixed now (I didn't realize that I need to keep track both of the axes stack and of the order in which the axes were created).

@tacaswell The only reference to AxesStack I could find on SO is http://stackoverflow.com/questions/24104990/how-to-get-a-list-of-axes-for-a-figure-in-pyplot which somehow ironically ends with your comment

fig is a Figure object. I am not sure where AxesStack gets used, but you should probably not be using it.

In any case this PR literally doesn't change any public API -- the old AxesStack class is still there, it's just used nowhere :-) (and 1. the AxesStack object instantiated by the Figure class was a private attribute, so it's fair game to change it and 2. in fact, the new private AxesStack implements (nearly) the same API as the old one...).

Honestly if we start having to worry about modifying private APIs then I don't know what's left to do other than adding features over features and never fixing old stuff because, well, it may always break someone else who is playing with the internals (that person should have asked for a public API, or known better). Again, I'm not removing AxesStack for now; if someone complains that they were actually using it, we can always consider undeprecating it.

NelleV · 2016-11-10T17:25:54Z

The code looks good to me.
I don't think we loose any functionality with this switch and the code is much cleaner.

NelleV · 2016-11-10T17:27:09Z

lib/matplotlib/figure.py

+        return next((ax for _, k, ax in self._items if k == key), None)
+
+    def current_key_axes(self):
+        """Return the topmost key, axes pair, or `None, None` if empty.


topmost? Is that a word I don't know?

http://www.merriam-webster.com/dictionary/topmost

Should add some parentheses because right now it can read as either "a key" or "an axes pair" or None, None, but it's really "(key, axes) pair" or None, None.

good point, fixed

efiring · 2016-11-10T23:51:12Z

Also related to #7335.

QuLogic

Maybe a silly question, but is the only difference between this and an OrderedDict that it can do operations based on the value instead of the key? A comment/doc to that effect would be useful for the future.

QuLogic · 2016-11-11T00:27:44Z

lib/matplotlib/figure.py

+        return key, ax
+
+    def add(self, key, ax):
+        """Append a key, axes pair, unless the axes are already present.


Add parentheses on this one too.

QuLogic · 2016-11-11T00:31:12Z

lib/matplotlib/figure.py

+    def as_list(self):
+        """Copy of the list of axes, in the order of insertion.
+        """
+        return [ax for _, _, ax in sorted(self._items)]


I know it's originally sorted this way, but I wonder if that's even necessary...

The "keep creation order" behavior is mandated by the tests right now, and I don't see a reason to get rid of it.

anntzer · 2016-11-11T01:35:41Z

@QuLogic also because keys may not be hashable. Added a comment to that effect.

tacaswell · 2016-11-11T02:13:32Z

@anntzer It as always fun when 2.5 year old comments come back at me ;)

NelleV · 2016-11-11T02:15:46Z

This PR clearly supersedes #7335, thought I think the latter is 2.0 safe while this one isn't.

tacaswell · 2016-11-11T02:16:47Z

lib/matplotlib/figure.py

+    def get(self, key):
+        """Find the axes corresponding to a key; defaults to `None`.
+        """
+        return next((ax for _, k, ax in self._items if k == key), None)


@anntzer I consistently learn new python details from your code.

tacaswell · 2016-11-11T02:35:44Z

lib/matplotlib/figure.py

+        """
+        # Skipping existing Axes is needed to support calling `add_axes` with
+        # an already existing Axes.
+        if not any(a == ax for _, _, a in self._items):


This probably needs to handle the duplicate key case?

Each call to add is actually preceded by a call to get, so technically we can rely on the caller code to never try to add in a duplicate key. Also I worded the docstring carefully: the pair is not appended if the axes is already present but there's no check on whether the key is :-)

In practice I could make the behavior (no checking for duplicate key) clearer in the docstring, or add a check and error out if the key is already there; either is fine with me. Please let me know your preference.

tacaswell · 2016-11-11T02:43:35Z

👍 One small concern about the duplicate key, other wise LGTM

I strongly prefer to not backport this for 2.0.

NelleV · 2016-11-11T03:09:42Z

We should never have duplicate keys, so what should be the expected behaviour? Raise an error?

tacaswell · 2016-11-11T03:13:49Z

Lets go with raise (and keep the raise / API change in #7335).

QuLogic · 2016-11-11T03:23:34Z

Hmm, so I just thought of something that the key function actually handles that isn't here, namely, that elements of args and kwargs may be mutable, which may not be something you want in the key. For example:

rect = np.array([0.2, 0.2, 0.2, 0.2])
ax1 = fig.add_axes(rect, ...)
rect[:2] += 0.2
ax2 = fig.add_axes(rect, ...)

ax2 should be new, but I think it will return ax1 with this class.

anntzer · 2016-11-11T05:05:45Z

In fact I just realized there's a much bigger issue here: you can't use == to compare arrays for full equality, so something as simple as

plt.axes(np.array(...)); plt.axes(np.array(...))

will fail with

ValueError: The truth value of an array with more than one element is ambiguous. Use a.any() or a.all()

I guess there's no way around having some variant of make_key :-(

tacaswell · 2016-11-11T17:42:25Z

do we need to support mutable/non-hashable keys?

efiring · 2016-11-11T20:22:42Z

On 2016/11/11 8:23 AM, Antony Lee wrote:

I would like to claim no, of course... in fact I would even like to
claim that the whole

|If the figure already has an axes with the same parameters, then it will
simply make that axes current and return it. |

behavior is seriously misguided.

I suspect we all agree on this. I think it is the result of starting
off following Matlab long ago for the sake of compatibility. Then our
code and documentation get ever more tied in knots as we go beyond
Matlab while trying to maintain compatibility with a bad API. Maybe we
need to be a little more willing to shed such relics. How much user
code is likely to rely on this?

anntzer · 2016-11-11T20:34:38Z

If you're willing to break backcompatibility here, the good thing is that's there's an obvious deprecation path:

merge fix the stack remove error #7335 as a stopgap measure and 2) raise a deprecation warning whenever a key collision occurs
A few (cough... one... cough) releases later,
entirely drop that behavior (i.e. create a new axes every time).

By the way I assume(?) it is possible to change the rectangle where an axes sit, in which case that provides another way to break the current implementation (create an axes, move it somewhere else, and either try to create another axes where the first one was, or try to create another axes where the first one is currently).

In fact even MATLAB doesn't provide that behavior:

>> axes('Position', [.1 .1 .7 .7]); axes('Position', [.1, .1, .7, .7]); get(gcf, 'children')

ans = 

  2×1 Axes array:

  Axes
  Axes

Kojoley · 2016-11-13T12:00:22Z

lib/matplotlib/figure.py

-        return self._axstack.as_list()
-
-    axes = property(fget=_get_axes, doc="Read-only: list of axes in Figure")
+    axes = property(lambda self: self._axstack.as_list(),


Can't this be

@property def axes(self): """Read-only: list of axes in Figure""" return self._axstack.as_list()

I don't think one is more readable that the other?

NelleV · 2017-01-17T18:02:37Z

Is there any interest in pushing this forward?

anntzer · 2017-01-17T18:13:01Z

Why not :-)
Rebased, and updated the deprecation warning to 2.1.

codecov-io · 2017-01-17T20:21:13Z

Current coverage is 62.01% (diff: 97.05%)

Merging #7377 into master will decrease coverage by 0.09%

@@             master      #7377   diff @@
==========================================
  Files           174        174          
  Lines         56065      56068     +3   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits          34820      34770    -50   
- Misses        21245      21298    +53   
  Partials          0          0

Powered by Codecov. Last update fa95b58...9b7bfdc

NelleV

I think this looks good. I'd rather merge this sooner than later, if something is terribly wrong, we might pick it up before the 2.1 release.

anntzer · 2017-01-20T02:11:45Z

Actually, see the discussion starting at #7377 (comment). I think the issue mentioned by @QuLogic cannot really be fixed with the current implementation so perhaps we should instead follow the path mentioned in #7377 (comment) (see comment by @efiring re: backcompatibility, too).

tacaswell · 2017-08-10T03:33:21Z

This needs a rebase.

The current implementation of AxesStack subclasses cbook.Stack, which requires hashable keys, which leads to additional complexity on the caller's side (`_make_key`). Instead, switch to using two lists (keys and axes) and relying on `list.index`, which makes the implementation much simpler. Also make the new class private and deprecate the previous one.

anntzer · 2017-08-10T13:31:34Z

done

QuLogic · 2017-08-13T04:57:01Z

Are we not worried about #7377 (comment)?

tacaswell · 2017-08-14T01:21:38Z

@QuLogic I missed that when I was re-reading the comments last night, sorry.

NelleV self-assigned this Nov 2, 2016

anntzer force-pushed the private-axesstack branch from 3be290e to d48d6cd Compare November 2, 2016 02:30

tacaswell reviewed Nov 2, 2016

View reviewed changes

tacaswell added this to the 2.1 (next point release) milestone Nov 2, 2016

anntzer force-pushed the private-axesstack branch 2 times, most recently from 9f227e3 to acd4bb2 Compare November 2, 2016 05:29

anntzer force-pushed the private-axesstack branch 3 times, most recently from 7eb590d to a288d93 Compare November 2, 2016 08:42

NelleV reviewed Nov 10, 2016

View reviewed changes

anntzer force-pushed the private-axesstack branch from a288d93 to 265626f Compare November 10, 2016 21:37

QuLogic reviewed Nov 11, 2016

View reviewed changes

anntzer force-pushed the private-axesstack branch 2 times, most recently from 4c0ce14 to 1143621 Compare November 11, 2016 01:34

tacaswell reviewed Nov 11, 2016

View reviewed changes

Kojoley reviewed Nov 13, 2016

View reviewed changes

efiring mentioned this pull request Nov 14, 2016

fix the stack remove error #7335

Closed

anntzer force-pushed the private-axesstack branch 2 times, most recently from f21c036 to 9b7bfdc Compare January 17, 2017 18:12

NelleV approved these changes Jan 20, 2017

View reviewed changes

NelleV changed the title ~~Switch to a private, simpler AxesStack.~~ [MRG+1] Switch to a private, simpler AxesStack. Jan 20, 2017

anntzer mentioned this pull request Apr 9, 2017

Allow unhashable keys in AxesStack. #8451

Merged

anntzer force-pushed the private-axesstack branch from 9b7bfdc to 0022611 Compare August 10, 2017 13:31

efiring mentioned this pull request Aug 12, 2017

Axes creation seems to reuse an old one instead of creating a new one #9024

Closed

tacaswell changed the title ~~[MRG+1] Switch to a private, simpler AxesStack.~~ ENH: Switch to a private, simpler AxesStack. Aug 13, 2017

tacaswell merged commit 4c33d97 into matplotlib:master Aug 13, 2017

anntzer deleted the private-axesstack branch August 13, 2017 14:07

This was referenced Aug 14, 2017

FIX: the new _AxesStack with np.array as input #9029

Closed

Revert "ENH: Switch to a private, simpler AxesStack." #9034

Merged

anntzer mentioned this pull request Aug 31, 2017

DOC: document axes-collision deprecation #9132

Merged

anntzer mentioned this pull request Jun 13, 2018

plt.subplot eats my subplots #11435

Closed

anntzer mentioned this pull request Oct 13, 2018

Reactivating pyplot.subplot throws warning. #12513

Closed

anntzer mentioned this pull request Sep 14, 2021

missing imshow() subplots when using tight_layout() #4976

Closed

anntzer mentioned this pull request Feb 10, 2022

Deprecate auto-removal of overlapping Axes by plt.subplot{,2grid}. #22418

Merged

6 tasks

		@@ -157,6 +158,60 @@ def __contains__(self, a):
		return a in self.as_list()


		class _AxesStack:

ENH: Switch to a private, simpler AxesStack. #7377

ENH: Switch to a private, simpler AxesStack. #7377

Conversation

anntzer commented Nov 2, 2016

NelleV commented Nov 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tacaswell commented Nov 2, 2016

anntzer commented Nov 2, 2016

NelleV commented Nov 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

efiring commented Nov 10, 2016

QuLogic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anntzer commented Nov 11, 2016

tacaswell commented Nov 11, 2016

NelleV commented Nov 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anntzer Nov 11, 2016 • edited Loading

Choose a reason for hiding this comment

tacaswell commented Nov 11, 2016

NelleV commented Nov 11, 2016

tacaswell commented Nov 11, 2016

QuLogic commented Nov 11, 2016

anntzer commented Nov 11, 2016

tacaswell commented Nov 11, 2016

efiring commented Nov 11, 2016

anntzer commented Nov 11, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NelleV commented Jan 17, 2017

anntzer commented Jan 17, 2017

codecov-io commented Jan 17, 2017 • edited Loading

Current coverage is 62.01% (diff: 97.05%)

NelleV left a comment

Choose a reason for hiding this comment

anntzer commented Jan 20, 2017

tacaswell commented Aug 10, 2017

anntzer commented Aug 10, 2017

QuLogic commented Aug 13, 2017

tacaswell commented Aug 14, 2017

anntzer Nov 11, 2016 •

edited

Loading

anntzer commented Nov 11, 2016 •

edited

Loading

codecov-io commented Jan 17, 2017 •

edited

Loading