Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Allow unhashable keys in AxesStack.#8451
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
lib/matplotlib/figure.py Outdated
@@ -122,7 +124,7 @@ def add(self, key, a): | |||
try: | |||
hash(key) | |||
except TypeError: | |||
raise ValueError("first argument, %s, is not a valid key" % 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.
But if the same key comes in again it will get a different object which sort of defeats the point of 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.
But transforms are explicitly mutable (not just by fiddling with the attributes: they actually expose a bunch of methods explicitly for that purpose), which means that trying to key on them is bound to give wrong results (this is the same reason why lists are not hashable in python). (In fact I had prepared another PR for making all transform classes hashable, but that is not a robust solution in the presence of mutability.)
I argued in#7377 that this behavior (trying to detect and drop duplicates axes) is already brittle anyways and should be deprecated.
I thought that these things are keyed on frozen transforms? …On Mon, Apr 10, 2017 at 4:20 PM, Antony Lee ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In lib/matplotlib/figure.py <#8451 (comment)> : > @@ -122,7 +124,7 @@ def add(self, key, a): try: hash(key) except TypeError: - raise ValueError("first argument, %s, is not a valid key" % key) But transforms are explicitly mutable (not just by fiddling with the attributes: they actually expose a bunch of methods explicitly for that purpose), which means that trying to key on them is bound to give wrong results (this is the same reason why lists are not hashable in python). (In fact I had prepared another PR for making all transform classes hashable, but that is not a robust solution in the presence of mutability.) I argued in#7377 <#7377> that this behavior (trying to detect and drop duplicates axes) is already brittle anyways and should be deprecated. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#8451 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AARy-P1RGG6Io2QHihT4-seu-dxA12H8ks5ruo7zgaJpZM4M3-G2> . |
Freezing a transform doesn't make it immutable, it just makes it independent of changes to its children (by recursively replacing the children by "private" copies of them). This is in fact the goal stated in the docstring:
|
Oh, wow. I guess I completely misunderstood how frozen() was being used forall this time (not that I ever coded anything using it... just simplymisunderstood it). I am wondering now if our whole transforms hashing hasbeen working completely by accident because people don't typically changethe transforms after the fact? …On Mon, Apr 10, 2017 at 4:36 PM, Antony Lee ***@***.***> wrote: Freezing a transform doesn't make it immutable, it just makes it independent of changes to its children (by recursively replacing the children by "private" copies of them). This is in fact the goal stated in the docstring: Returns a frozen copy of this transform node. The frozen copy will not update when its children change. Useful for storing a previously known state of a transform where ``copy.deepcopy()`` might normally be used. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#8451 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AARy-A5rnotj66dMaUxA27x-20kpWpTDks5rupLDgaJpZM4M3-G2> . |
Transforms are not hashable andtypically not used as keys in AxesStack. |
right, I meant "keys", not hash. …On Mon, Apr 10, 2017 at 4:46 PM, Antony Lee ***@***.***> wrote: Transforms are not hashable and *typically* not used as keys in AxesStack. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#8451 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AARy-FaFLh5OKGrsLwyNToubo2zodXXEks5rupUNgaJpZM4M3-G2> . |
Ok, that makes sense, but this still seems hackish. Would in be better to remove the transform from the kwargs used to make the key (in On reading the code of the OP, does |
FWIW I still think the ultimately correct way to do this is to get rid of the brittle "check whether there is already an axes with the same key" approach (for reasons already explained elsewhere). Alternatively, instead of using a dict, you could just use a list of pairs (key, value) and check for equality with the keys one at a time -- there is no way you can have so many Axes that the performance becomes an issue. But this is still bound to have problematic semantics due to key mutability. |
@anntzer I am convinced on this, can you rebase? |
They simply will compare unequal to anything else.
c4e83b4
to3d6c9c0
Comparedone |
Thank you@anntzer |
Uh oh!
There was an error while loading.Please reload this page.
They simply will compare unequal to anything else.
Closes#8395.
Alternative to#8399 (although the part about removing
Transform.__eq__
in#8399 stays valid).See discussion starting at#7377 (comment) for why this is a reasonable approach.