Conversion between gmpy2 and sage objects

[vklein]

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 numbers
  • RealNumber (sage.rings.real_mpfr)
  • RealDoubleElement (sage.rings.real_double)
• __mpc__ on relevant Sage complex numbers
  • ComplexDoubleElement (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:

• ​https://github.com/aleaxit/gmpy/pull/180
• ​https://github.com/aleaxit/gmpy/pull/181

[git]

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

​48daf58

Add path of gmpy2 in include_dirs

[vdelecroix]

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?

[git]

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

​54c04ae

Minor improvements

[vklein]

Replying to vdelecroix: Done with commit ​54c04ae

[vdelecroix]

With ​PR #137 from gmpy2

• you can get rid of the function get_gmpy2_path() and most of the declarations in sage/libs/gmpy2.pxd.

• you can add tests for conversions using the methods __mpz__ and __mpq__

  sage: import gmpy2
  sage: gmpy2.mpz(23)
  

  and

  sage: import gmpy2
  sage: gmpy2.mpq(12/14)    
  

[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?

[vdelecroix]

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.

[jdemeyer]

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)

[vklein]

The coercion aspect has been moved into another ticket #23052.

[git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

​87578d9

Add path of gmpy2 in include_dirs

[jdemeyer]

Didn't we patch Cython in #22728 precisely to avoid needed to mess with include paths?

[git]

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

​8cbbe31	update checksum and package version
​ea8aeab	Integer and rational constructors support gmpy2 params

[git]

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

​c28a1ee

remove OptionalExtension sage.libs.gmpy2

[vdelecroix]

• Is there a need of get_gmpy2_path?
• I think we should avoid the import gmpy2 completely and that is_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))
  

[git]

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

​db38086	remove get_gmpy2_path()
​7edcfe4	Add few doctests

[git]

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

​6bf31ba

Use of MPZ_Check and MPQ_Check functions from gmpy2 C-API

[vdelecroix]

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))

[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.

[vdelecroix]

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.

[vklein]

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.

[git]

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

​53b8d50

Change way of creating a Rational from a mpz

[git]

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 C-API
​504ab68	Change way of creating a Rational from a mpz
​072a15c	add __mpz__() to Rational

[vdelecroix]

I thought that we agreed on comment:32...

[vklein]

A merge error

[git]

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

​1e404b0

trac 29928 Change way of creating a Rational from a mpz

[git]

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

​09288c7

trac 22928 update gmpy2 snapshot version

[git]

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 C-API
​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

[git]

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 C-API
​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

[vklein]

rebase on 8.0.beta12

[vdelecroix]

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?

[vklein]

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

[git]

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

​3af335a

trac 22928 add test for mpfr

[git]

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 C-API
​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

[jdemeyer]

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.

[vklein]

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.

[git]

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 C-API
​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

[vklein]

rebase on 8.1.rc0

[jdemeyer]

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.

[jdemeyer]

Working on this...

[jdemeyer]

Is there any reason why we need this many commits? Can I just squash all that to 1 commit?

[vklein]

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

[jdemeyer]

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

[vklein]

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).

[jdemeyer]

Now working on making gmpy2 optional...

[git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

​34ef6dc	Add HAVE_GMPY2 compile-time constant
​a4be146	Conversion between gmpy2 and Sage objects
​480a79d	Cleaner and optional gmpy2 interface

[vklein]

working on complex types (__mpc__ function and constructor)

[jdemeyer]

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 package-version.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 snapshot-14.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

[vdelecroix]

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.

[git]

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

​6d674fc

trac 22928 : fix __mpc__ documentation

[git]

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 compile-time 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

[vklein]

Rebase on the last Trac #22927 HEAD

[vdelecroix]

This branch will not work. The commit in #24215 is 7f06e71 but you rebased it here. So there will be a conflict.

[git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

​34ef6dc	Add HAVE_GMPY2 compile-time 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

[vklein]

rework done

[vdelecroix]

sage: import gmpy2
sage: gmpy2.mpz(1/2)
--> SEGFAULT <--

[jdemeyer]

Replying to vdelecroix:

sage: import gmpy2
sage: gmpy2.mpz(1/2)
--> SEGFAULT <--

That is really a bug in gmpy2, not in Sage.

[vdelecroix]

Indeed ​https://github.com/aleaxit/gmpy/issues/173

[jdemeyer]

I might review this, but only after the dependencies have been merged in a beta.

[jdemeyer]

Rebased to 8.2.beta0

---

New commits:

​0cf649f	Conversion between gmpy2 and Sage objects
​bc582a8	Cleaner and optional gmpy2 interface
​9528b59	trac 22928 : add __mpc__ function to Sage complex numbers

[vklein]

Thanks for the rebase.

[vklein]

rebase on 24417 in progress.

[vklein]

New commits:

​16cc1ad	24417: fix gmpy2 segfaults in conversions
​aaf54d3	Conversion between gmpy2 and Sage objects
​4da4c9b	Cleaner and optional gmpy2 interface
​01978e3	trac 22928 : add __mpc__ function to Sage complex numbers

[vdelecroix]

Some trivial merge/rebase to be done on 8.2.beta2

[git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

​ca21a73	Conversion between gmpy2 and Sage objects
​2c3c349	Cleaner and optional gmpy2 interface
​b1481a2	trac 22928 : add __mpc__ function to Sage complex numbers

[vklein]

Rebase on 8.2.beta2

[vdelecroix]

Sage is crashing

ImportError: /opt/sage/local/lib/python2.7/site-packages/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

@@ +1 @@
+# distutils: libraries = gmp mpfr mpc
+
 from sage.libs.gmp.types cimport *
 from sage.libs.mpfr.types cimport *

[vdelecroix]

You should test different precisions for each possible type (that is RealField(prec), ComplexField(prec) and MPComplexField(prec)).

[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

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'>

[vdelecroix]

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?

[vklein]

src/sage/libs/mpc.pxd

@@ +1 @@
+# distutils: libraries = gmp mpfr mpc
+
 from sage.libs.gmp.types cimport *
 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 ?

[vdelecroix]

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)

[vklein]

Ok i see.

Replying to vdelecroix:

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?

I think it is. I will look at it and open an issue if needed.

[jdemeyer]

Replying to vdelecroix:

src/sage/libs/mpc.pxd

@@ +1 @@
+# distutils: libraries = gmp mpfr mpc
+
 from sage.libs.gmp.types cimport *
 from sage.libs.mpfr.types cimport *

See #24549 for that.

[vklein]

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'>

[vdelecroix]

The ComplexNumber function is wrong in several ways (see #13110). Just ignore it.

[vklein]

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.

[vklein]

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

[vdelecroix]

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.

[vklein]

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 ?

[vdelecroix]

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 floating-point numbers with 53 bits of precision (stupidly named).

[vdelecroix]

That is to say: RR is not a function. It is the object RealField(53).

sage: RR is RealField(53)
True

[git]

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

[vklein]

Add some test with different precision ​comment:93.
Rebase on #24549.

[vdelecroix]

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

[vklein]

Ok i will add this type of doctest.

[vklein]

These test 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 ?

[vdelecroix]

Replying to vklein:

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

Is 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.

[vdelecroix]

Are you sure that precision means the same thing on both sides?

[vklein]

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.

[vdelecroix]

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.

[vdelecroix]

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)

[vklein]

Yes very accurate !

It works fine without .real and .imag. Tests are running now.

[git]

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

​5d044bf

Trac #22928: Add tests and fix a bug with precision

[vdelecroix]

Do you really need the cast <mpc_t>? This is already explicit in the gmpy2 Cython header.

[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 ?

[vdelecroix]

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

@@ -45 +45 @@
 cdef extern from "mpc.h":
     # mpc complexes
     ctypedef struct __mpc_struct:
-        pass
+        mpfr_t re
+        mpfr_t im
     ctypedef __mpc_struct mpc_t[1];
     ctypedef __mpc_struct *mpc_ptr;
     ctypedef const __mpc_struct *mpc_srcptr;

[vklein]

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.

[jdemeyer]

Replying to vklein:

Yes very accurate !

It works fine without .real and .imag. Tests are running now.

This is also obviously an upstream bug.

[jdemeyer]

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.

[jdemeyer]

I think it would be good to at least report the bugs upstream to gmpy2.

[vklein]

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

[git]

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

​b6e2c05

Trac #22928: replace cimport * by cimport mpc_t in

[vklein]

Replying to jdemeyer:

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.

Done

[jdemeyer]

Replying to vklein:

Replying to jdemeyer:

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.

Done

No. I wrote from sage.libs.mpc.types cimport mpc_t and I meant exactly what I wrote.

[vklein]

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/sage-runtests", line 123, in <module>
    from sage.doctest.control import DocTestController
  File "/home/vklein/odk/sage/local/lib/python2.7/site-packages/sage/doctest/control.py", line 33, in <module>
    from .sources import FileDocTestSource, DictAsObject
  File "/home/vklein/odk/sage/local/lib/python2.7/site-packages/sage/doctest/sources.py", line 32, in <module>
    from .parsing import SageDocTestParser
  File "/home/vklein/odk/sage/local/lib/python2.7/site-packages/sage/doctest/parsing.py", line 57, in <module>
    from sage.all import RealIntervalField
  File "/home/vklein/odk/sage/local/lib/python2.7/site-packages/sage/all.py", line 87, in <module>
    from sage.misc.all       import *         # takes a while
  File "/home/vklein/odk/sage/local/lib/python2.7/site-packages/sage/misc/all.py", line 84, in <module>
    from .functional import (additive_order,
  File "/home/vklein/odk/sage/local/lib/python2.7/site-packages/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/site-packages/sage/rings/complex_number.so: undefined symbol: mpc_set_fr_fr

[vdelecroix]

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.

[jdemeyer]

But who is calling mpc_set_fr_fr in that file?

[jdemeyer]

I see. The gmpy2 function GMPy_MPC_From_mpfr requires MPC.

[vdelecroix]

Should we add a distutils directive to the gmpy2.pxd header?

[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 two mpfr_set calls. So maybe we could just do that. I'll try to create a PR for gmpy2.

[vdelecroix]

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 two mpfr_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.

[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 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.

[vdelecroix]

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 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.

And using a mpc from gmpy2 does not imply that you are using mpc!?

[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.

[jdemeyer]

Replying to vdelecroix:

And using a mpc from gmpy2 does not imply that you are using mpc!?

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.

[jdemeyer]

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.

[jdemeyer]

With my patch to gmpy2, the following works:

src/sage/rings/complex_number.pyx

@@ -31 +31 @@
 import math
 import operator
 
-from sage.libs.mpc cimport mpc_t
+from sage.libs.mpc.types cimport mpc_t
 from sage.libs.mpfr cimport *
 from sage.structure.element cimport FieldElement, RingElement, Element, ModuleElement
 from sage.categories.map cimport Map
@@ -193 +193 @@
             elif isinstance(real, complex):
                 real, imag = real.real, real.imag
             elif HAVE_GMPY2 and type(real) is gmpy2.mpc:
-                mpfr_set(self.__re, (<mpc_t>(<gmpy2.mpc>real).c).re, rnd)
-                mpfr_set(self.__im, (<mpc_t>(<gmpy2.mpc>real).c).im, rnd)
+                mpfr_set(self.__re, (<gmpy2.mpc>real).c.re, rnd)
+                mpfr_set(self.__im, (<gmpy2.mpc>real).c.im, rnd)
                 return
             else:
                 imag = 0

[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.

[vdelecroix]

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.

[jdemeyer]

Fine for me!

[git]

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

​1c5685f

Trac #22928: Patchs gmpy2 and update complex_number

[git]

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

[vklein]

Patch are done. The corresponding PRs are merged.

[jdemeyer]

You need to bump the gmpy2 version number for the patches to apply.

[git]

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

​e3a3bf4

Trac #22928 : update version patch p0 to p1

[vklein]

I see, thanks.

[vdelecroix]

The following line misses a # optional - gmpy2

            sage: x = mpfr(R.pi())

[vklein]

Yeah and two others after this one

sage: y = RealField(128)(x) 
sage: mpfr(y) == x

I will rerun tests without gmpy2 package installed

[git]

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

​e9a6fda

Trac #22928 : add some missings # optional - gmpy2

[vklein]

All test without gmpy2 passed, tests with gmpy2 are runnings.

[vdelecroix]

needs review? The branch is fine for me in the current state.

[vklein]

Okay then.

[vdelecroix]

In real_double.pyx you call cimport gmpy2 but does not do gmpy2.import_gmpy2().

[vklein]

Yes but real_double.pyx do cimport sage.ring.integer and the later call gmpy2.import_gmpy2().

[jdemeyer]

Replying to vklein:

Yes but real_double.pyx do cimport sage.ring.integer and the later call gmpy2.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.

[vdelecroix]

Is it ok Jeroen?

[slabbe]

A small remark to say that some warnings associated to gmpy2 appear at each run of sage -b (without gmpy2):

[sagelib-8.2.rc3] sage/rings/complex_double.pyx: cannot find cimported module 'gmpy2'
[sagelib-8.2.rc3] sage/rings/complex_number.pyx: cannot find cimported module 'gmpy2'
[sagelib-8.2.rc3] sage/rings/integer.pyx: cannot find cimported module 'gmpy2'
[sagelib-8.2.rc3] sage/rings/complex_mpc.pyx: cannot find cimported module 'gmpy2'
[sagelib-8.2.rc3] sage/rings/rational.pyx: cannot find cimported module 'gmpy2'
[sagelib-8.2.rc3] sage/rings/real_double.pyx: cannot find cimported module 'gmpy2'
[sagelib-8.2.rc3] sage/rings/real_mpfr.pyx: cannot find cimported module 'gmpy2'

Is it possible to clean this?

[vklein]

According to Jeroen it's harmless and will require a cython patch to fix it.

[jdemeyer]

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.