Opened 5 years ago
Closed 3 years ago
#23052 closed defect (fixed)
operations between sage and gmpy2 numbers
Reported by:  vklein  Owned by:  vklein 

Priority:  major  Milestone:  sage8.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, GitHub, GitLab)  Commit:  b7fbb9a4dac5d6882f6b83a57447dd79ecafb84c 
Dependencies:  Stopgaps: 
Description (last modified by )
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 5 years ago by
 Description modified (diff)
 Owner changed from (none) to vklein
comment:2 in reply to: ↑ description Changed 5 years ago by
comment:4 Changed 5 years ago by
 Dependencies changed from 22928 to #22928
Thanks for the clarification.
comment:5 Changed 5 years ago by
 Description modified (diff)
 Summary changed from Provide coercion for gmpy2 numbers to coercion for gmpy2 numbers
comment:6 Changed 5 years ago by
 Dependencies changed from #22928 to #22928, #23024
 Description modified (diff)
comment:7 Changed 3 years ago by
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) <ipythoninput2eeaeca1c122b> in <module>() > 1 type(gmpy2.mpz(Integer(1)) + Integer(1)) /opt/sage/local/lib/python2.7/sitepackages/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/sitepackages/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) <ipythoninput3ea356a02d58b> in <module>() > 1 type(Integer(1) + gmpy2.mpz(Integer(1))) /opt/sage/local/lib/python2.7/sitepackages/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/sitepackages/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 followup: ↓ 9 Changed 3 years ago by
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 years ago by
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 years ago by
Let me update the ticket description then.
comment:11 Changed 3 years ago by
 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 years ago by
 Description modified (diff)
 Report Upstream changed from Not yet reported upstream; Will do shortly. to Reported upstream. No feedback yet.
comment:13 Changed 3 years ago by
 Report Upstream changed from Reported upstream. No feedback yet. to Fixed upstream, in a later stable release.
comment:14 followup: ↓ 15 Changed 3 years ago by
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 ; followup: ↓ 16 Changed 3 years ago by
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 years ago by
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 years ago by
 Branch set to u/vklein/23052
comment:18 Changed 3 years ago by
 Commit set to edfe875c099ab48145b976de2dcc2d70fff0f1b5
 Status changed from new to needs_review
comment:19 Changed 3 years ago by
 Dependencies #22928, #23024 deleted
comment:20 Changed 3 years ago by
 Milestone changed from sage8.0 to sage8.7
comment:21 Changed 3 years ago by
 Status changed from needs_review to needs_work
You create a lot of warnings with the macros. This should be fixed.
gmpy22.1.0a4.p1] src/gmpy2_cache.c: In function 'GMPy_MPZ_NewInit': [gmpy22.1.0a4.p1] src/gmpy2_convert.h:44:60: warning: suggest parentheses around '&&' within '' [Wparentheses] [gmpy22.1.0a4.p1] #define HAS_STRICT_MPZ_CONVERSION(x) HAS_MPZ_CONVERSION(x) && \ [gmpy22.1.0a4.p1] ^ [gmpy22.1.0a4.p1] src/gmpy2_convert.h:52:25: note: in expansion of macro 'HAS_STRICT_MPZ_CONVERSION' [gmpy22.1.0a4.p1] HAS_STRICT_MPZ_CONVERSION(x)) [gmpy22.1.0a4.p1] ^ [gmpy22.1.0a4.p1] src/gmpy2_convert.h:61:25: note: in expansion of macro 'IS_INTEGER' [gmpy22.1.0a4.p1] #define IS_RATIONAL(x) (IS_INTEGER(x)  IS_RATIONAL_ONLY(x)) [gmpy22.1.0a4.p1] ^ [gmpy22.1.0a4.p1] src/gmpy2_convert.h:64:21: note: in expansion of macro 'IS_RATIONAL' [gmpy22.1.0a4.p1] #define IS_REAL(x) (IS_RATIONAL(x)  IS_REAL_ONLY(x)) [gmpy22.1.0a4.p1] ^ [gmpy22.1.0a4.p1] src/gmpy2_cache.c:191:9: note: in expansion of macro 'IS_REAL' [gmpy22.1.0a4.p1] if (IS_REAL(n)) { [gmpy22.1.0a4.p1] ^ [gmpy22.1.0a4.p1] src/gmpy2_convert.h:46:62: warning: suggest parentheses around '&&' within '' [Wparentheses] [gmpy22.1.0a4.p1] #define HAS_STRICT_MPFR_CONVERSION(x) HAS_MPFR_CONVERSION(x) && \ [gmpy22.1.0a4.p1] ^ [gmpy22.1.0a4.p1] src/gmpy2_convert.h:63:63: note: in expansion of macro 'HAS_STRICT_MPFR_CONVERSION' [gmpy22.1.0a4.p1] #define IS_REAL_ONLY(x) (MPFR_Check(x)  PyFloat_Check(x)  HAS_STRICT_MPFR_CONVERSION(x)) [gmpy22.1.0a4.p1] ^ [gmpy22.1.0a4.p1] src/gmpy2_convert.h:64:39: note: in expansion of macro 'IS_REAL_ONLY' [gmpy22.1.0a4.p1] #define IS_REAL(x) (IS_RATIONAL(x)  IS_REAL_ONLY(x)) [gmpy22.1.0a4.p1] ^ [gmpy22.1.0a4.p1] src/gmpy2_cache.c:191:9: note: in expansion of macro 'IS_REAL' [gmpy22.1.0a4.p1] if (IS_REAL(n)) { [gmpy22.1.0a4.p1] ^
comment:22 Changed 3 years ago by
 Commit changed from edfe875c099ab48145b976de2dcc2d70fff0f1b5 to b60be6d6e89e491cf5e9edc8bf30964b75921643
Branch pushed to git repo; I updated commit sha1. New commits:
b60be6d  Trac #23052: Fix thousands lines of warnings.

comment:24 Changed 3 years ago by
 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 3 years ago by
 Commit changed from b60be6d6e89e491cf5e9edc8bf30964b75921643 to 5dff83b8730264b6d41edb177dfc5b47b5ea1be4
Branch pushed to git repo; I updated commit sha1. New commits:
5dff83b  Trac #23052: arith.misc.crt ensure this function

comment:26 Changed 3 years ago by
 Status changed from needs_work to needs_review
function sage.arith.misc.crt
fixed
comment:27 Changed 3 years ago by
comment:28 Changed 3 years ago by
 Commit changed from 5dff83b8730264b6d41edb177dfc5b47b5ea1be4 to b7fbb9a4dac5d6882f6b83a57447dd79ecafb84c
Branch pushed to git repo; I updated commit sha1. New commits:
b7fbb9a  Trac #23052: Rename patch and add links to the...

comment:29 Changed 3 years ago by
Patch renamed and links added.
comment:30 Changed 3 years ago by
 Reviewers set to Vincent Delecroix
 Status changed from needs_review to positive_review
comment:31 Changed 3 years ago by
 Branch changed from u/vklein/23052 to b7fbb9a4dac5d6882f6b83a57447dd79ecafb84c
 Resolution set to fixed
 Status changed from positive_review to closed
Replying to vklein:
This makes no sense to me. What does it mean to provide coercion for certain operations only?