Opened 3 years ago
Closed 3 years ago
#28219 closed defect (fixed)
add a doctest for pynac's rational hash function
Reported by:  ghmwageringel  Owned by:  

Priority:  major  Milestone:  sage8.9 
Component:  packages: standard  Keywords:  
Cc:  Merged in:  
Authors:  Markus Wageringel  Reviewers:  John Palmieri 
Report Upstream:  Fixed upstream, in a later stable release.  Work issues:  
Branch:  1ad2bfb (Commits, GitHub, GitLab)  Commit:  1ad2bfbed1f67c4d35375173553fec56564a7374 
Dependencies:  #28412  Stopgaps: 
Description (last modified by )
The upgrade to Pynac 0.7.26 #28412 resolves the following issues with hashes in SR
. This ticket adds a doctest for this.
This fails with Python 2 and 3:
sage: hash(1/3) == hash(SR(1/3)) False
This only fails with Python 3:
sage: hash(2^641) == hash(SR(2^641)) False
Change History (13)
comment:1 Changed 3 years ago by
 Branch set to u/ghmwageringel/28219
 Commit set to 83528857bfe969fcb717b954daf2653e0fa7789f
 Report Upstream changed from N/A to Not yet reported upstream; Will do shortly.
 Status changed from new to needs_review
 Summary changed from patch pynac's rational and integer hash function to patch pynac's rational and integer hash functions
comment:2 Changed 3 years ago by
 Report Upstream changed from Not yet reported upstream; Will do shortly. to Completely fixed; Fix reported upstream
The tests pass. Upstream PR: https://github.com/pynac/pynac/pull/344
comment:3 Changed 3 years ago by
 Report Upstream changed from Completely fixed; Fix reported upstream to Fixed upstream, in a later stable release.
Merged. It will be in the next release which I'll put in an update ticket soon. Thanks!
comment:4 Changed 3 years ago by
 Status changed from needs_review to needs_work
 Work issues set to update pynac
Thank you. I will then remove the patch from this ticket, but will keep the new doctest so that it gets added to Sage.
comment:5 Changed 3 years ago by
If a new release of pynac will take a while, we can add the patch here and then remove it when a new pynac appears. This would fix one of the few remaining Python 3 problems. Any ideas when a new release will be available?
comment:6 Changed 3 years ago by
I propose merging this ticket now, so that we can fix this doctest. This would make immediate progress on Python 3. Opinions?
comment:7 Changed 3 years ago by
That would be fine by me.
comment:8 Changed 3 years ago by
 Reviewers set to John Palmieri
 Status changed from needs_work to positive_review
comment:9 Changed 3 years ago by
See #28412.
comment:10 Changed 3 years ago by
 Status changed from positive_review to needs_review
Back from positive, to avoid conflicts with #28412.
comment:11 Changed 3 years ago by
 Branch changed from u/ghmwageringel/28219 to u/ghmwageringel/28219v2
 Commit changed from 83528857bfe969fcb717b954daf2653e0fa7789f to 1ad2bfbed1f67c4d35375173553fec56564a7374
 Dependencies set to #28412
 Description modified (diff)
 Summary changed from patch pynac's rational and integer hash functions to add a doctest for pynac's rational hash function
 Work issues update pynac deleted
I have removed the patch from the branch, but kept the new doctest. This needs review again.
New commits:
1ad2bfb  28219: add doctest for Pynac's rational hash function

comment:12 Changed 3 years ago by
 Status changed from needs_review to positive_review
With #28412, all tests pass with Python 2, and with Python 3 the new test passes, and no new failures occur.
comment:13 Changed 3 years ago by
 Branch changed from u/ghmwageringel/28219v2 to 1ad2bfbed1f67c4d35375173553fec56564a7374
 Resolution set to fixed
 Status changed from positive_review to closed
Here is a tentative patch I would like to get tested. The implementation is taken directly from
sage.libs.gmp.pylong.mpz_pythonhash
. The only difference is in how the final mapping from1
to2
is applied. It is important that_mpq_pythonhash
obtains the hash values for numerator and denominator before this mapping is applied, as that is what happens in Sage.New commits:
28219: patch Pynac's rational and integer hash functions