Opened 10 months ago

Closed 9 months 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) Commit: 1ad2bfbed1f67c4d35375173553fec56564a7374
Dependencies: #28412 Stopgaps:

Description (last modified by gh-mwageringel)

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 10 months ago by gh-mwageringel

  • Authors set to Markus Wageringel
  • 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

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 -1 to -2 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:

​835288528219: patch Pynac's rational and integer hash functions

comment:2 Changed 10 months ago by gh-mwageringel

  • 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 10 months ago by rws

  • 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 10 months ago by gh-mwageringel

  • 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 10 months ago by jhpalmieri

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 9 months ago by jhpalmieri

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 9 months ago by gh-mwageringel

That would be fine by me.

comment:8 Changed 9 months ago by jhpalmieri

  • Reviewers set to John Palmieri
  • Status changed from needs_work to positive_review

comment:9 Changed 9 months ago by rws

See #28412.

comment:10 Changed 9 months ago by rws

  • Status changed from positive_review to needs_review

Back from positive, to avoid conflicts with #28412.

comment:11 Changed 9 months ago by gh-mwageringel

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

​1ad2bfb28219: add doctest for Pynac's rational hash function

comment:12 Changed 9 months ago by jhpalmieri

  • 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 9 months ago by vbraun

  • Branch changed from u/gh-mwageringel/28219v2 to 1ad2bfbed1f67c4d35375173553fec56564a7374
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.