search

sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.32k stars 453 forks source link

operations between sage and gmpy2 numbers #23052

Closed 4a781b2b-eada-4ae4-9b46-f6cbb63459b9 closed 5 years ago

4a781b2b-eada-4ae4-9b46-f6cbb63459b9 commented 7 years ago

Currently, operations involving Sage and gmpy2 numbers are broken (see [comment: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

Upstream: Fixed upstream, in a later stable release.

CC: @videlec @jdemeyer

Component: packages: standard

Author: Vincent Klein

Branch/Commit: b7fbb9a

Reviewer: Vincent Delecroix

Issue created by migration from https://trac.sagemath.org/ticket/23052

4a781b2b-eada-4ae4-9b46-f6cbb63459b9 commented 7 years ago

Description changed:

--- 
+++ 
@@ -1,2 +1,3 @@
 Provide coercion for gmpy2 numbers for operations -, +, * and /
+
 This ticket follow the discussion about coercion for gmpy2 numbers #22928.
jdemeyer commented 7 years ago

Replying to @vinklein:

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?

4a781b2b-eada-4ae4-9b46-f6cbb63459b9 commented 7 years ago

Description changed:

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

 This ticket follow the discussion about coercion for gmpy2 numbers #22928.
4a781b2b-eada-4ae4-9b46-f6cbb63459b9 commented 7 years ago
comment:3

You're right, description updated

jdemeyer commented 7 years ago

Changed dependencies from 22928 to #22928

jdemeyer commented 7 years ago
comment:4

Thanks for the clarification.

videlec commented 7 years ago

Description changed:

--- 
+++ 
@@ -1,3 +1,3 @@
-Provide coercion for gmpy2 numbers
-
-This ticket follow the discussion about coercion for gmpy2 numbers #22928.
+This ticket follows the discussion about coercion for gmpy2 numbers #22928. It is not clear:
+- whether this coercion is desirable
+- in which direction coercion should be done (Sage -> gmpy2 or gmpy2 -> Sage)
videlec commented 6 years ago

Changed dependencies from #22928 to #22928, #23024

videlec commented 6 years ago

Description changed:

--- 
+++ 
@@ -1,3 +1,5 @@
-This ticket follows the discussion about coercion for gmpy2 numbers #22928. It is not clear:
-- whether this coercion is desirable
-- in which direction coercion should be done (Sage -> gmpy2 or gmpy2 -> Sage)
+This ticket follows the discussion about coercion for gmpy2 numbers #22928 (see also #23024 that proposes to make `gmpy2` a standard package). As it is the case with Python types, we implement coercion from gmpy2 types to the corresponding Sage types
+- `gmpy2.mpz` -> `Integer`
+- `gmpy2.mpq` -> `Rational`
+- `gmpy2.mpfr` -> `RealNumber`
+- `gmpy2.mpc` -> `MPComplexNumber`
videlec commented 5 years ago
comment:7

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'>'
videlec commented 5 years ago
comment:8

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

jdemeyer commented 5 years ago
comment:9

Replying to @videlec:

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.

videlec commented 5 years ago
comment:10

Let me update the ticket description then.

videlec commented 5 years ago

Upstream: Not yet reported upstream; Will do shortly.

videlec commented 5 years ago

Description changed:

--- 
+++ 
@@ -1,5 +1,3 @@
-This ticket follows the discussion about coercion for gmpy2 numbers #22928 (see also #23024 that proposes to make `gmpy2` a standard package). As it is the case with Python types, we implement coercion from gmpy2 types to the corresponding Sage types
-- `gmpy2.mpz` -> `Integer`
-- `gmpy2.mpq` -> `Rational`
-- `gmpy2.mpfr` -> `RealNumber`
-- `gmpy2.mpc` -> `MPComplexNumber`
+Currently, operations involving Sage and gmpy2 numbers are broken (see [comment: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.
4a781b2b-eada-4ae4-9b46-f6cbb63459b9 commented 5 years ago

Description changed:

--- 
+++ 
@@ -1,3 +1,5 @@
 Currently, operations involving Sage and gmpy2 numbers are broken (see [comment: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
4a781b2b-eada-4ae4-9b46-f6cbb63459b9 commented 5 years ago

Changed upstream from Not yet reported upstream; Will do shortly. to Reported upstream. No feedback yet.

4a781b2b-eada-4ae4-9b46-f6cbb63459b9 commented 5 years ago

Changed upstream from Reported upstream. No feedback yet. to Fixed upstream, in a later stable release.

4a781b2b-eada-4ae4-9b46-f6cbb63459b9 commented 5 years ago
comment:14

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 ?

videlec commented 5 years ago
comment:15

Replying to @vinklein:

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.

videlec commented 5 years ago
comment:16

Replying to @videlec:

Replying to @vinklein:

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.

4a781b2b-eada-4ae4-9b46-f6cbb63459b9 commented 5 years ago

Branch: u/vklein/23052

4a781b2b-eada-4ae4-9b46-f6cbb63459b9 commented 5 years ago
comment:18

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

New commits:

edfe875Trac #23052: patch gmpy2 with PR #217 ...
4a781b2b-eada-4ae4-9b46-f6cbb63459b9 commented 5 years ago

Commit: edfe875

4a781b2b-eada-4ae4-9b46-f6cbb63459b9 commented 5 years ago

Changed dependencies from #22928, #23024 to none

videlec commented 5 years ago
comment:21

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]              ^
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

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

b60be6dTrac #23052: Fix thousands lines of warnings.
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

Changed commit from edfe875 to b60be6d

4a781b2b-eada-4ae4-9b46-f6cbb63459b9 commented 5 years ago
comment:23

Warnings fixed.

videlec commented 5 years ago
comment:24

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
----------------------------------------------------------------------
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

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

5dff83bTrac #23052: arith.misc.crt ensure this function
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

Changed commit from b60be6d to 5dff83b

4a781b2b-eada-4ae4-9b46-f6cbb63459b9 commented 5 years ago
comment:26

function sage.arith.misc.crt fixed

videlec commented 5 years ago
comment:27

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.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

Changed commit from 5dff83b to b7fbb9a

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

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

b7fbb9aTrac #23052: Rename patch and add links to the...
4a781b2b-eada-4ae4-9b46-f6cbb63459b9 commented 5 years ago
comment:29

Patch renamed and links added.

videlec commented 5 years ago

Reviewer: Vincent Delecroix

vbraun commented 5 years ago

Changed branch from u/vklein/23052 to b7fbb9a