Opened 2 years ago

Closed 2 months ago

#23052 closed defect (fixed)

operations between sage and gmpy2 numbers

Reported by: vklein Owned by: vklein
Priority: major Milestone: sage-8.7
Component: packages: standard Keywords:
Cc: vdelecroix, jdemeyer Merged in:
Authors: Vincent Klein Reviewers: Vincent Delecroix
Report Upstream: Fixed upstream, in a later stable release. Work issues:
Branch: b7fbb9a (Commits) Commit: b7fbb9a4dac5d6882f6b83a57447dd79ecafb84c
Dependencies: Stopgaps:

Description (last modified by vklein)

Currently, operations involving Sage and gmpy2 numbers are broken (see 7). Though, it works fine with gmpy2 and Python numbers. The reason is that the various binary operations implemented in gmpy2 just check for Python integer types. It should be possible to modify these functions in order that an operation involving a gmpy2 number and a type implementing one of the __mpz__/__mpq__/__mpfr__/__mpc__ method should work.

Note: #22928 implemented the conversion Sage type -> gmpy2 type via the implementation of the __mpz__, __mpq__, __mpfr__ and __mpc__ methods.

Upstream issue: https://github.com/aleaxit/gmpy/issues/214

Change History (31)

comment:1 Changed 2 years ago by vklein

  • Description modified (diff)
  • Owner changed from (none) to vklein

comment:2 in reply to: ↑ description Changed 2 years ago by jdemeyer

Replying to vklein:

Provide coercion for gmpy2 numbers for operations -, +, * and /

This makes no sense to me. What does it mean to provide coercion for certain operations only?

comment:3 Changed 2 years ago by vklein

  • Description modified (diff)

You're right, description updated

comment:4 Changed 2 years ago by jdemeyer

  • Dependencies changed from 22928 to #22928

Thanks for the clarification.

comment:5 Changed 2 years ago by vdelecroix

  • Description modified (diff)
  • Summary changed from Provide coercion for gmpy2 numbers to coercion for gmpy2 numbers

comment:6 Changed 16 months ago by vdelecroix

  • Dependencies changed from #22928 to #22928, #23024
  • Description modified (diff)

comment:7 Changed 3 months ago by vdelecroix

I don't think that this feature is desirable. The behavior should be the same as with cypari2

sage: import cypari2
sage: pari = cypari2.Pari()
sage: type(pari(1) + 1)
<type 'cypari2.gen.Gen'>
sage: type(1 + pari(1))
<type 'cypari2.gen.Gen'>

Mixing gmpy2 numbers with sage numbers currently raise errors

sage: import gmpy2
sage: type(gmpy2.mpz(1) + 1)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-2-eeaeca1c122b> in <module>()
----> 1 type(gmpy2.mpz(Integer(1)) + Integer(1))

/opt/sage/local/lib/python2.7/site-packages/sage/rings/integer.pyx in sage.rings.integer.Integer.__add__ (build/cythonized/sage/rings/integer.c:11460)()
   1683             return y
   1684 
-> 1685         return coercion_model.bin_op(left, right, operator.add)
   1686 
   1687     cpdef _add_(self, right):

/opt/sage/local/lib/python2.7/site-packages/sage/structure/coerce.pyx in sage.structure.coerce.CoercionModel.bin_op (build/cythonized/sage/structure/coerce.c:11110)()
   1208         # We should really include the underlying error.
   1209         # This causes so much headache.
-> 1210         raise bin_op_exception(op, x, y)
   1211 
   1212     cpdef canonical_coercion(self, x, y):

TypeError: unsupported operand parent(s) for +: '<type 'mpz'>' and 'Integer Ring'
sage: type(1 + gmpy2.mpz(1))
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-3-ea356a02d58b> in <module>()
----> 1 type(Integer(1) + gmpy2.mpz(Integer(1)))

/opt/sage/local/lib/python2.7/site-packages/sage/rings/integer.pyx in sage.rings.integer.Integer.__add__ (build/cythonized/sage/rings/integer.c:11460)()
   1683             return y
   1684 
-> 1685         return coercion_model.bin_op(left, right, operator.add)
   1686 
   1687     cpdef _add_(self, right):

/opt/sage/local/lib/python2.7/site-packages/sage/structure/coerce.pyx in sage.structure.coerce.CoercionModel.bin_op (build/cythonized/sage/structure/coerce.c:11110)()
   1208         # We should really include the underlying error.
   1209         # This causes so much headache.
-> 1210         raise bin_op_exception(op, x, y)
   1211 
   1212     cpdef canonical_coercion(self, x, y):

TypeError: unsupported operand parent(s) for +: 'Integer Ring' and '<type 'mpz'>'

comment:8 follow-up: Changed 3 months ago by vdelecroix

To make this work, one has to modify the various binary operators in gmpy2 (e.g. GMPy_Integer_Add).

comment:9 in reply to: ↑ 8 Changed 3 months ago by jdemeyer

Replying to vdelecroix:

To make this work, one has to modify the various binary operators in gmpy2 (e.g. GMPy_Integer_Add).

This is obviously the right thing to do. Given that

sage: from gmpy2 import mpz
sage: mpz(1) + int(1)
mpz(2)

works, it's actually strange that

sage: from gmpy2 import mpz
sage: mpz(1) + Integer(1)
Traceback (most recent call last):
...
TypeError: unsupported operand parent(s) for +: '<type 'mpz'>' and 'Integer Ring'

doesn't work.

comment:10 Changed 3 months ago by vdelecroix

Let me update the ticket description then.

comment:11 Changed 3 months ago by vdelecroix

  • Component changed from packages: optional to packages: standard
  • Description modified (diff)
  • Report Upstream changed from N/A to Not yet reported upstream; Will do shortly.
  • Summary changed from coercion for gmpy2 numbers to operations between sage and gmpy2 numbers
  • Type changed from enhancement to defect

comment:12 Changed 3 months ago by vklein

  • Description modified (diff)
  • Report Upstream changed from Not yet reported upstream; Will do shortly. to Reported upstream. No feedback yet.

comment:13 Changed 3 months ago by vklein

  • Report Upstream changed from Reported upstream. No feedback yet. to Fixed upstream, in a later stable release.

comment:14 follow-up: Changed 3 months ago by vklein

The gmpy2 pull request #217 has been merged this morning.
Should we make a patch based on this or wait for the next gmpy2 release ?

comment:15 in reply to: ↑ 14 ; follow-up: Changed 3 months ago by vdelecroix

Replying to vklein:

The gmpy2 pull request #217 has been merged this morning.

This is a great news. Thanks for the work!

Should we make a patch based on this or wait for the next gmpy2 release ?

Do we have a schedule for the next gmpy2 release? Since gmpy2 becomes a standard package for the next Sage release it would be nice if it would happen before.

Otherwise, I am not a big fan of using patches that are integrated upstream but not released yet.

comment:16 in reply to: ↑ 15 Changed 3 months ago by vdelecroix

Replying to vdelecroix:

Replying to vklein:

Should we make a patch based on this or wait for the next gmpy2 release ?

Do we have a schedule for the next gmpy2 release? Since gmpy2 becomes a standard package for the next Sage release it would be nice if it would happen before.

Somehow #199 is a bit of an answer.

comment:17 Changed 3 months ago by vklein

  • Branch set to u/vklein/23052

comment:18 Changed 3 months ago by vklein

  • Commit set to edfe875c099ab48145b976de2dcc2d70fff0f1b5
  • Status changed from new to needs_review

After some talk with vdelecroix we agree to make a gmpy2 patch with #217.


New commits:

edfe875Trac #23052: patch gmpy2 with PR #217 ...

comment:19 Changed 3 months ago by vklein

  • Dependencies #22928, #23024 deleted

comment:20 Changed 3 months ago by vklein

  • Milestone changed from sage-8.0 to sage-8.7

comment:21 Changed 3 months ago by vdelecroix

  • Status changed from needs_review to needs_work

You create a lot of warnings with the macros. This should be fixed.

gmpy2-2.1.0a4.p1]     src/gmpy2_cache.c: In function 'GMPy_MPZ_NewInit':
[gmpy2-2.1.0a4.p1]     src/gmpy2_convert.h:44:60: warning: suggest parentheses around '&&' within '||' [-Wparentheses]
[gmpy2-2.1.0a4.p1]      #define HAS_STRICT_MPZ_CONVERSION(x) HAS_MPZ_CONVERSION(x) && \
[gmpy2-2.1.0a4.p1]                                                                 ^
[gmpy2-2.1.0a4.p1]     src/gmpy2_convert.h:52:25: note: in expansion of macro 'HAS_STRICT_MPZ_CONVERSION'
[gmpy2-2.1.0a4.p1]                              HAS_STRICT_MPZ_CONVERSION(x))
[gmpy2-2.1.0a4.p1]                              ^
[gmpy2-2.1.0a4.p1]     src/gmpy2_convert.h:61:25: note: in expansion of macro 'IS_INTEGER'
[gmpy2-2.1.0a4.p1]      #define IS_RATIONAL(x) (IS_INTEGER(x) || IS_RATIONAL_ONLY(x))
[gmpy2-2.1.0a4.p1]                              ^
[gmpy2-2.1.0a4.p1]     src/gmpy2_convert.h:64:21: note: in expansion of macro 'IS_RATIONAL'
[gmpy2-2.1.0a4.p1]      #define IS_REAL(x) (IS_RATIONAL(x) || IS_REAL_ONLY(x))
[gmpy2-2.1.0a4.p1]                          ^
[gmpy2-2.1.0a4.p1]     src/gmpy2_cache.c:191:9: note: in expansion of macro 'IS_REAL'
[gmpy2-2.1.0a4.p1]          if (IS_REAL(n)) {
[gmpy2-2.1.0a4.p1]              ^
[gmpy2-2.1.0a4.p1]     src/gmpy2_convert.h:46:62: warning: suggest parentheses around '&&' within '||' [-Wparentheses]
[gmpy2-2.1.0a4.p1]      #define HAS_STRICT_MPFR_CONVERSION(x) HAS_MPFR_CONVERSION(x) && \
[gmpy2-2.1.0a4.p1]                                                                   ^
[gmpy2-2.1.0a4.p1]     src/gmpy2_convert.h:63:63: note: in expansion of macro 'HAS_STRICT_MPFR_CONVERSION'
[gmpy2-2.1.0a4.p1]      #define IS_REAL_ONLY(x) (MPFR_Check(x) || PyFloat_Check(x) || HAS_STRICT_MPFR_CONVERSION(x))
[gmpy2-2.1.0a4.p1]                                                                    ^
[gmpy2-2.1.0a4.p1]     src/gmpy2_convert.h:64:39: note: in expansion of macro 'IS_REAL_ONLY'
[gmpy2-2.1.0a4.p1]      #define IS_REAL(x) (IS_RATIONAL(x) || IS_REAL_ONLY(x))
[gmpy2-2.1.0a4.p1]                                            ^
[gmpy2-2.1.0a4.p1]     src/gmpy2_cache.c:191:9: note: in expansion of macro 'IS_REAL'
[gmpy2-2.1.0a4.p1]          if (IS_REAL(n)) {
[gmpy2-2.1.0a4.p1]              ^

comment:22 Changed 2 months ago by git

  • Commit changed from edfe875c099ab48145b976de2dcc2d70fff0f1b5 to b60be6d6e89e491cf5e9edc8bf30964b75921643

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

b60be6dTrac #23052: Fix thousands lines of warnings.

comment:23 Changed 2 months ago by vklein

  • Status changed from needs_work to needs_review

Warnings fixed.

comment:24 Changed 2 months ago by vdelecroix

  • Status changed from needs_review to needs_work

some failures...

Doctesting 3797 files using 14 threads.
sage -t --long src/sage/arith/misc.py
**********************************************************************
File "src/sage/arith/misc.py", line 3120, in sage.arith.misc.crt
Failed example:
    crt(mpz(2), mpz(3), mpz(7), mpz(11))
Expected:
    58
Got:
    mpz(58)
**********************************************************************
File "src/sage/arith/misc.py", line 3122, in sage.arith.misc.crt
Failed example:
    crt(mpz(2), 3, mpz(7), numpy.int8(11))
Expected:
    58
Got:
    mpz(58)
**********************************************************************
1 item had failures:
   2 of  37 in sage.arith.misc.crt
    [1056 tests, 2 failures, 30.02 s]
----------------------------------------------------------------------
sage -t --long src/sage/arith/misc.py  # 2 doctests failed
----------------------------------------------------------------------

comment:25 Changed 2 months ago by git

  • Commit changed from b60be6d6e89e491cf5e9edc8bf30964b75921643 to 5dff83b8730264b6d41edb177dfc5b47b5ea1be4

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

5dff83bTrac #23052: arith.misc.crt ensure this function

comment:26 Changed 2 months ago by vklein

  • Status changed from needs_work to needs_review

function sage.arith.misc.crt fixed

comment:27 Changed 2 months ago by vdelecroix

The patch that you called PR217.patch comes from two pull requests (#217 and #218). You would better change the name of the patch and mention very explicitely in the header of the patch the urls of the actual pull requests.

comment:28 Changed 2 months ago by git

  • Commit changed from 5dff83b8730264b6d41edb177dfc5b47b5ea1be4 to b7fbb9a4dac5d6882f6b83a57447dd79ecafb84c

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

b7fbb9aTrac #23052: Rename patch and add links to the...

comment:29 Changed 2 months ago by vklein

Patch renamed and links added.

comment:30 Changed 2 months ago by vdelecroix

  • Reviewers set to Vincent Delecroix
  • Status changed from needs_review to positive_review

comment:31 Changed 2 months ago by vbraun

  • Branch changed from u/vklein/23052 to b7fbb9a4dac5d6882f6b83a57447dd79ecafb84c
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.