fix the stack remove error #7335

JunTan · 2016-10-24T00:44:01Z

Before fixing the bug, doing the following will give you error:
#7194

>>> a=figure.AxesStack()
>>> a.add('123',plt.figure().add_subplot(111))
<matplotlib.axes.AxesSubplot object at 0x7f13d27a3490>
>>> a._elements
[('123', (1, <matplotlib.axes.AxesSubplot object at 0x7f13d27a3490>))]
    >>> a.add('123',plt.figure().add_subplot(111))
    Traceback (most recent call last):
    File "<stdin>", line 1, in <module>
    File "/usr/lib/pymodules/python2.7/matplotlib/figure.py", line 120, in
    add
      Stack.remove(self, (key, a_existing))
    File "/usr/lib/pymodules/python2.7/matplotlib/cbook.py", line 1343, in
    remove
      raise ValueError('Unknown element o')
    ValueError: Unknown element o

Now the warning should be more clear and should not raise any error

QuLogic · 2016-10-24T00:51:04Z

Which error do you mean? Can you add a test as well?

NelleV · 2016-10-25T15:51:13Z

Hi @JunTan,

Please add a meaningful description of the patch you propose and a link to the ticket.
Also, the tests you've added are not proper python tests. You can read about testing here: http://docs.python-guide.org/en/latest/writing/tests/
As well as check out the code in that folder to identify what is expected of you.

JunTan · 2016-10-25T18:45:36Z

I just pushed a simple test for that.

On Sun, Oct 23, 2016 at 5:51 PM, Elliott Sales de Andrade <
notifications@github.com> wrote:

Which error do you mean? Can you add a test as well?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#7335 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AKLpNpKTAgHuuFTWereZJB_0Zp5wT80Kks5q3AECgaJpZM4KeTdD
.

remove unnecessary test

anntzer · 2016-11-01T19:12:30Z

lib/matplotlib/figure.py

@@ -362,7 +362,7 @@ def _repr_html_(self):
        # We can't use "isinstance" here, because then we'd end up importing
        # webagg unconditiionally.
        if (self.canvas is not None and
-            'WebAgg' in self.canvas.__class__.__name__):
+                'WebAgg' in self.canvas.__class__.__name__):


This should probably match the exact class name instead.

This is a pep8 modification, so no need to hold this PR on that

No, my point was "this should be something like self.canvas.__class__.__name__ == "<something>"" (or self.canvas.__class__.__module__ == "<something>") rather than a substring check.

Actually the whole condition should just get rewritten as if type(self.canvas).__name__ == "FigureCanvasWebAgg": ...

NelleV · 2016-11-02T05:56:16Z

Hi @JunTan
There is small element missing from your PR. I'll let you figure out what it is.

JunTan · 2016-11-02T18:30:36Z

@NelleV what is PR?

I figure out there is a blank line contains whitespace in figure.py

WeatherGod · 2016-11-02T18:36:53Z

PR is short for "Pull Request". Sorry, we tend to get used to our jargon.

On Wed, Nov 2, 2016 at 2:30 PM, JunTan notifications@github.com wrote:

@NelleV https://github.com/NelleV what is PR?

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#7335 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AARy-BM6R3-1umylo70Z1swn9g3uz8LUks5q6NbNgaJpZM4KeTdD
.

QuLogic · 2016-11-02T19:43:29Z

lib/matplotlib/tests/test_figure.py

+@cleanup
+def test_stack_remove():
+    '''
+    Before fixing the bug, doing the following will give you following error:


This docstring is not very useful. The reason is that nose uses the first line as the name of the test. So the test gets called "Before fixing the bug, doing the following will give you following error:" which is just wrong.

Anyway, I don't think this docstring provides any useful information, because it is clear from the name and the test code itself what it is. I think you can just drop the docstring entirely.

sure, I added the docstring because I got people asking me what is the test for.

QuLogic · 2016-11-02T19:43:43Z

lib/matplotlib/tests/test_figure.py

+    a = figure.AxesStack()
+    a.add(key, plt.figure().add_subplot(111))
+    assert_raises(KeyError, a.add, key, plt.figure().add_subplot(111))
+    # Verify some things


Is something supposed to come after this?

no, sorry , it should come before the assertion.

… or modify the old one

QuLogic · 2016-11-03T01:25:11Z

If this is complete (which it seems to be), please remove the [WIP] from the title.

tacaswell · 2016-11-03T22:35:10Z

lib/matplotlib/figure.py

@@ -125,12 +125,10 @@ def add(self, key, a):
        except TypeError:
            raise ValueError("first argument, %s, is not a valid key" % key)

-        a_existing = self.get(key)


Why does reaching into the internals and casting it to a dict fixes this bug?

the casting is there because there was a stack.remove() takes in the casting value as an argument not the one return from self.get(key). Now the stack.remove() is removed because the old key-value pair should not be removed or changed if the key is added again.

I do not follow 😞

refer to this JunTan@db343c3#diff-c3ce78ddcc28c54e04ade080c149c1b1R130

There was a stack.remove() attempts to remove the key-value pair if the key is added to the stack again. stack.remove() takes in (key, (self.ind, axes)) as its parameter.Before casting, stack.remove() takes in (key, , axes). Thus the casting fixes this problem.

Then we decide to remove stack.remove() because the old key-value pair should not be removed or changed if the key is added again. Now it just raises error if you try to add an old key again.

Thus the casting fixes the bug of the call to stack.remove().

I now see that the underlying self.get() also casts self._elements through a dict. It turns out I am deeply confused about this class (and it's super class) works.

tacaswell · 2016-11-04T00:51:48Z

Won't this just swap the error that the OP raised from remove failing to this error?

It looks like changing the remove to self.remove(a_existing) would have been the minimal change?

NelleV · 2016-11-04T23:16:44Z

Hi @tacaswell
so, that particular feature never worked (replacing an axes with a new one and the same key).
After discussing with @JunTan and @anntzer , we thought that it may be best to make it impossible to replace an axes rather than to fix the code. I think it will avoid users accidentally replacing one axes of the AxesStack with another one.
I also think the AxesStack was never meant to be public in the first place. It is used to keep track of the axes of a figure.

Note that if a user is using the AxesStack class on it's own, to obtain the "old" behavior (that did not work), the user could explicitly remove the axes from the stack manually using the remove method.

I think it is much more pythonic being explicit about this rather than the initial intended behavior.

tacaswell · 2016-11-10T13:47:14Z

Looking at the history this code it looks like @efiring wrote this in 2010 and this functionality worked for 3 days ;)

This patch does not actually fix the bug that the OP reported, but changes the API to raise a different exception.

I still think it is better to just fix the intended behavior (if it should have beet public or not, it is), but if you want to change the API (which is low impact due to their being a bug) then this needs to be documented in API changes (and probably documented in the docstring).

It is not clear to me that the intended behavior is 'non-pythonic', it seems equivalent to __setitem__ being able to replace a value.

tacaswell

Either needs to actually fix the bug or document the API change.

Will defer to @efiring on this PR.

efiring · 2016-11-14T07:23:48Z

If we are going to accept #7377 or some variation on it, then I think it makes sense to leave things alone for now rather than merge this, with or without modifications.

efiring · 2016-11-14T07:38:18Z

Looking again at #7377, I see that it is not OK as-is, but I look favorably on the proposal to deprecate and remove the behavior in which add_axes returns an existing one instead of a new one.

NelleV · 2016-11-21T21:18:19Z

Hi @JunTan
This is a small reminder that the change needs to be documented as discussed.
Thanks,

NelleV · 2016-11-22T19:30:46Z

This looks good. Thanks @JunTan !

QuLogic · 2016-11-22T19:54:59Z

doc/users/whats_new.rst

@@ -19,6 +19,15 @@ New in matplotlib 2.0

   matplotlib 2.0 supports Python 2.7, and 3.4+

+The behavior of AxesStack re-add key changes


I'm not sure AxesStack is what should be documented here; it's an internal (if not private) implementation detail and what the user really sees is that Figure.add_axes (and plt.axes?) no longer has this behaviour. "What's new" should be about user-visible changes. This specific information could probably go in api/api_changes.

Agreed that this should be moved to the api_changes and cleaned up (see @QuLogic's comment below).

QuLogic · 2016-11-22T19:55:52Z

doc/users/whats_new.rst

+If the user really wants to re-add the same key, then the old one should
+be removed first by using :func:`AxesStack.remove` and re-add the new key.
+
+:func:`~figure.AxesStack.add`


This seems to be leftover from something.

NelleV · 2016-11-29T23:10:21Z

@anntzer Can you check this PR? It'd be nice to have this in 2.0.

@QuLogic We document AxesStack because this "bug" was detected by someone who was explicitely using this function. It is in our public API, thus is public. If it wasn't, this bug would never have been detected as the case described here never happens when AxesStack is used internally.
The long term plan is to deprecate this from our public API, but this is not the goal of this PR.

QuLogic · 2016-11-29T23:14:27Z

I'm not saying not to document it; I'm saying it should be in the API's What's New section. What should be in the main What's New section is something about Figure.add_axes (or plt.axes), because that is what gets used by most users.

anntzer · 2016-11-30T05:58:27Z

lib/matplotlib/figure.py

@@ -1745,7 +1743,7 @@ def subplots_adjust(self, *args, **kwargs):
            if not isinstance(ax, SubplotBase):
                # Check if sharing a subplots axis
                if (ax._sharex is not None and
-                    isinstance(ax._sharex, SubplotBase)):
+                        isinstance(ax._sharex, SubplotBase)):


Just make this if isinstance(ax._sharex, SubplotBase): (the first check is not needed).

review outdated

trpham · 2016-12-06T20:39:02Z

lib/matplotlib/tests/test_figure.py

+    key = '123'
+    axes = plt.figure().add_subplot(111)
+    a.add(key, axes)
+    # Verify some things


Should the comment be more specific?

anntzer · 2017-09-16T23:19:24Z

Closing as we're just going (in the long run) to deprecate AxesStack (or rather, make it private). See #9037 and linked issues.
Thanks for the PR.

fix the stack remove error

db343c3

Kojoley added status: needs clarification Issues that need more information to resolve. status: needs revision labels Oct 24, 2016

add test for the bug in AxesStack.add

25492ce

NelleV added the Machine Shop label Oct 25, 2016

NelleV changed the title ~~fix the stack remove error~~ [WIP] fix the stack remove error Oct 25, 2016

JunTan added 3 commits October 31, 2016 19:29

move testing for Axes Stack add into test_figure.py

23cfcda

fix style and add explanation

6932d7c

Delete test_axesstack_add.py

0ab7a30

remove unnecessary test

anntzer reviewed Nov 1, 2016

View reviewed changes

change warning to raise error

9d2d33e

correct pep8 style

23a8d73

QuLogic reviewed Nov 2, 2016

View reviewed changes

one more assert verifies re-add a key to the stack should not succeed…

33318e3

… or modify the old one

NelleV closed this Nov 3, 2016

NelleV reopened this Nov 3, 2016

tacaswell reviewed Nov 3, 2016

View reviewed changes

tacaswell added this to the 2.0.1 (next bug fix release) milestone Nov 4, 2016

NelleV changed the title ~~[WIP] fix the stack remove error~~ [MRG+1] fix the stack remove error Nov 5, 2016

NelleV added the status: needs review label Nov 8, 2016

tacaswell previously requested changes Nov 10, 2016

View reviewed changes

efiring mentioned this pull request Nov 10, 2016

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

Merged

NelleV added status: needs revision and removed status: needs review labels Nov 21, 2016

JunTan added 2 commits November 21, 2016 18:32

document the changes in AxesStack add

6dc9248

made the documentation of AxesStack.add nicer and clearer

4c72fe7

NelleV added status: needs review and removed status: needs revision labels Nov 22, 2016

QuLogic reviewed Nov 22, 2016

View reviewed changes

anntzer reviewed Nov 30, 2016

View reviewed changes

trpham reviewed Dec 6, 2016

View reviewed changes

NelleV removed the Machine Shop label Dec 19, 2016

QuLogic changed the title ~~[MRG+1] fix the stack remove error~~ fix the stack remove error Feb 5, 2017

dstansby added the status: needs revision label Feb 8, 2017

QuLogic modified the milestones: 2.0.1 (next bug fix release), 2.0.2 (next bug fix release) May 3, 2017

anntzer closed this Sep 16, 2017

QuLogic removed Documentation status: needs review labels Oct 20, 2017

		@@ -19,6 +19,15 @@ New in matplotlib 2.0

		matplotlib 2.0 supports Python 2.7, and 3.4+

		The behavior of AxesStack re-add key changes

fix the stack remove error #7335

fix the stack remove error #7335

Conversation

JunTan commented Oct 24, 2016 • edited Loading

QuLogic commented Oct 24, 2016

NelleV commented Oct 25, 2016

JunTan commented Oct 25, 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

NelleV commented Nov 2, 2016

JunTan commented Nov 2, 2016 • edited Loading

WeatherGod commented Nov 2, 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

QuLogic commented Nov 3, 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

Choose a reason for hiding this comment

tacaswell commented Nov 4, 2016

NelleV commented Nov 4, 2016

tacaswell commented Nov 10, 2016

tacaswell left a comment

Choose a reason for hiding this comment

efiring commented Nov 14, 2016

efiring commented Nov 14, 2016

NelleV commented Nov 21, 2016

NelleV commented Nov 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NelleV commented Nov 29, 2016

QuLogic commented Nov 29, 2016 • edited Loading

anntzer Nov 30, 2016 • edited Loading

Choose a reason for hiding this comment

trpham Dec 6, 2016 • edited Loading

Choose a reason for hiding this comment

anntzer commented Sep 16, 2017

JunTan commented Oct 24, 2016 •

edited

Loading

JunTan commented Nov 2, 2016 •

edited

Loading

QuLogic commented Nov 29, 2016 •

edited

Loading

anntzer Nov 30, 2016 •

edited

Loading

trpham Dec 6, 2016 •

edited

Loading