Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#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:

Status badges

Description

part of #24551

Change History (19)

comment:1 Changed 3 years ago by chapoton

  • Branch set to u/chapoton/26091
  • Commit set to 9f685f3db724a69dcc0e469949a196e858cb168e
  • Status changed from new to needs_review

New commits:

​9f685f3py3: adding hash for projective spaces

comment:2 Changed 3 years ago by git

  • Commit changed from 9f685f3db724a69dcc0e469949a196e858cb168e to 2caa7ae8d0ae8996eceb7cea1d6b539345c4f1e4

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

​2caa7aefixing one 64bits hash

comment:3 Changed 3 years ago by chapoton

  • 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:5 Changed 3 years ago by tmonteil

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 git

  • Commit changed from 2caa7ae8d0ae8996eceb7cea1d6b539345c4f1e4 to eafa8b6e66fad59630d023ebdf5dc61af5f73dbe

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

​eafa8b6fix hash for 32bit

comment:7 Changed 3 years ago by chapoton

Merci beaucoup !

comment:8 Changed 3 years ago by tscrim

Note that #23807 plans to solve this in a different way by making projective space use UniqueRepresentation.

comment:9 Changed 3 years ago by saraedum

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 saraedum

  • 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 tscrim

I expect this to be done and merged before #23807.

comment:12 Changed 3 years ago by git

  • Commit changed from eafa8b6e66fad59630d023ebdf5dc61af5f73dbe to a3bddad41a19d480475cdc761403d4b0b9d796b7

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

​a3bddaddetail in #26091 (hash doctest)

comment:13 Changed 3 years ago by chapoton

done (note that this is a case where the hash does not depend on the point..)

comment:14 Changed 3 years ago by chapoton

Everything seems to be ok. Positive review, somebody ?

comment:15 Changed 3 years ago by chapoton

  • Cc tscrim added

review, please

comment:16 Changed 3 years ago by chapoton

should we wait for #23807 ?

comment:17 Changed 3 years ago by tscrim

  • 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 vbraun

  • 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 embray

  • 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).

Note: See TracTickets for help on using tickets.