X Tutup
Skip to content

BUG : don't use mutable objects as dictionary keys#2927

Merged
pelson merged 1 commit intomatplotlib:masterfrom
tacaswell:clip_transform_hash
Jun 9, 2014
Merged

BUG : don't use mutable objects as dictionary keys#2927
pelson merged 1 commit intomatplotlib:masterfrom
tacaswell:clip_transform_hash

Conversation

@tacaswell
Copy link
Copy Markdown
Member

@tacaswell tacaswell commented Mar 25, 2014

Base key on the string version of the transform matrix

This addresses the core issue (we are trying to use a mutable object as a key) instead of breaking the data-model in one of two ways.

Closes #2828

Alternate to #2909

mdboom
mdboom reviewed Mar 25, 2014
View reviewed changes
lib/matplotlib/backends/backend_ps.py Outdated
Copy link
Copy Markdown
Member

@mdboom mdboom Mar 25, 2014

Choose a reason for hiding this comment

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

Shouldn't this be tostring()?

Copy link
Copy Markdown
Member Author

@tacaswell tacaswell Mar 25, 2014

Choose a reason for hiding this comment

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

yes, fixed, squashed and forced.

@mdboom
Copy link
Copy Markdown
Member

mdboom commented Mar 25, 2014

I see how this reduces the impact of the change (and possibility for unexpected consequences) vs. #2909, so this probably makes some sense.

@pelson: Agree?

@tacaswell tacaswell mentioned this pull request Mar 25, 2014
Closed
pelson
pelson reviewed Mar 25, 2014
View reviewed changes
lib/matplotlib/backends/backend_ps.py Outdated
Copy link
Copy Markdown
Member

@pelson pelson Mar 25, 2014

Choose a reason for hiding this comment

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

Is clippath immutable?

Copy link
Copy Markdown
Member

@pelson pelson Mar 25, 2014

Choose a reason for hiding this comment

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

Is clippath_transform always affine?

Copy link
Copy Markdown
Member Author

@tacaswell tacaswell Mar 25, 2014

Choose a reason for hiding this comment

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

It looks like get_matrix is defined on the Transform class so I think it should always be there, but the hash will just ignore the non-affine part.

clippath might be mutable, but it does not define __eq__ (assuming it is some class defined in path.py) so we are still getting the old py2k memory-address-as-hash behavior here.

Copy link
Copy Markdown
Member

@mdboom mdboom Mar 25, 2014

Choose a reason for hiding this comment

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

I suppose it's possible that clippath_transform is not always affine (though I don't know if that ever happens in practice). Maybe id(clippath_transform) would work here? (It would at least give us the old Python 2.x behavior).

Copy link
Copy Markdown
Member

@pelson pelson May 21, 2014

Choose a reason for hiding this comment

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

If there is no downside, ID is definitely the way to go IMHO...

@tacaswell tacaswell added this to the v1.4.0 milestone Apr 3, 2014
@efiring
Copy link
Copy Markdown
Member

efiring commented Jun 3, 2014

@tacaswell, I haven't looked into this in detail, but after reading the comments on this and related issues, I conclude that if you change the line to

key = (clippath, id(clippath_transform))

then everyone, and all Python versions, will be happy.

@tacaswell
Copy link
Copy Markdown
Member Author

tacaswell commented Jun 3, 2014

@efiring I agree, I just have not gotten to it yet.

@tacaswell
Copy link
Copy Markdown
Member Author

tacaswell commented Jun 7, 2014

Fixed and re-based.

@pelson
Copy link
Copy Markdown
Member

pelson commented Jun 9, 2014

@tacaswell - this doesn't merge cleanly. Would you mind rebasing and I'll merge.

@tacaswell
BUG : don't use mutable objects as dictionary keys
9b9c0c6 
Base key on the id of the transform.  This maintains
the old behavior and does not break the python hashable
object model.

changed the variable id -> pid so as to not shadow the
method `id`

Closes matplotlib#2828
@tacaswell
Copy link
Copy Markdown
Member Author

tacaswell commented Jun 9, 2014

@pelson Done

pelson added a commit that referenced this pull request Jun 9, 2014
@pelson
Merge pull request #2927 from tacaswell/clip_transform_hash
c4a014d 
BUG : don't use mutable objects as dictionary keys
@pelson pelson merged commit c4a014d into matplotlib:master Jun 9, 2014
@tacaswell tacaswell deleted the clip_transform_hash branch June 9, 2014 15:53
tacaswell added a commit to tacaswell/matplotlib that referenced this pull request Sep 16, 2014
@tacaswell
BUG : fix eps corruption when using clipping
660dc47 
_get_clip_path was returning '<built-in function id>' instead
of the ps-function name like it should have been.  This was introduced
in 9b9c0c6 in PR matplotlib#2927.

Closes matplotlib#3528
@tacaswell tacaswell mentioned this pull request Sep 16, 2014
Merged
tacaswell added a commit to tacaswell/matplotlib that referenced this pull request Sep 16, 2014
@tacaswell
BUG : fix eps corruption when using clipping
3424506 
_get_clip_path was returning '<built-in function id>' instead
of the ps-function name like it should have been.  This was introduced
in 9b9c0c6 in PR matplotlib#2927.

Closes matplotlib#3523
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.

Projects

None yet

Milestone

v1.4.0

Development

Successfully merging this pull request may close these issues.

PS backend fails to save polar plot

5 participants

@tacaswell @mdboom @efiring @pelson @QuLogic
X Tutup