Allow unhashable keys in AxesStack. #8451
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.
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.
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 for
all this time (not that I ever coded anything using it... just simply
misunderstood it). I am wondering now if our whole transforms hashing has
been working completely by accident because people don't typically change
the 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 and typically 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 to
3d6c9c0
Compare
|
done |
|
Thank you @anntzer |
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.