Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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. |
I just pushed a simple test for that. On Sun, Oct 23, 2016 at 5:51 PM, Elliott Sales de Andrade <
|
| @@ -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__): | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 likeself.canvas.__class__.__name__ == "<something>"" (orself.canvas.__class__.__module__ == "<something>") rather than a substring check.
There was a problem hiding this comment.
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 asif type(self.canvas).__name__ == "FigureCanvasWebAgg": ...
Hi@JunTan |
JunTan commentedNov 2, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@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, JunTannotifications@github.com wrote:
|
| @cleanup | ||
| def test_stack_remove(): | ||
| ''' | ||
| Before fixing the bug, doing the following will give you following error: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
sure, I added the docstring because I got people asking me what is the test for.
| 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
… or modify the old one
If this is complete (which it seems to be), please remove the |
| @@ -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) | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 astack.remove() takes in the casting value as an argument not the one return fromself.get(key). Now thestack.remove() is removed because the old key-value pair should not be removed or changed if the key is added again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I do not follow 😞
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
refer to thisJunTan@db343c3#diff-c3ce78ddcc28c54e04ade080c149c1b1R130
There was astack.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 removestack.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 tostack.remove().
There was a problem hiding this comment.
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 underlyingself.get() also castsself._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 It looks like changing the remove to |
Hi@tacaswell 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 looks good. Thanks@JunTan ! |
| @@ -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 | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm not sureAxesStack is what should be documented here; it's an internal (if not private) implementation detail and what the user really sees is thatFigure.add_axes (andplt.axes?) no longer has this behaviour. "What's new" should be about user-visible changes. This specific information could probably go inapi/api_changes.
There was a problem hiding this comment.
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).
| 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` |
There was a problem hiding this comment.
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.
@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 herenever happens when AxesStack is used internally. |
QuLogic commentedNov 29, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 |
| @@ -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)): | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Just make thisif isinstance(ax._sharex, SubplotBase): (the first check is not needed).
| key = '123' | ||
| axes = plt.figure().add_subplot(111) | ||
| a.add(key, axes) | ||
| # Verify some things |
There was a problem hiding this comment.
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?
Closing as we're just going (in the long run) to deprecate AxesStack (or rather, make it private). See#9037 and linked issues. |
Uh oh!
There was an error while loading.Please reload this page.
Before fixing the bug, doing the following will give you error:
#7194
Now the warning should be more clear and should not raise any error