Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged

Conversation

anntzer
Copy link
Contributor

@anntzeranntzer commentedApr 9, 2017
edited
Loading

They simply will compare unequal to anything else.

Closes#8395.
Alternative to#8399 (although the part about removingTransform.__eq__ in#8399 stays valid).
See discussion starting at#7377 (comment) for why this is a reasonable approach.

@@ -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)
Copy link
Member

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.

Copy link
ContributorAuthor

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.

@WeatherGod
Copy link
Member

WeatherGod commentedApr 10, 2017 via email

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

@anntzer
Copy link
ContributorAuthor

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.

@WeatherGod
Copy link
Member

WeatherGod commentedApr 10, 2017 via email

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

@anntzer
Copy link
ContributorAuthor

Transforms are not hashable andtypically not used as keys in AxesStack.

@WeatherGod
Copy link
Member

WeatherGod commentedApr 10, 2017 via email

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

@tacaswelltacaswell added this to the2.0.1 (next bug fix release) milestoneApr 12, 2017
@tacaswell
Copy link
Member

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 (inFigure._make_key)?

On reading the code of the OP, doestransform even get used? If anAxes is passed in, kwargs isonly used for the key and nothing else. This seems not quite right.

@anntzer
Copy link
ContributorAuthor

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.

@QuLogicQuLogic modified the milestones:2.0.1 (next bug fix release),2.0.2 (next bug fix release)May 3, 2017
@tacaswell
Copy link
Member

@anntzer I am convinced on this, can you rebase?

They simply will compare unequal to anything else.
@anntzeranntzerforce-pushed theunhashable-keys-in-axesstack branch fromc4e83b4 to3d6c9c0CompareJuly 10, 2017 23:53
@anntzer
Copy link
ContributorAuthor

done

@tacaswelltacaswell merged commitf5f3d79 intomatplotlib:masterJul 11, 2017
@tacaswell
Copy link
Member

Thank you@anntzer

@QuLogicQuLogic modified the milestones:2.1 (next point release),2.0.3 (next bug fix release)Jul 11, 2017
@anntzeranntzer deleted the unhashable-keys-in-axesstack branchJuly 11, 2017 03:11
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@tacaswelltacaswelltacaswell left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v2.1
Development

Successfully merging this pull request may close these issues.

4 participants
@anntzer@WeatherGod@tacaswell@QuLogic

[8]ページ先頭

©2009-2025 Movatter.jp