Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Jun 9, 2014

Conversation

tacaswell
Copy link
Member

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

@@ -554,7 +554,8 @@ def _convert_path(self, path, transform, clip=False, simplify=None):
return ps

def _get_clip_path(self, clippath, clippath_transform):
id = self._clip_paths.get((clippath, clippath_transform))
key = (clippath, clippath_transform.get_matrix().tostring)
Copy link
Member

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
Member Author

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

@@ -554,7 +554,8 @@ def _convert_path(self, path, transform, clip=False, simplify=None):
return ps

def _get_clip_path(self, clippath, clippath_transform):
id = self._clip_paths.get((clippath, clippath_transform))
key = (clippath, clippath_transform.get_matrix().tostring())
Copy link
Member

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
Member

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
Member Author

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
Member

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
Member

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
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
Member Author

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

@tacaswell
Copy link
Member Author

Fixed and re-based.

@pelson
Copy link
Member

pelson commented Jun 9, 2014

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

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
Member Author

@pelson Done

pelson added a commit that referenced this pull request Jun 9, 2014
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
_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 added a commit to tacaswell/matplotlib that referenced this pull request Sep 16, 2014
_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
Labels
Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PS backend fails to save polar plot
5 participants