#26091 closed enhancement (fixed)
py3: hash for projective space
Reported by:  chapoton  Owned by:  

Priority:  major  Milestone:  sage8.4 
Component:  python3  Keywords:  
Cc:  tmonteil, tscrim  Merged in:  
Authors:  Frédéric Chapoton  Reviewers:  Julian Rüth, Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  a3bddad (Commits, GitHub, GitLab)  Commit:  
Dependencies:  Stopgaps: 
Description
part of #24551
Change History (19)
comment:1 Changed 3 years ago by
 Branch set to u/chapoton/26091
 Commit set to 9f685f3db724a69dcc0e469949a196e858cb168e
 Status changed from new to needs_review
comment:2 Changed 3 years ago by
 Commit changed from 9f685f3db724a69dcc0e469949a196e858cb168e to 2caa7ae8d0ae8996eceb7cea1d6b539345c4f1e4
Branch pushed to git repo; I updated commit sha1. New commits:
2caa7ae  fixing one 64bits hash

comment:3 Changed 3 years ago by
 Cc tmonteil added
@tmonteil, could you please help me fix the 32 bits hash, by running the following doctest with this branch on your 32 bit machine and reporting the result ?
sage: P.<x,y> = ProjectiveSpace(Zmod(10), 1) sage: hash(P([2, 5])) 479010389 # 32bit
comment:4 Changed 3 years ago by
I stopped my patchbot and restarted it on this ticket, see https://patchbot.sagemath.org/log/Pending/26091/debian/9.4/i686/4.9.06686/tmonteildebianstretch32/20180819%2011:15:44
comment:5 Changed 3 years ago by
And the result is:
sage t long src/sage/schemes/projective/projective_point.py ********************************************************************** File "src/sage/schemes/projective/projective_point.py", line 402, in sage.schemes.projective.projective_point.SchemeMorphism_point_projective_ring.__hash__ Failed example: hash(P([2, 5])) Expected: 479010389 Got: 2047536788 **********************************************************************
comment:6 Changed 3 years ago by
 Commit changed from 2caa7ae8d0ae8996eceb7cea1d6b539345c4f1e4 to eafa8b6e66fad59630d023ebdf5dc61af5f73dbe
Branch pushed to git repo; I updated commit sha1. New commits:
eafa8b6  fix hash for 32bit

comment:7 Changed 3 years ago by
Merci beaucoup !
comment:8 Changed 3 years ago by
Note that #23807 plans to solve this in a different way by making projective space use UniqueRepresentation
.
comment:9 Changed 3 years ago by
Would you mind replacing the sage: hash(P([2, 5]))
with sage: hash(P([2, 5])) == hash(P([2, 5]))
? I find these 32/64 bit tests really annoying because you cannot update these easily.
comment:10 Changed 3 years ago by
 Reviewers set to Julian Rüth
If you changed that, it would be a positive review from me. But I haven't looked at #23807, so I leave it to you to figure out whether that one should go in instead.
comment:11 Changed 3 years ago by
I expect this to be done and merged before #23807.
comment:12 Changed 3 years ago by
 Commit changed from eafa8b6e66fad59630d023ebdf5dc61af5f73dbe to a3bddad41a19d480475cdc761403d4b0b9d796b7
Branch pushed to git repo; I updated commit sha1. New commits:
a3bddad  detail in #26091 (hash doctest)

comment:13 Changed 3 years ago by
done (note that this is a case where the hash does not depend on the point..)
comment:14 Changed 3 years ago by
Everything seems to be ok. Positive review, somebody ?
comment:16 Changed 3 years ago by
should we wait for #23807 ?
comment:17 Changed 3 years ago by
 Reviewers changed from Julian Rüth to Julian Rüth, Travis Scrimshaw
 Status changed from needs_review to positive_review
No, let's just get this in and rebase #23807 on this.
comment:18 Changed 3 years ago by
 Branch changed from u/chapoton/26091 to a3bddad41a19d480475cdc761403d4b0b9d796b7
 Resolution set to fixed
 Status changed from positive_review to closed
comment:19 Changed 3 years ago by
 Commit a3bddad41a19d480475cdc761403d4b0b9d796b7 deleted
Ah, I just noticed this one. Thanks. I was just starting to fix this myself but then I remembered to check first (actually I had already added a hash for this class a while ago, but only as a quick hacknever got around to documenting it or anything, so this is better).
New commits:
py3: adding hash for projective spaces