Opened 5 years ago

Closed 5 years ago

#23818 closed enhancement (fixed)

py3: hash for complex interval fields

Reported by: Frédéric Chapoton Owned by:
Priority: major Milestone: sage-8.1
Component: python3 Keywords:
Cc: Erik Bray, Jeroen Demeyer, John Palmieri, aapitzsch Merged in:
Authors: Frédéric Chapoton Reviewers: John Palmieri
Report Upstream: N/A Work issues:
Branch: 54846bf (Commits, GitHub, GitLab) Commit: 54846bf4f0758636eac705a45158ee21b6482703
Dependencies: Stopgaps:

Status badges

Description

as a tiny step towards py3

Change History (16)

comment:1 Changed 5 years ago by Frédéric Chapoton

Branch: u/chapoton/23818
Commit: d07ee6926df2a1f30ce04c85c5efc6aa847b4db2
Status: newneeds_review

New commits:

d07ee69py3: adding hash to complex interval fields

comment:2 Changed 5 years ago by Frédéric Chapoton

ping ?

comment:3 Changed 5 years ago by John Palmieri

Reviewers: John Palmieri
Status: needs_reviewpositive_review

This looks okay to me. Any reason to worry about using __repr__ to define the hash? Is there a better option here?

I'll mark it as positive review, and anyone who objects can change it back.

comment:4 Changed 5 years ago by Erik Bray

Status: positive_reviewneeds_work

I think a better value to use for hashing is something like: (self.__class__, self._prec) since that's closer to, for example, how the object would be reduced for pickling.

comment:5 Changed 5 years ago by git

Commit: d07ee6926df2a1f30ce04c85c5efc6aa847b4db2bdb334a701f52802c48c58dc096c3c9cc190a96f

Branch pushed to git repo; I updated commit sha1. New commits:

4f1dcd7Merge branch 'u/chapoton/23818' in 8.1.b5
bdb334atrac 23818 another hash

comment:6 Changed 5 years ago by Frédéric Chapoton

Here it is. The 32-bit hash needs to be checked. I just took the 64-bit hash mod 2**32.

Last edited 5 years ago by Frédéric Chapoton (previous) (diff)

comment:7 Changed 5 years ago by Frédéric Chapoton

Status: needs_workneeds_review

comment:8 Changed 5 years ago by Erik Bray

Hmm... just an aside, I didn't realize the doctester had a way to specify different results for 64-bit vs 32-bit. It would be good to have other platform specifiers too, or perhaps general support for platform markers https://www.python.org/dev/peps/pep-0508/#environment-markers

That said, I don't know if it makes enormous sense to test the hash value, since the details of how specific objects are hashed in Python is implementation-specific and can change between Python versions and generally isn't guaranteed. Instead, I would just test that hash() of two instances compares equal when it should.

comment:9 Changed 5 years ago by Frédéric Chapoton

Raah, you made me change the hash, and now the new hash does not make sense.. Can I set it back to use _repr_ ?

comment:10 Changed 5 years ago by Frédéric Chapoton

Status: needs_reviewneeds_work

comment:11 Changed 5 years ago by git

Commit: bdb334a701f52802c48c58dc096c3c9cc190a96f81088b1db2760193beab507e9a369aa62f39a6fc

Branch pushed to git repo; I updated commit sha1. New commits:

0b31f81one cmp removed in doc
869cbd5Merge branch 'u/chapoton/23818' in 8.1.b7
81088b1trac 23818 fixing hash doctest

comment:12 Changed 5 years ago by git

Commit: 81088b1db2760193beab507e9a369aa62f39a6fc54846bf4f0758636eac705a45158ee21b6482703

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

54846bftrac 23818 adding hash

comment:13 Changed 5 years ago by Frédéric Chapoton

Status: needs_workneeds_review

comment:14 in reply to:  9 Changed 5 years ago by Erik Bray

Replying to chapoton:

Raah, you made me change the hash, and now the new hash does not make sense.. Can I set it back to use _repr_ ?

Well I didn't make you do it, but I think it makes more sense this way. In any case the actual numerical hash value is completely implementation and system dependent and shouldn't be tested unless you're actually testing the hash algorithm itself. Instead just ensure that two immutable(?) instances of the same class have the same hash--which it seems you did.

comment:15 Changed 5 years ago by Erik Bray

Status: needs_reviewpositive_review

comment:16 Changed 5 years ago by Volker Braun

Branch: u/chapoton/2381854846bf4f0758636eac705a45158ee21b6482703
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.