#26091 closed enhancement (fixed)
py3: hash for projective space
Reported by: | chapoton | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-8.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 4 years ago by
- Branch set to u/chapoton/26091
- Commit set to 9f685f3db724a69dcc0e469949a196e858cb168e
- Status changed from new to needs_review
comment:2 Changed 4 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 4 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 # 32-bit
comment:4 Changed 4 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.0-6-686/tmonteil-debian-stretch-32/2018-08-19%2011:15:44
comment:5 Changed 4 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 4 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 4 years ago by
Merci beaucoup !
comment:8 Changed 4 years ago by
Note that #23807 plans to solve this in a different way by making projective space use UniqueRepresentation
.
comment:9 Changed 4 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 4 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 4 years ago by
I expect this to be done and merged before #23807.
comment:12 Changed 4 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 4 years ago by
done (note that this is a case where the hash does not depend on the point..)
comment:14 Changed 4 years ago by
Everything seems to be ok. Positive review, somebody ?
comment:16 Changed 4 years ago by
should we wait for #23807 ?
comment:17 Changed 4 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 4 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 4 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 hack--never got around to documenting it or anything, so this is better).
New commits:
py3: adding hash for projective spaces