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

py3: implement PolyDict.__hash__ #24761

Closed
embray opened this issue Feb 16, 2018 · 15 comments
Closed

py3: implement PolyDict.__hash__ #24761

embray opened this issue Feb 16, 2018 · 15 comments

Comments

@embray
Copy link
Contributor

embray commented Feb 16, 2018

This type needs to have an explicit __hash__ implementation for Python 3.

Although its underlying data structure is a mutable dict, it's not intended to be mutated and the API generally disallows this (it still might be better if it used an immutable dict).

Component: python3

Author: Erik Bray

Branch/Commit: 4f5e114

Reviewer: Frédéric Chapoton

Issue created by migration from https://trac.sagemath.org/ticket/24761

@embray embray added this to the sage-8.2 milestone Feb 16, 2018
@embray
Copy link
Contributor Author

embray commented Feb 16, 2018

Dependencies: #24760

@embray
Copy link
Contributor Author

embray commented Feb 16, 2018

Changed dependencies from #24760 to #24759

@tscrim
Copy link
Collaborator

tscrim commented Feb 17, 2018

comment:3

Instead of sorting the output, I would use a frozenset since that is more inline with the mathematical interpretation. I also feel like that (in general) would be faster than sorting, but I haven't tested. Thoughts?

@jdemeyer
Copy link

comment:4

Just one detail: type(x) is faster than x.__class__ in Cython code.

@fchapoton
Copy link
Contributor

comment:5

does not apply

@embray
Copy link
Contributor Author

embray commented Feb 19, 2018

comment:6

Replying to @tscrim:

Instead of sorting the output, I would use a frozenset since that is more inline with the mathematical interpretation. I also feel like that (in general) would be faster than sorting, but I haven't tested. Thoughts?

Yes, I think you have a point there. I'll try it out.

@embray
Copy link
Contributor Author

embray commented Feb 19, 2018

comment:7

Replying to @jdemeyer:

Just one detail: type(x) is faster than x.__class__ in Cython code.

Interesting--I'd have just assumed they would optimize that, but I guess technically you can't since many objects' .__class__ can be overridden. We're certainly not concerned with that here.

@jdemeyer
Copy link

comment:8

Replying to @embray:

Interesting--I'd have just assumed they would optimize that, but I guess technically you can't since many objects' .__class__ can be overridden.

Indeed. Cython really wants to be Python-compatible. type() is a built-in function, so Cython knows exactly what it does. But .__class__ is just an attribute that a custom (meta?)class can override.

This also explains why you shouldn't do from six.moves import range in Cython code: instead of a known builtin, you get a random Python object that Cython cannot do anything with.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 19, 2018

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

43d9234fixing doctests
f2ea845Merge branch 'public/polydict-sort-repr' in 8.2.b6
4f5e114Implement PolyDict.__hash__

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 19, 2018

Changed commit from a515c54 to 4f5e114

@embray
Copy link
Contributor Author

embray commented Feb 19, 2018

comment:10

Rebased and incorporated minor review comments.


New commits:

43d9234fixing doctests
f2ea845Merge branch 'public/polydict-sort-repr' in 8.2.b6
4f5e114Implement PolyDict.__hash__

@fchapoton
Copy link
Contributor

Changed dependencies from #24759 to none

@fchapoton
Copy link
Contributor

Reviewer: Frédéric Chapoton

@fchapoton
Copy link
Contributor

comment:12

ok

@vbraun
Copy link
Member

vbraun commented Mar 6, 2018

Changed branch from u/embray/polydict-hash to 4f5e114

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants