Skip to content

fix the stack remove error #7335

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 10 commits into from
Closed

fix the stack remove error #7335

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
db343c3
fix the stack remove error
JunTan Oct 24, 2016
25492ce
add test for the bug in AxesStack.add
JunTan Oct 25, 2016
23cfcda
move testing for Axes Stack add into test_figure.py
JunTan Nov 1, 2016
6932d7c
fix style and add explanation
JunTan Nov 1, 2016
0ab7a30
Delete test_axesstack_add.py
JunTan Nov 1, 2016
9d2d33e
change warning to raise error
JunTan Nov 1, 2016
23a8d73
correct pep8 style
JunTan Nov 2, 2016
33318e3
one more assert verifies re-add a key to the stack should not succeed…
JunTan Nov 2, 2016
6dc9248
document the changes in AxesStack add
JunTan Nov 22, 2016
4c72fe7
made the documentation of AxesStack.add nicer and clearer
JunTan Nov 22, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions doc/users/whats_new.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

--------------------------------------------

The AxesStack does not allow users to re-add the same key. If the user tries
to do so, a KeyError will be raised.
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`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be leftover from something.



Default style changes
Expand Down
18 changes: 8 additions & 10 deletions lib/matplotlib/figure.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
a_existing = dict(self._elements).get(key)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not follow 😞

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if a_existing is not None:
Stack.remove(self, (key, a_existing))
warnings.warn(
"key %s already existed; Axes is being replaced" % key)
# I don't think the above should ever happen.
raise KeyError('Key %s already exists in the AxesStack' % key)
# This only happens if user call this function directly

if a in self:
return None
Expand Down Expand Up @@ -362,7 +360,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__):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably match the exact class name instead.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

from matplotlib.backends import backend_webagg
return backend_webagg.ipython_inline_display(self)

Expand Down Expand Up @@ -816,7 +814,7 @@ def _make_key(self, *args, **kwargs):
'make a hashable key out of args and kwargs'

def fixitems(items):
#items may have arrays and lists in them, so convert them
# items may have arrays and lists in them, so convert them
# to tuples for the key
ret = []
for k, v in items:
Expand Down Expand Up @@ -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)):
Copy link
Contributor

@anntzer anntzer Nov 30, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

ax._sharex.update_params()
ax.set_position(ax._sharex.figbox)
elif (ax._sharey is not None and
Expand Down Expand Up @@ -1921,8 +1919,8 @@ def figaspect(arg):
# could become rc parameters, for now they're hardwired.
figsize_min = np.array((4.0, 2.0)) # min length for width/height
figsize_max = np.array((16.0, 16.0)) # max length for width/height
#figsize_min = rcParams['figure.figsize_min']
#figsize_max = rcParams['figure.figsize_max']
# figsize_min = rcParams['figure.figsize_min']
# figsize_max = rcParams['figure.figsize_max']

# Extract the aspect ratio of the array
if isarray:
Expand Down
16 changes: 14 additions & 2 deletions lib/matplotlib/tests/test_figure.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,13 @@
import six
from six.moves import xrange

from nose.tools import assert_equal, assert_true
from matplotlib import rcParams
from nose.tools import assert_equal, assert_true, assert_raises
from matplotlib import rcParams, figure
from matplotlib.testing.decorators import image_comparison, cleanup
from matplotlib.axes import Axes
import matplotlib.pyplot as plt
import numpy as np
import warnings


@cleanup
Expand Down Expand Up @@ -216,6 +217,17 @@ def test_figaspect():
assert h / w == 1


@cleanup
def test_stack_remove():
a = figure.AxesStack()
key = '123'
axes = plt.figure().add_subplot(111)
a.add(key, axes)
# Verify some things
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is something supposed to come after this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, sorry , it should come before the assertion.

Copy link
Contributor

@trpham trpham Dec 6, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the comment be more specific?

assert_raises(KeyError, a.add, key, plt.figure().add_subplot(111))
assert_equal(a._elements, [(key, (1, axes))])


if __name__ == "__main__":
import nose
nose.runmodule(argv=['-s', '--with-doctest'], exit=False)