fix the stack remove error by JunTan · Pull Request #7335 · matplotlib/matplotlib · GitHub

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

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

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.

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

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": ...

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

@NelleV what is PR?

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

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
.

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.

Is something supposed to come after this?

no, sorry , it should come before the assertion.

… or modify the old one

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

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.

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?

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.

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.

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

Will defer to @efiring on this PR.

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.

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.

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

This looks good. Thanks @JunTan !

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).

This seems to be leftover from something.

@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.

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.

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

review outdated

Should the comment be more specific?

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

                       # webagg unconditiionally.
                       if (self.canvas is not None and
-                          'WebAgg' in self.canvas.__class__.__name__):
+                              'WebAgg' in self.canvas.__class__.__name__):

+                  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

                               # Check if sharing a subplots axis
                               if (ax._sharex is not None and
-                                  isinstance(ax._sharex, SubplotBase)):
+                                      isinstance(ax._sharex, SubplotBase)):

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

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

                       except TypeError:
                           raise ValueError("first argument, %s, is not a valid key" % key)
-                      a_existing = self.get(key)


		matplotlib 2.0 supports Python 2.7, and 3.4+

		The behavior of AxesStack re-add key changes

+              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`

fix the stack remove error by JunTan · Pull Request #7335 · matplotlib/matplotlib · GitHub

Repository navigation

fix the stack remove error - #7335

fix the stack remove error#7335JunTan wants to merge 10 commits intomatplotlib:mastermatplotlib/matplotlib:masterfrom JunTan:7194JunTan/matplotlib:7194Copy head branch name to clipboard

Conversation

JunTan commented Oct 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

QuLogic commented Oct 24, 2016

Uh oh!

NelleV commented Oct 25, 2016

Uh oh!

JunTan commented Oct 25, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NelleV commented Nov 2, 2016

Uh oh!

JunTan commented Nov 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WeatherGod commented Nov 2, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

QuLogic commented Nov 3, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tacaswell commented Nov 4, 2016

Uh oh!

NelleV commented Nov 4, 2016

Uh oh!

tacaswell commented Nov 10, 2016

Uh oh!

tacaswell left a comment

Choose a reason for hiding this comment

Uh oh!

efiring commented Nov 14, 2016

Uh oh!

efiring commented Nov 14, 2016

Uh oh!

NelleV commented Nov 21, 2016

Uh oh!

NelleV commented Nov 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NelleV commented Nov 29, 2016

Uh oh!

QuLogic commented Nov 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anntzer Nov 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

fix the stack remove error#7335
JunTan wants to merge 10 commits into
matplotlib:masterfrom
JunTan:7194

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