#22928 closed enhancement (fixed)
Conversion between gmpy2 and sage objects
Reported by:  vklein  Owned by:  vklein 

Priority:  major  Milestone:  sage8.2 
Component:  interfaces  Keywords:  thursdaysbdx 
Cc:  Vincent Delecroix  Merged in:  
Authors:  Vincent Klein  Reviewers:  Jeroen Demeyer, Vincent Delecroix 
Report Upstream:  N/A  Work issues:  
Branch:  e9a6fda (Commits, GitHub, GitLab)  Commit:  
Dependencies:  #24549  Stopgaps: 
Description (last modified by )
The library gmpy2
supports conversions by the use of special methods. In order to allow conversion of Sage objects into gmpy2 objects, we implement the following methods
__mpz__
on Sage Integer__mpq__
on Sage Rational__mpfr__
on relevant Sage real numbersRealNumber
(sage.rings.real_mpfr
)RealDoubleElement
(sage.rings.real_double
)
__mpc__
on relevant Sage complex numbersComplexDoubleElement
(sage.rings.complex_double
)MPComplexNumber
(sage.rings.complex_mpc
)ComplexNumber
(sage.rings.complex_number
)
Conversly, we make Sage integer, rational, real and complex constructors accept gmpy2 arguments.
Upstream issues:
Change History (173)
comment:1 Changed 5 years ago by
Dependencies:  → #22927 

comment:2 Changed 5 years ago by
Owner:  set to vklein 

comment:3 Changed 5 years ago by
Keywords:  jsb++ added 

comment:4 Changed 5 years ago by
Keywords:  thursdaysbdx added; jsb++ removed 

comment:5 Changed 5 years ago by
Description:  modified (diff) 

comment:6 Changed 5 years ago by
Branch:  → u/vklein/conversion_between_gmpy2_and_sage_integers 

comment:7 Changed 5 years ago by
Commit:  → 48daf5857197f66b880a563ce27c34cf21959c87 

comment:8 followup: 10 Changed 5 years ago by
Looks good. Some small remarks.
1) For the error about missing gmpy2
you can use PackageNotFoundError
from sage.misc.package
2) add __mpz__() to class Rational
should be add __mpq__() to class Rational
3) You have an indentation problem here
Coercion for gmpy2 numbers sage: from gmpy2 import * # optional  gmpy2 sage: a, b = cm.canonical_coercion(mpz(5), 10) # optional  gmpy2 sage: a, b # optional  gmpy2
It should be
Coercion for gmpy2 numbers:: sage: from gmpy2 import * # optional  gmpy2 sage: a, b = cm.canonical_coercion(mpz(5), 10) # optional  gmpy2 sage: a, b # optional  gmpy2
4) There should be tests for a + b
, a * b
, a / b
when one of a
or b
is from Sage and the other from gmpy2
5) Couldn't you share
#Handle gmpy2 objects elif is_gmpy2_type(type(x)): return self.canonical_coercion(py_scalar_to_element(x), y) elif is_gmpy2_type(type(y)): return self.canonical_coercion(x, py_scalar_to_element(y))
with the associated code that deals with numpy types?
comment:9 Changed 5 years ago by
Commit:  48daf5857197f66b880a563ce27c34cf21959c87 → 54c04ae7c0bf7d48ca89c1e21c4184133e4c6a9c 

Branch pushed to git repo; I updated commit sha1. New commits:
54c04ae  Minor improvements

comment:11 Changed 5 years ago by
Description:  modified (diff) 

comment:12 Changed 5 years ago by
Description:  modified (diff) 

comment:13 Changed 5 years ago by
Description:  modified (diff) 

comment:14 Changed 5 years ago by
Description:  modified (diff) 

comment:15 Changed 5 years ago by
Description:  modified (diff) 

comment:16 Changed 5 years ago by
Description:  modified (diff) 

comment:17 Changed 5 years ago by
With PR #137 from gmpy2
 you can get rid of the function
get_gmpy2_path()
and most of the declarations insage/libs/gmpy2.pxd
.
 you can add tests for conversions using the methods
__mpz__
and__mpq__
sage: import gmpy2 sage: gmpy2.mpz(23)
andsage: import gmpy2 sage: gmpy2.mpq(12/14)
comment:18 followup: 19 Changed 5 years ago by
Is the coercion really needed? I mean, we cannot add coercion in Sage for every single external package. Is it really important that we can add a Sage polynomial to a gmpy2 integer for example?
comment:19 followup: 20 Changed 5 years ago by
Replying to jdemeyer:
Is the coercion really needed? I mean, we cannot add coercion in Sage for every single external package. Is it really important that we can add a Sage polynomial to a gmpy2 integer for example?
Not sure. Why do we provide coercion for numpy
? It is convenient to have Integer('2') + gmpy2.mpz('3')
working.
comment:20 Changed 5 years ago by
Replying to vdelecroix:
It is convenient to have
Integer('2') + gmpy2.mpz('3')
working.
Even if you agree with this, one needs to wonder whether the result should be a Sage integer or a gmpy2 integer (that's why I mentioned polynomials in 18)
comment:21 Changed 5 years ago by
Description:  modified (diff) 

The coercion aspect has been moved into another ticket #23052.
comment:22 Changed 5 years ago by
Commit:  54c04ae7c0bf7d48ca89c1e21c4184133e4c6a9c → 87578d9b02c6f2b88b0cd55d1d27a574316439e7 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
87578d9  Add path of gmpy2 in include_dirs

comment:23 Changed 5 years ago by
Didn't we patch Cython in #22728 precisely to avoid needed to mess with include paths?
comment:24 Changed 5 years ago by
Commit:  87578d9b02c6f2b88b0cd55d1d27a574316439e7 → ea8aeabc90ff7479e805ca5aa6657bd5c7cf2597 

comment:25 Changed 5 years ago by
Commit:  ea8aeabc90ff7479e805ca5aa6657bd5c7cf2597 → c28a1eee830fa3937e58f3657814f7c0aaa187dc 

Branch pushed to git repo; I updated commit sha1. New commits:
c28a1ee  remove OptionalExtension sage.libs.gmpy2

comment:26 Changed 5 years ago by
Component:  packages: optional → packages: standard 

comment:27 Changed 5 years ago by
 Is there a need of
get_gmpy2_path
?  I think we should avoid the import
gmpy2
completely and thatis_gmpy2_type
can be much faster. I made issue #139  Also test
sage: Integer(mpz(3)) sage: Rational(mpq(2, 3)) sage: Rational(mpz(5))
comment:28 Changed 5 years ago by
Commit:  c28a1eee830fa3937e58f3657814f7c0aaa187dc → 7edcfe46cb8d4abdb922f98d39604475a1350c78 

comment:29 Changed 5 years ago by
Description:  modified (diff) 

comment:30 Changed 5 years ago by
Description:  modified (diff) 

comment:31 Changed 5 years ago by
Commit:  7edcfe46cb8d4abdb922f98d39604475a1350c78 → 6bf31bab5e23764516042ad45047ef633886e01d 

Branch pushed to git repo; I updated commit sha1. New commits:
6bf31ba  Use of MPZ_Check and MPQ_Check functions from gmpy2 CAPI

comment:32 Changed 5 years ago by
In case a mpz
is used to construct a Rational
I would modify
self.__set_value(integer.Integer(x), base)
to
mpq_set_z(self.value, MPZ(<MPZ_Object *> x))
comment:33 followup: 34 Changed 5 years ago by
It may be faster but you lose a standard processing for all integer types. For example if
elif isinstance(x, integer.Integer): set_from_Integer(self, x)
evolve in some way you must copy the modification for the MPZ_Object case.
comment:34 followup: 35 Changed 5 years ago by
Replying to vklein:
It may be faster but you lose a standard processing for all integer types. For example if
elif isinstance(x, integer.Integer): set_from_Integer(self, x)evolve in some way you must copy the modification for the MPZ_Object case.
It is not only faster, it is much easier to read.
The concerned code is the following
elif MPQ_Check(<PyObject *> x): mpq_set(self.value, MPQ(<MPQ_Object *> x)) elif MPZ_Check(<PyObject *> x): self.__set_value(integer.Integer(x), base)
I don't see the logic in converting x
to a Sage Integer and then calling __set_value
instead of using the straightforward mpq_set_z(...)
.
BTW, I don't understand your comment. What should be an evolution of set_from_Integer
? An integer is a rational with denominator 1. This will not change in any near future.
comment:35 Changed 5 years ago by
Replying to vdelecroix:
... It is not only faster, it is much easier to read.
Not sure if there is a clear difference in readability between the two options, by the way there are many recursive call in this function.
Another reason for the present choice was to be consistent with numpy.integer processing. When modifying an existing code i tend to stick to the preceding coding style, (And having a standard processing for all int type is an understandable design choice). On the other hand in numpy.integer case doing a recursive call seems to be most concise way to do it, wich as you demonstrated is not the case for gmpy2 mpz.
That being said i got your point and prefer your solution.
comment:36 Changed 5 years ago by
Commit:  6bf31bab5e23764516042ad45047ef633886e01d → 53b8d500d1cfa1d837cb02f3779be15f106e4cb2 

Branch pushed to git repo; I updated commit sha1. New commits:
53b8d50  Change way of creating a Rational from a mpz

comment:37 Changed 5 years ago by
Commit:  53b8d500d1cfa1d837cb02f3779be15f106e4cb2 → 072a15c0456d864dfacc21c068f7cc6b98432a9c 

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:
8bbf2d3  update checksum, snapshot version update in setup.py

08c7a72  Add path of gmpy2 in include_dirs

359a0ff  update checksum and package version

0d4fd2e  Integer and rational constructors support gmpy2 params

7c66a7c  remove OptionalExtension sage.libs.gmpy2

d143dde  remove get_gmpy2_path()

2c56a3f  Add few doctests

e5264fd  Use of MPZ_Check and MPQ_Check functions from gmpy2 CAPI

504ab68  Change way of creating a Rational from a mpz

072a15c  add __mpz__() to Rational

comment:40 Changed 5 years ago by
Commit:  072a15c0456d864dfacc21c068f7cc6b98432a9c → 1e404b0b9ef9e2fc2cb6ed4c9b9ec0f44512465c 

Branch pushed to git repo; I updated commit sha1. New commits:
1e404b0  trac 29928 Change way of creating a Rational from a mpz

comment:41 Changed 5 years ago by
Description:  modified (diff) 

Summary:  Conversion between gmpy2 and sage integers → Conversion between gmpy2 and sage objects 
comment:42 Changed 5 years ago by
Commit:  1e404b0b9ef9e2fc2cb6ed4c9b9ec0f44512465c → 09288c73a47d1ce44b5f9e27aab620b75b71c977 

Branch pushed to git repo; I updated commit sha1. New commits:
09288c7  trac 22928 update gmpy2 snapshot version

comment:43 Changed 5 years ago by
Component:  packages: standard → interfaces 

comment:44 Changed 5 years ago by
Dependencies:  #22927 → #22927, #23309 

comment:45 Changed 5 years ago by
Commit:  09288c73a47d1ce44b5f9e27aab620b75b71c977 → 3e4c6f6267c3e405d6cd4612b616c7d7d14ff63d 

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:
3e5ded6  Add few doctests

758d7fd  Use of MPZ_Check and MPQ_Check functions from gmpy2 CAPI

d96f7bb  Change way of creating a Rational from a mpz

0b8c45e  add __mpz__() to Rational

8f90a09  trac 29928 Change way of creating a Rational from a mpz

6263747  trac 22928 update gmpy2 snapshot version

5b8b77b  trac 22928 RealNumber and RealDoubleElement and gmpy2

6e54851  trac 22928 add __mpfr__ to RealNumber class

7b75526  trac 22928 merge trac 22927

3e4c6f6  trac 22928 add __mpfr__ to RealDoubleElement class

comment:46 Changed 5 years ago by
Commit:  3e4c6f6267c3e405d6cd4612b616c7d7d14ff63d → 043250a6637cb989e53d0852d841367423dc3436 

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:
601935b  Add few doctests

6868a33  Use of MPZ_Check and MPQ_Check functions from gmpy2 CAPI

c91ada4  Change way of creating a Rational from a mpz

2d68635  add __mpz__() to Rational

48d4ce2  trac 29928 Change way of creating a Rational from a mpz

dffb83e  trac 22928 update gmpy2 snapshot version

2fa80f5  trac 22928 RealNumber and RealDoubleElement and gmpy2

32b4b86  trac 22928 add __mpfr__ to RealNumber class

2553307  trac 22928 merge trac 22927

043250a  trac 22928 add __mpfr__ to RealDoubleElement class

comment:48 Changed 5 years ago by
With RealNumber
you should check if precision is kept. Like with this number
sage: R = RealField(256) sage: R.pi() 3.141592653589793238462643383279502884197169399375105820974944592307816406286
Is there a way to check the precision of a real number with gmpy2?
comment:49 Changed 5 years ago by
Yes you can do it with the mpfr's precision attribute.
sage: from gmpy2 import mpfr sage: R = RealField(256) sage: r = mpfr(R.pi()) sage: r.precision 256
I will add this as a doctest
comment:50 Changed 5 years ago by
Commit:  043250a6637cb989e53d0852d841367423dc3436 → 3af335aa159ce276538f9d8347baa3a45d1af2bb 

Branch pushed to git repo; I updated commit sha1. New commits:
3af335a  trac 22928 add test for mpfr

comment:51 Changed 5 years ago by
Commit:  3af335aa159ce276538f9d8347baa3a45d1af2bb → f5a37994b02d98e73cfad3322348b1aaa57c40de 

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:
f821d9e  Use of MPZ_Check and MPQ_Check functions from gmpy2 CAPI

24988ee  Change way of creating a Rational from a mpz

f0e2114  add __mpz__() to Rational

2299a79  trac 29928 Change way of creating a Rational from a mpz

d38e2dc  trac 22928 update gmpy2 snapshot version

2183a91  trac 22928 RealNumber and RealDoubleElement and gmpy2

c607d49  trac 22928 add __mpfr__ to RealNumber class

e3c99f1  trac 22928 merge trac 22927

9b5df1b  trac 22928 add __mpfr__ to RealDoubleElement class

f5a3799  trac 22928 add test for mpfr

comment:52 Changed 5 years ago by
Replying to vklein:
All these features need
gmpy2
being a standard package.
I don't think that this is completely true. I understand why you think so, but it shouldn't be too hard to implement this as an optional interface.
comment:53 Changed 5 years ago by
At the begining i ve tried to make it optional. I don't rememeber exactly why i have come to the conclusion that it can't be optional. I will take a second look at it.
comment:54 Changed 5 years ago by
Commit:  f5a37994b02d98e73cfad3322348b1aaa57c40de → 99b77348d7189300b0fa1e1d04cdb9cbcb4b0765 

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:
ba3bc11  Use of MPZ_Check and MPQ_Check functions from gmpy2 CAPI

6a7f661  Change way of creating a Rational from a mpz

e98845c  add __mpz__() to Rational

6161e79  trac 29928 Change way of creating a Rational from a mpz

aac3bf4  trac 22928 update gmpy2 snapshot version

4a9e3b9  trac 22928 RealNumber and RealDoubleElement and gmpy2

4c25530  trac 22928 add __mpfr__ to RealNumber class

1129eca  trac 22928 merge trac 22927

0ddcc85  trac 22928 add __mpfr__ to RealDoubleElement class

99b7734  trac 22928 add test for mpfr

comment:56 Changed 5 years ago by
There is some very ugly Cython code in this branch. I guess this is mostly caused by upstream's ignorance on how to do Cython declarations correctly. I'll try to fix this.
comment:58 Changed 5 years ago by
Is there any reason why we need this many commits? Can I just squash all that to 1 commit?
comment:59 Changed 5 years ago by
Just historical reason. The gmpy2's cython interface has evolved since this version. You can look if it covers what seems relevant to you PR#155.
The plan of my last push was just to rebase this ticket before integrating the future stable version of gmpy2
comment:60 Changed 5 years ago by
Branch:  u/vklein/conversion_between_gmpy2_and_sage_integers → u/jdemeyer/conversion_between_gmpy2_and_sage_integers 

comment:61 Changed 5 years ago by
Commit:  99b77348d7189300b0fa1e1d04cdb9cbcb4b0765 → c08a7102060473a105ff4106ed75fc615c79c7f4 

I made an upstream pull request https://github.com/aleaxit/gmpy/pull/169 to clean up some Cython declarations.
Then, I cleaned up the branch on this ticket using that PR and I made this branch independent of the gmpy2 upgrade for easier development.
New commits:
60f61ff  Conversion between gmpy2 and Sage objects

c08a710  Cleaner gmpy2 interface

comment:62 Changed 5 years ago by
Thanks ! I will update the tarball of #22927, so people can test this ticket without doing their own (as current one doesn't contain the new cython interface).
comment:63 Changed 5 years ago by
Dependencies:  #22927, #23309 → #22927 

Now working on making gmpy2 optional...
comment:64 Changed 5 years ago by
Dependencies:  #22927 → #22927, #24215 

comment:65 Changed 5 years ago by
Commit:  c08a7102060473a105ff4106ed75fc615c79c7f4 → 480a79d37226e927b25508f706b1b67717e2e2ee 

comment:67 Changed 5 years ago by
Branch:  u/jdemeyer/conversion_between_gmpy2_and_sage_integers → u/vklein/conversion_between_gmpy2_and_sage_integers 

comment:68 Changed 5 years ago by
Commit:  480a79d37226e927b25508f706b1b67717e2e2ee → 0ecbffda255da8d6ab042eab194cd295b72e5d13 

New commits:
d6072ca  use a snapshot version of gmpy2

ce7a082  update checksum, snapshot version update in setup.py

2eca651  New snapshot version, update checksum and packageversion.txt

bd257c1  Update checksum and snapshot version

32cc1ab  trac 22927 update gmpy2 snapshot version

aab4169  trac 22927 update gmpy2 package version

0ba3ffb  trac 22927: update package version to snapshot14.11.17

3bf081f  Merge branch 'u/vklein/add_gmpy2_as_an_optional_package' of git://trac.sagemath.org/sage into t/22928/conversion_between_gmpy2_and_sage_integers

0ecbffd  trac 22928 : add __mpc__ function to Sage complex numbers

comment:69 Changed 5 years ago by
Milestone:  sage8.0 → sage8.2 

The documentation
Convert Sage ``ComplexNumber`` to gmpy2 ``Complex``.
refers to a type Complex
that does not exist in gmpy2 and ComplexNumber
should be one of ComplexDoubleElement
, MPComplexNumber
or ComplexNumber
which are three distinct classes.
comment:70 Changed 5 years ago by
Commit:  0ecbffda255da8d6ab042eab194cd295b72e5d13 → 6d674fc2956a9674ccb7349b47c316193788ee84 

Branch pushed to git repo; I updated commit sha1. New commits:
6d674fc  trac 22928 : fix __mpc__ documentation

comment:71 Changed 5 years ago by
Commit:  6d674fc2956a9674ccb7349b47c316193788ee84 → e406a9e61f43e832baecf6d2aa300f01362ca8d7 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
e000563  Trac #22927 gmpy2 update package version to 2.1.0a1

82d9462  Add HAVE_GMPY2 compiletime constant

2d01807  Conversion between gmpy2 and Sage objects

b9c7c43  Cleaner and optional gmpy2 interface

476ddc2  trac 22928 : add __mpc__ function to Sage complex numbers

e406a9e  trac 22928 : fix __mpc__ documentation

comment:73 Changed 5 years ago by
Status:  new → needs_review 

comment:74 Changed 5 years ago by
Description:  modified (diff) 

comment:75 Changed 5 years ago by
Status:  needs_review → needs_work 

This branch will not work. The commit in #24215 is 7f06e71
but you rebased it here. So there will be a conflict.
comment:76 Changed 5 years ago by
Commit:  e406a9e61f43e832baecf6d2aa300f01362ca8d7 → ddd29a839bd97eabf21deef796472384441e9755 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
34ef6dc  Add HAVE_GMPY2 compiletime constant

158d984  Force absolute import in have_module()

7f06e71  Fix documentation

9dceca0  Merge branch 'u/jdemeyer/ticket/24215' of git://trac.sagemath.org/sage into 22928

8bed52c  Conversion between gmpy2 and Sage objects

4f79ed7  Cleaner and optional gmpy2 interface

8938549  trac 22928 : add __mpc__ function to Sage complex numbers

ddd29a8  trac 22928 : fix __mpc__ documentation

comment:78 followup: 79 Changed 5 years ago by
Status:  needs_review → needs_work 

sage: import gmpy2 sage: gmpy2.mpz(1/2) > SEGFAULT <
comment:79 Changed 5 years ago by
Status:  needs_work → needs_review 

Replying to vdelecroix:
sage: import gmpy2 sage: gmpy2.mpz(1/2) > SEGFAULT <
That is really a bug in gmpy2
, not in Sage.
comment:81 Changed 5 years ago by
I might review this, but only after the dependencies have been merged in a beta.
comment:82 Changed 5 years ago by
Branch:  u/vklein/conversion_between_gmpy2_and_sage_integers → u/jdemeyer/conversion_between_gmpy2_and_sage_integers 

comment:83 Changed 5 years ago by
Commit:  ddd29a839bd97eabf21deef796472384441e9755 → 9528b59e0936668ff3c05ec9ced10c322c71e8ba 

Dependencies:  #22927, #24215 
comment:85 Changed 5 years ago by
Dependencies:  → #24417 

comment:87 Changed 5 years ago by
Branch:  u/jdemeyer/conversion_between_gmpy2_and_sage_integers → u/vklein/conversion_between_gmpy2_and_sage_integers 

comment:88 Changed 5 years ago by
Commit:  9528b59e0936668ff3c05ec9ced10c322c71e8ba → 01978e3951497753e85d0c9a2180a54030cb4732 

Status:  needs_work → needs_review 
comment:89 Changed 5 years ago by
Status:  needs_review → needs_work 

Some trivial merge/rebase to be done on 8.2.beta2
comment:90 Changed 5 years ago by
Commit:  01978e3951497753e85d0c9a2180a54030cb4732 → b1481a2aed0ca67bf974ef85a772a97d587a7a2d 

comment:92 followups: 96 99 Changed 5 years ago by
Status:  needs_review → needs_work 

Sage is crashing
ImportError: /opt/sage/local/lib/python2.7/sitepackages/sage/rings/complex_number.so: undefined symbol: mpc_set_fr_fr
The reason is that you are importing from sage.libs.mpc cimport *
in complex_number.pyx
and that this header file does not have the proper distutils directive. Just apply the patch

src/sage/libs/mpc.pxd
diff git a/src/sage/libs/mpc.pxd b/src/sage/libs/mpc.pxd index 0e12d31468..0067b8b126 100644
a b 1 # distutils: libraries = gmp mpfr mpc 2 1 3 from sage.libs.gmp.types cimport * 2 4 from sage.libs.mpfr.types cimport *
comment:93 Changed 5 years ago by
You should test different precisions for each possible type (that is RealField(prec)
, ComplexField(prec)
and MPComplexField(prec)
).
comment:94 followups: 100 102 Changed 5 years ago by
The following is not nice
sage: gmpy2.mpfr(gmpy2.mpq('1/3'), precision=123).precision 123 sage: RealNumber(gmpy2.mpfr(gmpy2.mpq('1/3'), precision=123)) 0.33333333333333333333333333333333333332 sage: _.prec() # why not 123? 130
And this is even worse
sage: ComplexNumber(gmpy2.mpc(gmpy2.mpq('1/3'), precision=123r)) Traceback (most recent call last): ... TypeError: unable to coerce to a ComplexNumber: <type 'str'>
comment:95 followup: 98 Changed 5 years ago by
Funny behavior of gmpy2
precision with Sage integers
sage: gmpy2.mpfr('1.0', precision=40) mpfr('1.0',40) sage: gmpy2.mpc('1.0', precision=40) Traceback (most recent call last): ... TypeError: precision for mpc() must be integer or tuple
Is that worth an issue?
comment:96 Changed 5 years ago by
src/sage/libs/mpc.pxd
diff git a/src/sage/libs/mpc.pxd b/src/sage/libs/mpc.pxd index 0e12d31468..0067b8b126 100644
a b 1 # distutils: libraries = gmp mpfr mpc 2 1 3 from sage.libs.gmp.types cimport * 2 4 from sage.libs.mpfr.types cimport *
I have miss this one but encoutered it on 23024. It has been solved by updating module_list.py by adding mpc as a lib to sage.rings.complex_number Extension. What is the best way to fix it between these two ?
comment:97 Changed 5 years ago by
Modifying module_list.py
is not a good idea. If I write my own Cython file with
from sage.rings.complex_number cimport whatever
Then my program will not compile.
addendum: will not compile if I do not explicitely state that my program needs to be linked against mpc... that would better to be automatic (especially in interactive use of Cython inside Sage)
comment:98 Changed 5 years ago by
Ok i see.
Replying to vdelecroix:
Funny behavior of
gmpy2
precision with Sage integerssage: gmpy2.mpfr('1.0', precision=40) mpfr('1.0',40) sage: gmpy2.mpc('1.0', precision=40) Traceback (most recent call last): ... TypeError: precision for mpc() must be integer or tupleIs that worth an issue?
I think it is. I will look at it and open an issue if needed.
comment:99 Changed 5 years ago by
Dependencies:  #24417 → #24549 

Replying to vdelecroix:
src/sage/libs/mpc.pxd
diff git a/src/sage/libs/mpc.pxd b/src/sage/libs/mpc.pxd index 0e12d31468..0067b8b126 100644
a b 1 # distutils: libraries = gmp mpfr mpc 2 1 3 from sage.libs.gmp.types cimport * 2 4 from sage.libs.mpfr.types cimport *
See #24549 for that.
comment:100 Changed 5 years ago by
Replying to vdelecroix:
And this is even worse
sage: ComplexNumber(gmpy2.mpc(gmpy2.mpq('1/3'), precision=123r)) Traceback (most recent call last): ... TypeError: unable to coerce to a ComplexNumber: <type 'str'>
But should we really use ComplexNumber this way ?
This call the function create_ComplexNumber and his first parameter is supposed to be the real part only.
In your call the first parameter is the whole complex number.
Secondly create_ComplexNumber always try to convert the parameters (real and imag) into string before calling the ComplexNumber constructor.
Given that if you try to call ComplexNumber() with another complex type as parameter you will get the same result :
sage: cn = ComplexNumber(5,4) sage: ComplexNumber(cn)  TypeError Traceback (most recent call last) ... TypeError: unable to coerce to a ComplexNumber: <type 'str'>
comment:101 Changed 5 years ago by
The ComplexNumber
function is wrong in several ways (see #13110). Just ignore it.
comment:102 followup: 106 Changed 5 years ago by
Replying to vdelecroix:
The following is not nice
sage: gmpy2.mpfr(gmpy2.mpq('1/3'), precision=123).precision 123 sage: RealNumber(gmpy2.mpfr(gmpy2.mpq('1/3'), precision=123)) 0.33333333333333333333333333333333333332 sage: _.prec() # why not 123? 130
Like for ComplexNumber
function it seems that RealNumber
function it's not designed to be used this way. It's supposed to take a string as first parameter.
That being said the gmpy2's precision is not currently transmitted with RR
function.
sage: RR(gmpy2.mpfr(gmpy2.mpq('1/3'), precision=123)) 0.333333333333333 sage: _.prec() 53
I am on it.
comment:103 Changed 5 years ago by
The only fix i see so far would be do a new mpfr_init2
during _set
call. It's a little ugly :
#real_mpfr.pyx line 1467 elif HAVE_GMPY2 and type(x) is gmpy2.mpfr: self._parent = self._parent.to_prec(x.precision) mpfr_init2(self.value, x.precision) mpfr_set(self.value, (<gmpy2.mpfr>x).f, parent.rnd)
Or we can assume the user shoud do that :
sage: R = RealField(128) sage: R(gmpy2.mpfr(gmpy2.mpq('1/3'), precision=128)) 0.33333333333333333333333333333333333333 sage: _.prec() 128
comment:104 Changed 5 years ago by
RealNumber
should be ignored for the same reason (it is aimed to be used by the preparser). You can read #13110 that I already mentioned in comment:101.
comment:105 Changed 5 years ago by
Sure ! My current question was more about what should we do for the RR
function. Should we transmit the precision (and replacing the parent during __c_init__
or during _set
) or rely on a consistent usage of RealField
?
comment:106 Changed 5 years ago by
Replying to vklein:
Replying to vdelecroix:
sage: RR(gmpy2.mpfr(gmpy2.mpq('1/3'), precision=123)) 0.333333333333333 sage: _.prec() 53
That's fine. RR
is the field of floatingpoint numbers with 53 bits of precision (stupidly named).
comment:107 Changed 5 years ago by
That is to say: RR
is not a function. It is the object RealField(53)
.
sage: RR is RealField(53) True
comment:108 Changed 5 years ago by
Commit:  b1481a2aed0ca67bf974ef85a772a97d587a7a2d → e9188bae6e2e5407101d1ef46f3343b7250ae375 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
b1047cc  Clean up MPC declarations

d9308b2  Conversion between gmpy2 and Sage objects

074acd6  Cleaner and optional gmpy2 interface

2604a37  trac 22928 : add __mpc__ function to Sage complex numbers

e9188ba  Trac #22928: Test different precisions for each possible type

comment:109 Changed 5 years ago by
Status:  needs_work → needs_review 

Add some test with different precision comment:93.
Rebase on #24549.
comment:110 followup: 111 Changed 5 years ago by
You do not need to put # optional  gmpy2
everywhere. Only where you explicitely need gmpy2
.
Could you also doctest the fact that the composition of conversions Sage number > gmpy2 number > Sage number
is the identity when precisions are set appropriately (ie we get back an equal element)? For example
sage: MPCF = MPComplexField(63) sage: x = MPCF('15.64E+128', '15.64E+128') sage: y = mpc(x) # optional  gmpy2 sage: y.precision # optional  gmpy2 (63, 63) sage: MPCF(y) == x # optional  gmpy2 True
And also the other way around gmpy2 number > Sage number > gmpy2 number
sage: x = gmpy2.mpc('1.324+4e50j', precision=(70,70)) # optional  gmpy2 sage: y = ComplexField(70)(x) # optional  gmpy2 sage: gmpy2.mpc(y) == x # optional  gmpy2 True
comment:111 Changed 5 years ago by
Status:  needs_review → needs_work 

Ok i will add this type of doctest.
comment:112 followup: 113 Changed 5 years ago by
These tests works with MPComplexField
and RealField
but they didn't work with ComplexField
in gmpy2 number > Sage number > gmpy2 number
way :
sage: x = gmpy2.mpc('1.324+4e50j', precision=(70,70)) sage: y = ComplexField(70)(x) sage: gmpy2.mpc(y) == x False sage: x mpc('1.3240000000000000000005+3.9999999999999999999976e+50j',(70,70)) sage: gmpy2.mpc(y) mpc('1.3240000000000000657252+4.0000000000000003051908e+50j',(70,70))
Is this an issue or a normal rounding behaviour ?
comment:113 Changed 5 years ago by
Replying to vklein:
These tests works with
MPComplexField
andRealField
but they didn't work withComplexField
ingmpy2 number > Sage number > gmpy2 number
way :sage: x = gmpy2.mpc('1.324+4e50j', precision=(70,70)) sage: y = ComplexField(70)(x) sage: gmpy2.mpc(y) == x FalseIs this an issue or a normal rounding behaviour ?
To me, this is an issue. The code in your example should just be copying mpfr_t
values around.
comment:115 Changed 5 years ago by
I think that the problem is that we just copy the values of real and imag part and both of them are gmpy2.mpfr
with their own precision.
sage: x = gmpy2.mpc('1.324+4e50j', precision=(70,70)) sage: x.precision (70, 70) sage: x.real.precision 53 sage: x.imag.precision 53 sage: x mpc('1.3240000000000000000005+3.9999999999999999999976e+50j',(70,70)) sage: x.real, x.imag (mpfr('1.3240000000000001'), mpfr('4.0000000000000003e+50'))
MPComplexField
works fine because it use mpc_set and not two mpfr_set.
comment:116 Changed 5 years ago by
I don't know if it is a bug from gmpy2
but having x.real
to return a double precision and not the corresponding gmpy2.mpfr
with the same precision as the real part is weird. Would be nice to see whether it is intended in gmpy2
.
For this ticket you could just avoid using .real
and .imag
in the ComplexNumber
code.
comment:117 Changed 5 years ago by
Specification are really cool!
imag Returns the imaginary component. real Returns the real component.
(see https://github.com/aleaxit/gmpy/blob/master/docs/mpc.rst)
comment:118 followup: 125 Changed 5 years ago by
Yes very accurate !
It works fine without .real
and .imag
. Tests are running now.
comment:119 Changed 5 years ago by
Commit:  e9188bae6e2e5407101d1ef46f3343b7250ae375 → 5d044bfb4c8ada79db6e8b0147a1f1120cf49a9d 

Branch pushed to git repo; I updated commit sha1. New commits:
5d044bf  Trac #22928: Add tests and fix a bug with precision

comment:120 Changed 5 years ago by
Status:  needs_work → needs_review 

comment:121 Changed 5 years ago by
Do you really need the cast <mpc_t>
? This is already explicit in the gmpy2
Cython header.
comment:122 followup: 123 Changed 5 years ago by
I think i do at least
mpfr_set(self.__im, (<gmpy2.mpc>real).c.im, rnd) ^  sage/rings/complex_number.pyx:197:55: Cannot convert Python object to '__mpfr_struct *'
What syntax would you suggest ?
comment:123 Changed 5 years ago by
Replying to vklein:
I think i do at least
mpfr_set(self.__im, (<gmpy2.mpc>real).c.im, rnd) ^  sage/rings/complex_number.pyx:197:55: Cannot convert Python object to '__mpfr_struct *'What syntax would you suggest ?
The one in your comment. There might be something wrong with the Cython headers. Maybe the following change in gmpy2.pxd
might be enough

src/gmpy2.pxd
diff git a/src/gmpy2.pxd b/src/gmpy2.pxd index 7cdcc72..d6cac1b 100644
a b cdef extern from "mpfr.h": 45 45 cdef extern from "mpc.h": 46 46 # mpc complexes 47 47 ctypedef struct __mpc_struct: 48 pass 48 mpfr_t re 49 mpfr_t im 49 50 ctypedef __mpc_struct mpc_t[1]; 50 51 ctypedef __mpc_struct *mpc_ptr; 51 52 ctypedef const __mpc_struct *mpc_srcptr;
comment:124 Changed 5 years ago by
It works with your solution. That being said i am not sure if avoiding a cast is worth patching gmpy2
.
I would rather keep the current syntax and do a proper pull request for gmpy2
.
comment:125 Changed 5 years ago by
Replying to vklein:
Yes very accurate !
It works fine without
.real
and.imag
. Tests are running now.
This is also obviously an upstream bug.
comment:126 followup: 132 Changed 5 years ago by
In complex_number.pyx
you are only using the mpc_t
type, nothing else from MPC. So just use from sage.libs.mpc.types cimport mpc_t
.
comment:127 followup: 130 Changed 5 years ago by
I think it would be good to at least report the bugs upstream to gmpy2.
comment:128 Changed 5 years ago by
Status:  needs_review → needs_work 

comment:129 Changed 5 years ago by
Reviewers:  → Jeroen Demeyer 

comment:130 Changed 5 years ago by
Replying to jdemeyer:
I think it would be good to at least report the bugs upstream to gmpy2.
Sure you're right. https://github.com/aleaxit/gmpy/issues/179
comment:131 Changed 5 years ago by
Commit:  5d044bfb4c8ada79db6e8b0147a1f1120cf49a9d → b6e2c05634879e77912848a89a45a145c1df526f 

Branch pushed to git repo; I updated commit sha1. New commits:
b6e2c05  Trac #22928: replace cimport * by cimport mpc_t in

comment:132 followup: 133 Changed 5 years ago by
Status:  needs_work → needs_review 

Replying to jdemeyer:
In
complex_number.pyx
you are only using thempc_t
type, nothing else from MPC. So just usefrom sage.libs.mpc.types cimport mpc_t
.
Done
comment:133 Changed 5 years ago by
Status:  needs_review → needs_work 

comment:134 Changed 5 years ago by
Doctests crash at runtime with the later syntax.
sage t long src/sage/rings/complex_number.pyx Traceback (most recent call last): File "/home/vklein/odk/sage/src/bin/sageruntests", line 123, in <module> from sage.doctest.control import DocTestController File "/home/vklein/odk/sage/local/lib/python2.7/sitepackages/sage/doctest/control.py", line 33, in <module> from .sources import FileDocTestSource, DictAsObject File "/home/vklein/odk/sage/local/lib/python2.7/sitepackages/sage/doctest/sources.py", line 32, in <module> from .parsing import SageDocTestParser File "/home/vklein/odk/sage/local/lib/python2.7/sitepackages/sage/doctest/parsing.py", line 57, in <module> from sage.all import RealIntervalField File "/home/vklein/odk/sage/local/lib/python2.7/sitepackages/sage/all.py", line 87, in <module> from sage.misc.all import * # takes a while File "/home/vklein/odk/sage/local/lib/python2.7/sitepackages/sage/misc/all.py", line 84, in <module> from .functional import (additive_order, File "/home/vklein/odk/sage/local/lib/python2.7/sitepackages/sage/misc/functional.py", line 27, in <module> from sage.rings.complex_double import CDF File "sage/rings/integer.pxd", line 7, in init sage.rings.complex_double File "sage/rings/rational.pxd", line 8, in init sage.rings.integer File "sage/rings/rational.pyx", line 94, in init sage.rings.rational File "sage/rings/real_mpfr.pyx", line 1, in init sage.rings.real_mpfr File "sage/rings/complex_number.pxd", line 6, in init sage.libs.mpmath.utils ImportError: /home/vklein/odk/sage/local/lib/python2.7/sitepackages/sage/rings/complex_number.so: undefined symbol: mpc_set_fr_fr
comment:135 Changed 5 years ago by
Indeed, mpc/types.pxd
does not contain the distutils directive for libraries. Since types.pxd
is included in all other files in mpc/
it makes more sense to have the directive in this file rather than in __init__.pxd
.
comment:138 followup: 139 Changed 5 years ago by
Should we add a distutils directive to the gmpy2.pxd
header?
comment:139 followup: 140 Changed 5 years ago by
Replying to vdelecroix:
Should we add a distutils directive to the
gmpy2.pxd
header?
No because not everybody using that header will need every library. It seems overkill to require gmp and mpfr and mpc if you only ever use the mpz_t
type.
I was thinking about something else: it is trivial to replace mpc_set_fr_fr
by two mpfr_set
calls. So maybe we could just do that. I'll try to create a PR for gmpy2.
comment:140 followup: 142 Changed 5 years ago by
Replying to jdemeyer:
Replying to vdelecroix:
Should we add a distutils directive to the
gmpy2.pxd
header?No because not everybody using that header will need every library. It seems overkill to require gmp and mpfr and mpc if you only ever use the
mpz_t
type.I was thinking about something else: it is trivial to replace
mpc_set_fr_fr
by twompfr_set
calls. So maybe we could just do that. I'll try to create a PR for gmpy2.
Using mpc_set_fr_fr
would impose to link against mpfr. A cleaner possibility is to split the gmpy2
headers into 3 headers gmpy2.gmp
, gmpy2.mpfr
and gmpy2.mpc
. Each of them with a proper distutils directive.
comment:141 Changed 5 years ago by
Description:  modified (diff) 

comment:142 followup: 143 Changed 5 years ago by
Replying to vdelecroix:
Using
mpc_set_fr_fr
would impose to link against mpfr.
I guess you mean "using mpfr_set
would impose to link against mpfr".
But that is in fact a very logical requirement, since the input type of GMPy_MPC_From_mpfr
is mpfr_t
. So you already need to have an mpfr_t
in order to use that function, which makes it likely that you are using MPFR already.
comment:143 followup: 145 Changed 5 years ago by
Replying to jdemeyer:
Replying to vdelecroix:
Using
mpc_set_fr_fr
would impose to link against mpfr.I guess you mean "using
mpfr_set
would impose to link against mpfr".But that is in fact a very logical requirement, since the input type of
GMPy_MPC_From_mpfr
ismpfr_t
. So you already need to have anmpfr_t
in order to use that function, which makes it likely that you are using MPFR already.
And using a mpc
from gmpy2
does not imply that you are using mpc
!?
comment:144 followup: 146 Changed 5 years ago by
Will it be so inconsistent to just stick with from sage.libs.mpc cimport *
for this ticket?
I don't see clearly how much impact it has on memory/performance.
comment:145 Changed 5 years ago by
Replying to vdelecroix:
And using a
mpc
fromgmpy2
does not imply that you are usingmpc
!?
Sure, but in an indirect way, through the gmpy2
package. My point is that you shouldn't require the MPC library in the Cython module which creates a gmpy2.mpc
object.
comment:146 Changed 5 years ago by
Replying to vklein:
Will it be so inconsistent to just stick with
from sage.libs.mpc cimport *
for this ticket?
I don't see clearly how much impact it has on memory/performance.
I agree... we can still fix this later. I'd like to wait a little bit for an answer from upstream though.
comment:147 Changed 5 years ago by
With my patch to gmpy2, the following works:

src/sage/rings/complex_number.pyx
diff git a/src/sage/rings/complex_number.pyx b/src/sage/rings/complex_number.pyx index bf0e4a3..76782c3 100644
a b from __future__ import absolute_import, print_function 31 31 import math 32 32 import operator 33 33 34 from sage.libs.mpc cimport mpc_t34 from sage.libs.mpc.types cimport mpc_t 35 35 from sage.libs.mpfr cimport * 36 36 from sage.structure.element cimport FieldElement, RingElement, Element, ModuleElement 37 37 from sage.categories.map cimport Map … … cdef class ComplexNumber(sage.structure.element.FieldElement): 193 193 elif isinstance(real, complex): 194 194 real, imag = real.real, real.imag 195 195 elif HAVE_GMPY2 and type(real) is gmpy2.mpc: 196 mpfr_set(self.__re, (< mpc_t>(<gmpy2.mpc>real).c).re, rnd)197 mpfr_set(self.__im, (< mpc_t>(<gmpy2.mpc>real).c).im, rnd)196 mpfr_set(self.__re, (<gmpy2.mpc>real).c.re, rnd) 197 mpfr_set(self.__im, (<gmpy2.mpc>real).c.im, rnd) 198 198 return 199 199 else: 200 200 imag = 0
comment:148 followup: 149 Changed 5 years ago by
And i have fixed issue gmpy2 issue #179 with this PR https://github.com/aleaxit/gmpy/pull/181.
What should we do next ? :
 Waiting for a gmpy2 release.
 Doing sage patchs for gmpy2 (maybe after
gmpy2
PR's are reviewed and merged).  Or switch this ticket in need_review as it is.
comment:149 Changed 5 years ago by
Replying to vklein:
And i have fixed issue gmpy2 issue #179 with this PR https://github.com/aleaxit/gmpy/pull/181.
What should we do next ? :
 Waiting for a gmpy2 release.
 Doing sage patchs for gmpy2 (maybe after
gmpy2
PR's are reviewed and merged). Or switch this ticket in need_review as it is.
I would say
 create a patch and add it to gmpy2
 move the ticket in needs review
 wait for the PR to be accepted before setting it to positive review
The rationale is that we are currently testing gmpy2 integration. At the time it stabilizes, we should ask for a new release and remove all of Sage patches.
comment:150 Changed 5 years ago by
Description:  modified (diff) 

comment:152 Changed 5 years ago by
Commit:  b6e2c05634879e77912848a89a45a145c1df526f → 1c5685f5f11c87b28ec3ad9e76d2e3ae54eef4df 

Branch pushed to git repo; I updated commit sha1. New commits:
1c5685f  Trac #22928: Patchs gmpy2 and update complex_number

comment:153 Changed 5 years ago by
Commit:  1c5685f5f11c87b28ec3ad9e76d2e3ae54eef4df → dcc7e9567a7df1f37aa795bb7f58ba184985745c 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
dcc7e95  Trac #22928: Patchs gmpy2 and update complex_number

comment:154 Changed 5 years ago by
Status:  needs_work → needs_review 

Patch are done. The corresponding PRs are merged.
comment:155 Changed 5 years ago by
Status:  needs_review → needs_work 

You need to bump the gmpy2
version number for the patches to apply.
comment:156 Changed 5 years ago by
Commit:  dcc7e9567a7df1f37aa795bb7f58ba184985745c → e3a3bf49b965b6f81814efe36d9b0bbe49a43156 

Branch pushed to git repo; I updated commit sha1. New commits:
e3a3bf4  Trac #22928 : update version patch p0 to p1

comment:158 Changed 5 years ago by
The following line misses a # optional  gmpy2
sage: x = mpfr(R.pi())
comment:159 Changed 5 years ago by
Yeah and two others after this one
sage: y = RealField(128)(x) sage: mpfr(y) == x
I will rerun tests without gmpy2 package installed
comment:160 Changed 5 years ago by
Status:  needs_review → needs_work 

comment:161 Changed 5 years ago by
Commit:  e3a3bf49b965b6f81814efe36d9b0bbe49a43156 → e9a6fdaf5e130e826929c4d9a6617a2128afe3d0 

Branch pushed to git repo; I updated commit sha1. New commits:
e9a6fda  Trac #22928 : add some missings # optional  gmpy2

comment:163 Changed 5 years ago by
Reviewers:  Jeroen Demeyer → Jeroen Demeyer, Vincent Delecroix 

needs review? The branch is fine for me in the current state.
comment:165 Changed 5 years ago by
In real_double.pyx
you call cimport gmpy2
but does not do gmpy2.import_gmpy2()
.
comment:166 followup: 167 Changed 5 years ago by
Yes but real_double.pyx
do cimport sage.ring.integer
and the later call gmpy2.import_gmpy2()
.
comment:167 Changed 5 years ago by
Replying to vklein:
Yes but
real_double.pyx
docimport sage.ring.integer
and the later callgmpy2.import_gmpy2()
.
This sentence makes me sad and worried at the same time. It shows that you absolutely do not understand what import_gmpy2()
does.
The real reason that you don't need import_gmpy2()
in that file is that you are simply not calling any gmpy2 functions in that Cython module.
comment:169 Changed 5 years ago by
Status:  needs_review → positive_review 

comment:170 Changed 5 years ago by
Branch:  u/vklein/conversion_between_gmpy2_and_sage_integers → e9a6fdaf5e130e826929c4d9a6617a2128afe3d0 

Resolution:  → fixed 
Status:  positive_review → closed 
comment:171 followup: 173 Changed 4 years ago by
Commit:  e9a6fdaf5e130e826929c4d9a6617a2128afe3d0 

A small remark to say that some warnings associated to gmpy2 appear at each run of sage b
(without gmpy2):
[sagelib8.2.rc3] sage/rings/complex_double.pyx: cannot find cimported module 'gmpy2' [sagelib8.2.rc3] sage/rings/complex_number.pyx: cannot find cimported module 'gmpy2' [sagelib8.2.rc3] sage/rings/integer.pyx: cannot find cimported module 'gmpy2' [sagelib8.2.rc3] sage/rings/complex_mpc.pyx: cannot find cimported module 'gmpy2' [sagelib8.2.rc3] sage/rings/rational.pyx: cannot find cimported module 'gmpy2' [sagelib8.2.rc3] sage/rings/real_double.pyx: cannot find cimported module 'gmpy2' [sagelib8.2.rc3] sage/rings/real_mpfr.pyx: cannot find cimported module 'gmpy2'
Is it possible to clean this?
comment:172 Changed 4 years ago by
According to Jeroen it's harmless and will require a cython patch to fix it.
comment:173 Changed 4 years ago by
Replying to slabbe:
Is it possible to clean this?
Unfortunately, it's not easy to properly fix. The basic problem is that the cimport
is parsed very very early in the Cython build system, at a point where macros like HAVE_GMPY2
are not considered yet.
Branch pushed to git repo; I updated commit sha1. New commits:
Add path of gmpy2 in include_dirs