Opened 2 years ago
Closed 2 years ago
#28219 closed defect (fixed)
add a doctest for pynac's rational hash function
| Reported by: | gh-mwageringel | Owned by: | |
|---|---|---|---|
| Priority: | major | Milestone: | sage-8.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: |
Status badges
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^64-1) == hash(SR(2^64-1)) False
Change History (13)
comment:1 Changed 2 years ago by
- Branch set to u/gh-mwageringel/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 2 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 2 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 2 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 2 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 2 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 2 years ago by
That would be fine by me.
comment:8 Changed 2 years ago by
- Reviewers set to John Palmieri
- Status changed from needs_work to positive_review
comment:9 Changed 2 years ago by
See #28412.
comment:10 Changed 2 years ago by
- Status changed from positive_review to needs_review
Back from positive, to avoid conflicts with #28412.
comment:11 Changed 2 years ago by
- Branch changed from u/gh-mwageringel/28219 to u/gh-mwageringel/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 2 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 2 years ago by
- Branch changed from u/gh-mwageringel/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 from-1to-2is applied. It is important that_mpq_pythonhashobtains 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