Opened 2 years ago

Closed 21 months ago

Last modified 18 months ago

#22928 closed enhancement (fixed)

Conversion between gmpy2 and sage objects

Reported by: vklein Owned by: vklein
Priority: major Milestone: sage-8.2
Component: interfaces Keywords: thursdaysbdx
Cc: vdelecroix Merged in:
Authors: Vincent Klein Reviewers: Jeroen Demeyer, Vincent Delecroix
Report Upstream: N/A Work issues:
Branch: e9a6fda (Commits) Commit:
Dependencies: #24549 Stopgaps:

Description (last modified by jdemeyer)

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:

Change History (173)

comment:1 Changed 2 years ago by vklein

  • Dependencies set to #22927

comment:2 Changed 2 years ago by vklein

  • Owner changed from (none) to vklein

comment:3 Changed 2 years ago by vdelecroix

  • Keywords jsb++ added

comment:4 Changed 2 years ago by vdelecroix

  • Keywords thursdaysbdx added; jsb++ removed

comment:5 Changed 2 years ago by vklein

  • Description modified (diff)

comment:6 Changed 2 years ago by vklein

  • Branch set to u/vklein/conversion_between_gmpy2_and_sage_integers

comment:7 Changed 2 years ago by git

  • Commit set to 48daf5857197f66b880a563ce27c34cf21959c87

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

48daf58Add path of gmpy2 in include_dirs

comment:8 follow-up: Changed 2 years ago by 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?

Last edited 2 years ago by vdelecroix (previous) (diff)

comment:9 Changed 2 years ago by git

  • Commit changed from 48daf5857197f66b880a563ce27c34cf21959c87 to 54c04ae7c0bf7d48ca89c1e21c4184133e4c6a9c

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

54c04aeMinor improvements

comment:10 in reply to: ↑ 8 Changed 2 years ago by vklein

Replying to vdelecroix: Done with commit 54c04ae

Last edited 2 years ago by vklein (previous) (diff)

comment:11 Changed 2 years ago by vklein

  • Description modified (diff)

comment:12 Changed 2 years ago by vklein

  • Description modified (diff)

comment:13 Changed 2 years ago by vklein

  • Description modified (diff)

comment:14 Changed 2 years ago by vdelecroix

  • Description modified (diff)

comment:15 Changed 2 years ago by vdelecroix

  • Description modified (diff)

comment:16 Changed 2 years ago by vdelecroix

  • Description modified (diff)

comment:17 Changed 2 years ago by 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)    
    
Last edited 2 years ago by vdelecroix (previous) (diff)

comment:18 follow-up: Changed 2 years ago by 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?

comment:19 in reply to: ↑ 18 ; follow-up: Changed 2 years ago by 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.

comment:20 in reply to: ↑ 19 Changed 2 years ago by 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)

comment:21 Changed 2 years ago by vklein

  • Description modified (diff)

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

comment:22 Changed 2 years ago by git

  • Commit changed from 54c04ae7c0bf7d48ca89c1e21c4184133e4c6a9c to 87578d9b02c6f2b88b0cd55d1d27a574316439e7

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

87578d9Add path of gmpy2 in include_dirs

comment:23 Changed 2 years ago by jdemeyer

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

comment:24 Changed 2 years ago by git

  • Commit changed from 87578d9b02c6f2b88b0cd55d1d27a574316439e7 to ea8aeabc90ff7479e805ca5aa6657bd5c7cf2597

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

8cbbe31update checksum and package version
ea8aeabInteger and rational constructors support gmpy2 params

comment:25 Changed 2 years ago by git

  • Commit changed from ea8aeabc90ff7479e805ca5aa6657bd5c7cf2597 to c28a1eee830fa3937e58f3657814f7c0aaa187dc

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

c28a1eeremove OptionalExtension sage.libs.gmpy2

comment:26 Changed 2 years ago by vdelecroix

  • Component changed from packages: optional to packages: standard

comment:27 Changed 2 years ago by 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))
    

comment:28 Changed 2 years ago by git

  • Commit changed from c28a1eee830fa3937e58f3657814f7c0aaa187dc to 7edcfe46cb8d4abdb922f98d39604475a1350c78

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

db38086remove get_gmpy2_path()
7edcfe4Add few doctests

comment:29 Changed 2 years ago by vklein

  • Description modified (diff)

comment:30 Changed 2 years ago by vklein

  • Description modified (diff)

comment:31 Changed 2 years ago by git

  • Commit changed from 7edcfe46cb8d4abdb922f98d39604475a1350c78 to 6bf31bab5e23764516042ad45047ef633886e01d

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

6bf31baUse of MPZ_Check and MPQ_Check functions from gmpy2 C-API

comment:32 Changed 2 years ago by 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))

comment:33 follow-up: Changed 2 years ago by 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.

comment:34 in reply to: ↑ 33 ; follow-up: Changed 2 years ago by 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.

comment:35 in reply to: ↑ 34 Changed 2 years ago by 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.

comment:36 Changed 2 years ago by git

  • Commit changed from 6bf31bab5e23764516042ad45047ef633886e01d to 53b8d500d1cfa1d837cb02f3779be15f106e4cb2

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

53b8d50Change way of creating a Rational from a mpz

comment:37 Changed 2 years ago by git

  • Commit changed from 53b8d500d1cfa1d837cb02f3779be15f106e4cb2 to 072a15c0456d864dfacc21c068f7cc6b98432a9c

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

8bbf2d3update checksum, snapshot version update in setup.py
08c7a72Add path of gmpy2 in include_dirs
359a0ffupdate checksum and package version
0d4fd2eInteger and rational constructors support gmpy2 params
7c66a7cremove OptionalExtension sage.libs.gmpy2
d143dderemove get_gmpy2_path()
2c56a3fAdd few doctests
e5264fdUse of MPZ_Check and MPQ_Check functions from gmpy2 C-API
504ab68Change way of creating a Rational from a mpz
072a15cadd __mpz__() to Rational

comment:38 Changed 2 years ago by vdelecroix

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

comment:39 Changed 2 years ago by vklein

A merge error

Last edited 2 years ago by vklein (previous) (diff)

comment:40 Changed 2 years ago by git

  • Commit changed from 072a15c0456d864dfacc21c068f7cc6b98432a9c to 1e404b0b9ef9e2fc2cb6ed4c9b9ec0f44512465c

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

1e404b0trac 29928 Change way of creating a Rational from a mpz

comment:41 Changed 2 years ago by vdelecroix

  • Description modified (diff)
  • Summary changed from Conversion between gmpy2 and sage integers to Conversion between gmpy2 and sage objects

comment:42 Changed 2 years ago by git

  • Commit changed from 1e404b0b9ef9e2fc2cb6ed4c9b9ec0f44512465c to 09288c73a47d1ce44b5f9e27aab620b75b71c977

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

09288c7trac 22928 update gmpy2 snapshot version

comment:43 Changed 2 years ago by vdelecroix

  • Component changed from packages: standard to interfaces

comment:44 Changed 2 years ago by vdelecroix

  • Dependencies changed from #22927 to #22927, #23309

comment:45 Changed 2 years ago by git

  • Commit changed from 09288c73a47d1ce44b5f9e27aab620b75b71c977 to 3e4c6f6267c3e405d6cd4612b616c7d7d14ff63d

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

3e5ded6Add few doctests
758d7fdUse of MPZ_Check and MPQ_Check functions from gmpy2 C-API
d96f7bbChange way of creating a Rational from a mpz
0b8c45eadd __mpz__() to Rational
8f90a09trac 29928 Change way of creating a Rational from a mpz
6263747trac 22928 update gmpy2 snapshot version
5b8b77btrac 22928 RealNumber and RealDoubleElement and gmpy2
6e54851trac 22928 add __mpfr__ to RealNumber class
7b75526trac 22928 merge trac 22927
3e4c6f6trac 22928 add __mpfr__ to RealDoubleElement class

comment:46 Changed 2 years ago by git

  • Commit changed from 3e4c6f6267c3e405d6cd4612b616c7d7d14ff63d to 043250a6637cb989e53d0852d841367423dc3436

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

601935bAdd few doctests
6868a33Use of MPZ_Check and MPQ_Check functions from gmpy2 C-API
c91ada4Change way of creating a Rational from a mpz
2d68635add __mpz__() to Rational
48d4ce2trac 29928 Change way of creating a Rational from a mpz
dffb83etrac 22928 update gmpy2 snapshot version
2fa80f5trac 22928 RealNumber and RealDoubleElement and gmpy2
32b4b86trac 22928 add __mpfr__ to RealNumber class
2553307trac 22928 merge trac 22927
043250atrac 22928 add __mpfr__ to RealDoubleElement class

comment:47 Changed 2 years ago by vklein

rebase on 8.0.beta12

comment:48 Changed 2 years ago by 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?

comment:49 Changed 2 years ago by 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

Last edited 2 years ago by vklein (previous) (diff)

comment:50 Changed 2 years ago by git

  • Commit changed from 043250a6637cb989e53d0852d841367423dc3436 to 3af335aa159ce276538f9d8347baa3a45d1af2bb

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

3af335atrac 22928 add test for mpfr

comment:51 Changed 2 years ago by git

  • Commit changed from 3af335aa159ce276538f9d8347baa3a45d1af2bb to f5a37994b02d98e73cfad3322348b1aaa57c40de

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

f821d9eUse of MPZ_Check and MPQ_Check functions from gmpy2 C-API
24988eeChange way of creating a Rational from a mpz
f0e2114add __mpz__() to Rational
2299a79trac 29928 Change way of creating a Rational from a mpz
d38e2dctrac 22928 update gmpy2 snapshot version
2183a91trac 22928 RealNumber and RealDoubleElement and gmpy2
c607d49trac 22928 add __mpfr__ to RealNumber class
e3c99f1trac 22928 merge trac 22927
9b5df1btrac 22928 add __mpfr__ to RealDoubleElement class
f5a3799trac 22928 add test for mpfr

comment:52 in reply to: ↑ description Changed 2 years ago by 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.

comment:53 Changed 2 years ago by 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.

comment:54 Changed 2 years ago by git

  • Commit changed from f5a37994b02d98e73cfad3322348b1aaa57c40de to 99b77348d7189300b0fa1e1d04cdb9cbcb4b0765

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

ba3bc11Use of MPZ_Check and MPQ_Check functions from gmpy2 C-API
6a7f661Change way of creating a Rational from a mpz
e98845cadd __mpz__() to Rational
6161e79trac 29928 Change way of creating a Rational from a mpz
aac3bf4trac 22928 update gmpy2 snapshot version
4a9e3b9trac 22928 RealNumber and RealDoubleElement and gmpy2
4c25530trac 22928 add __mpfr__ to RealNumber class
1129ecatrac 22928 merge trac 22927
0ddcc85trac 22928 add __mpfr__ to RealDoubleElement class
99b7734trac 22928 add test for mpfr

comment:55 Changed 2 years ago by vklein

rebase on 8.1.rc0

comment:56 Changed 2 years ago by 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.

comment:57 Changed 2 years ago by jdemeyer

Working on this...

comment:58 Changed 2 years ago by jdemeyer

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

comment:59 Changed 2 years ago by 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

comment:60 Changed 2 years ago by jdemeyer

  • Branch changed from u/vklein/conversion_between_gmpy2_and_sage_integers to u/jdemeyer/conversion_between_gmpy2_and_sage_integers

comment:61 Changed 2 years ago by jdemeyer

  • Commit changed from 99b77348d7189300b0fa1e1d04cdb9cbcb4b0765 to 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:

60f61ffConversion between gmpy2 and Sage objects
c08a710Cleaner gmpy2 interface

comment:62 Changed 2 years ago by 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).

comment:63 Changed 2 years ago by jdemeyer

  • Dependencies changed from #22927, #23309 to #22927

Now working on making gmpy2 optional...

comment:64 Changed 2 years ago by jdemeyer

  • Dependencies changed from #22927 to #22927, #24215

comment:65 Changed 2 years ago by git

  • Commit changed from c08a7102060473a105ff4106ed75fc615c79c7f4 to 480a79d37226e927b25508f706b1b67717e2e2ee

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

34ef6dcAdd HAVE_GMPY2 compile-time constant
a4be146Conversion between gmpy2 and Sage objects
480a79dCleaner and optional gmpy2 interface

comment:66 Changed 2 years ago by vklein

working on complex types (__mpc__ function and constructor)

Last edited 2 years ago by vklein (previous) (diff)

comment:67 Changed 2 years ago by vklein

  • Branch changed from u/jdemeyer/conversion_between_gmpy2_and_sage_integers to u/vklein/conversion_between_gmpy2_and_sage_integers

comment:68 Changed 2 years ago by jdemeyer

  • Commit changed from 480a79d37226e927b25508f706b1b67717e2e2ee to 0ecbffda255da8d6ab042eab194cd295b72e5d13

New commits:

d6072cause a snapshot version of gmpy2
ce7a082update checksum, snapshot version update in setup.py
2eca651New snapshot version, update checksum and package-version.txt
bd257c1Update checksum and snapshot version
32cc1abtrac 22927 update gmpy2 snapshot version
aab4169trac 22927 update gmpy2 package version
0ba3ffbtrac 22927: update package version to snapshot-14.11.17
3bf081fMerge branch 'u/vklein/add_gmpy2_as_an_optional_package' of git://trac.sagemath.org/sage into t/22928/conversion_between_gmpy2_and_sage_integers
0ecbffdtrac 22928 : add __mpc__ function to Sage complex numbers

comment:69 Changed 2 years ago by vdelecroix

  • Milestone changed from sage-8.0 to sage-8.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 2 years ago by git

  • Commit changed from 0ecbffda255da8d6ab042eab194cd295b72e5d13 to 6d674fc2956a9674ccb7349b47c316193788ee84

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

6d674fctrac 22928 : fix __mpc__ documentation

comment:71 Changed 23 months ago by git

  • Commit changed from 6d674fc2956a9674ccb7349b47c316193788ee84 to e406a9e61f43e832baecf6d2aa300f01362ca8d7

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

e000563Trac #22927 gmpy2 update package version to 2.1.0a1
82d9462Add HAVE_GMPY2 compile-time constant
2d01807Conversion between gmpy2 and Sage objects
b9c7c43Cleaner and optional gmpy2 interface
476ddc2trac 22928 : add __mpc__ function to Sage complex numbers
e406a9etrac 22928 : fix __mpc__ documentation

comment:72 Changed 23 months ago by vklein

  • Description modified (diff)

Rebase on the last Trac #22927 HEAD

comment:73 Changed 23 months ago by vklein

  • Status changed from new to needs_review

comment:74 Changed 23 months ago by vdelecroix

  • Description modified (diff)

comment:75 Changed 23 months ago by vdelecroix

  • Status changed from needs_review to 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 23 months ago by git

  • Commit changed from e406a9e61f43e832baecf6d2aa300f01362ca8d7 to ddd29a839bd97eabf21deef796472384441e9755

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

34ef6dcAdd HAVE_GMPY2 compile-time constant
158d984Force absolute import in have_module()
7f06e71Fix documentation
9dceca0Merge branch 'u/jdemeyer/ticket/24215' of git://trac.sagemath.org/sage into 22928
8bed52cConversion between gmpy2 and Sage objects
4f79ed7Cleaner and optional gmpy2 interface
8938549trac 22928 : add __mpc__ function to Sage complex numbers
ddd29a8trac 22928 : fix __mpc__ documentation

comment:77 Changed 23 months ago by vklein

  • Status changed from needs_work to needs_review

rework done

comment:78 follow-up: Changed 23 months ago by vdelecroix

  • Status changed from needs_review to needs_work
sage: import gmpy2
sage: gmpy2.mpz(1/2)
--> SEGFAULT <--

comment:79 in reply to: ↑ 78 Changed 23 months ago by jdemeyer

  • Status changed from needs_work to 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 23 months ago by jdemeyer

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

comment:82 Changed 22 months ago by jdemeyer

  • Branch changed from u/vklein/conversion_between_gmpy2_and_sage_integers to u/jdemeyer/conversion_between_gmpy2_and_sage_integers

comment:83 Changed 22 months ago by jdemeyer

  • Commit changed from ddd29a839bd97eabf21deef796472384441e9755 to 9528b59e0936668ff3c05ec9ced10c322c71e8ba
  • Dependencies #22927, #24215 deleted

Rebased to 8.2.beta0


New commits:

0cf649fConversion between gmpy2 and Sage objects
bc582a8Cleaner and optional gmpy2 interface
9528b59trac 22928 : add __mpc__ function to Sage complex numbers

comment:84 Changed 22 months ago by vklein

Thanks for the rebase.

comment:85 Changed 22 months ago by vdelecroix

  • Dependencies set to #24417

comment:86 Changed 22 months ago by vklein

  • Status changed from needs_review to needs_work

rebase on 24417 in progress.

Last edited 22 months ago by vklein (previous) (diff)

comment:87 Changed 22 months ago by vklein

  • Branch changed from u/jdemeyer/conversion_between_gmpy2_and_sage_integers to u/vklein/conversion_between_gmpy2_and_sage_integers

comment:88 Changed 22 months ago by vklein

  • Commit changed from 9528b59e0936668ff3c05ec9ced10c322c71e8ba to 01978e3951497753e85d0c9a2180a54030cb4732
  • Status changed from needs_work to needs_review

New commits:

16cc1ad24417: fix gmpy2 segfaults in conversions
aaf54d3Conversion between gmpy2 and Sage objects
4da4c9bCleaner and optional gmpy2 interface
01978e3trac 22928 : add __mpc__ function to Sage complex numbers

comment:89 Changed 22 months ago by vdelecroix

  • Status changed from needs_review to needs_work

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

comment:90 Changed 21 months ago by git

  • Commit changed from 01978e3951497753e85d0c9a2180a54030cb4732 to b1481a2aed0ca67bf974ef85a772a97d587a7a2d

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

ca21a73Conversion between gmpy2 and Sage objects
2c3c349Cleaner and optional gmpy2 interface
b1481a2trac 22928 : add __mpc__ function to Sage complex numbers

comment:91 Changed 21 months ago by vklein

  • Status changed from needs_work to needs_review

Rebase on 8.2.beta2

comment:92 follow-ups: Changed 21 months ago by vdelecroix

  • Status changed from needs_review to needs_work

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

    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
    13from sage.libs.gmp.types cimport *
    24from sage.libs.mpfr.types cimport *

comment:93 Changed 21 months ago by vdelecroix

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

comment:94 follow-ups: Changed 21 months ago by 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'>

comment:95 follow-up: Changed 21 months ago by 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?

comment:96 in reply to: ↑ 92 Changed 21 months ago by vklein

  • 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
    13from sage.libs.gmp.types cimport *
    24from 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 21 months ago by 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)

Last edited 21 months ago by vdelecroix (previous) (diff)

comment:98 in reply to: ↑ 95 Changed 21 months ago by 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.

comment:99 in reply to: ↑ 92 Changed 21 months ago by jdemeyer

  • Dependencies changed from #24417 to #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
    13from sage.libs.gmp.types cimport *
    24from sage.libs.mpfr.types cimport *

See #24549 for that.

comment:100 in reply to: ↑ 94 Changed 21 months ago by 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'>
Last edited 21 months ago by vklein (previous) (diff)

comment:101 Changed 21 months ago by vdelecroix

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

comment:102 in reply to: ↑ 94 ; follow-up: Changed 21 months ago by 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.

comment:103 Changed 21 months ago by 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

comment:104 Changed 21 months ago by 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.

comment:105 Changed 21 months ago by 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 ?

comment:106 in reply to: ↑ 102 Changed 21 months ago by 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).

comment:107 Changed 21 months ago by vdelecroix

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

sage: RR is RealField(53)
True

comment:108 Changed 21 months ago by git

  • Commit changed from b1481a2aed0ca67bf974ef85a772a97d587a7a2d to e9188bae6e2e5407101d1ef46f3343b7250ae375

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

b1047ccClean up MPC declarations
d9308b2Conversion between gmpy2 and Sage objects
074acd6Cleaner and optional gmpy2 interface
2604a37trac 22928 : add __mpc__ function to Sage complex numbers
e9188baTrac #22928: Test different precisions for each possible type

comment:109 Changed 21 months ago by vklein

  • Status changed from needs_work to needs_review

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

comment:110 follow-up: Changed 21 months ago by 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

comment:111 in reply to: ↑ 110 Changed 21 months ago by vklein

  • Status changed from needs_review to needs_work

Ok i will add this type of doctest.

comment:112 follow-up: Changed 21 months ago by 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
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 ?

Last edited 21 months ago by vklein (previous) (diff)

comment:113 in reply to: ↑ 112 Changed 21 months ago by 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.

comment:114 Changed 21 months ago by vdelecroix

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

comment:115 Changed 21 months ago by 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.

Last edited 21 months ago by vklein (previous) (diff)

comment:116 Changed 21 months ago by 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.

comment:117 Changed 21 months ago by 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)

comment:118 follow-up: Changed 21 months ago by vklein

Yes very accurate !

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

comment:119 Changed 21 months ago by git

  • Commit changed from e9188bae6e2e5407101d1ef46f3343b7250ae375 to 5d044bfb4c8ada79db6e8b0147a1f1120cf49a9d

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

5d044bfTrac #22928: Add tests and fix a bug with precision

comment:120 Changed 21 months ago by vklein

  • Status changed from needs_work to needs_review
Last edited 21 months ago by vklein (previous) (diff)

comment:121 Changed 21 months ago by vdelecroix

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

comment:122 follow-up: Changed 21 months ago by 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 ?

Last edited 21 months ago by vklein (previous) (diff)

comment:123 in reply to: ↑ 122 Changed 21 months ago by 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

    diff --git a/src/gmpy2.pxd b/src/gmpy2.pxd
    index 7cdcc72..d6cac1b 100644
    a b cdef extern from "mpfr.h": 
    4545cdef extern from "mpc.h":
    4646    # mpc complexes
    4747    ctypedef struct __mpc_struct:
    48         pass
     48        mpfr_t re
     49        mpfr_t im
    4950    ctypedef __mpc_struct mpc_t[1];
    5051    ctypedef __mpc_struct *mpc_ptr;
    5152    ctypedef const __mpc_struct *mpc_srcptr;

comment:124 Changed 21 months ago by 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.

Last edited 21 months ago by vklein (previous) (diff)

comment:125 in reply to: ↑ 118 Changed 21 months ago by 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.

comment:126 follow-up: Changed 21 months ago by 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.

comment:127 follow-up: Changed 21 months ago by jdemeyer

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

comment:128 Changed 21 months ago by jdemeyer

  • Status changed from needs_review to needs_work

comment:129 Changed 21 months ago by jdemeyer

  • Reviewers set to Jeroen Demeyer

comment:130 in reply to: ↑ 127 Changed 21 months ago by 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

comment:131 Changed 21 months ago by git

  • Commit changed from 5d044bfb4c8ada79db6e8b0147a1f1120cf49a9d to b6e2c05634879e77912848a89a45a145c1df526f

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

b6e2c05Trac #22928: replace cimport * by cimport mpc_t in

comment:132 in reply to: ↑ 126 ; follow-up: Changed 21 months ago by vklein

  • Status changed from needs_work to needs_review

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

comment:133 in reply to: ↑ 132 Changed 21 months ago by jdemeyer

  • Status changed from needs_review to needs_work

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.

comment:134 Changed 21 months ago by 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

comment:135 Changed 21 months ago by 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.

comment:136 Changed 21 months ago by jdemeyer

But who is calling mpc_set_fr_fr in that file?

comment:137 Changed 21 months ago by jdemeyer

I see. The gmpy2 function GMPy_MPC_From_mpfr requires MPC.

comment:138 follow-up: Changed 21 months ago by vdelecroix

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

comment:139 in reply to: ↑ 138 ; follow-up: Changed 21 months ago by 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.

comment:140 in reply to: ↑ 139 ; follow-up: Changed 21 months ago by 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.

comment:141 Changed 21 months ago by jdemeyer

  • Description modified (diff)

comment:142 in reply to: ↑ 140 ; follow-up: Changed 21 months ago by 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.

comment:143 in reply to: ↑ 142 ; follow-up: Changed 21 months ago by 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!?

comment:144 follow-up: Changed 21 months ago by 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.

Last edited 21 months ago by vklein (previous) (diff)

comment:145 in reply to: ↑ 143 Changed 21 months ago by 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.

comment:146 in reply to: ↑ 144 Changed 21 months ago by 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.

comment:147 Changed 21 months ago by jdemeyer

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 
    3131import math
    3232import operator
    3333
    34 from sage.libs.mpc cimport mpc_t
     34from sage.libs.mpc.types cimport mpc_t
    3535from sage.libs.mpfr cimport *
    3636from sage.structure.element cimport FieldElement, RingElement, Element, ModuleElement
    3737from sage.categories.map cimport Map
    cdef class ComplexNumber(sage.structure.element.FieldElement): 
    193193            elif isinstance(real, complex):
    194194                real, imag = real.real, real.imag
    195195            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)
    198198                return
    199199            else:
    200200                imag = 0

comment:148 follow-up: Changed 21 months ago by 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.

comment:149 in reply to: ↑ 148 Changed 21 months ago by 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.

comment:150 Changed 21 months ago by jdemeyer

  • Description modified (diff)

comment:151 Changed 21 months ago by jdemeyer

Fine for me!

comment:152 Changed 21 months ago by git

  • Commit changed from b6e2c05634879e77912848a89a45a145c1df526f to 1c5685f5f11c87b28ec3ad9e76d2e3ae54eef4df

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

1c5685fTrac #22928: Patchs gmpy2 and update complex_number

comment:153 Changed 21 months ago by git

  • Commit changed from 1c5685f5f11c87b28ec3ad9e76d2e3ae54eef4df to dcc7e9567a7df1f37aa795bb7f58ba184985745c

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

dcc7e95Trac #22928: Patchs gmpy2 and update complex_number

comment:154 Changed 21 months ago by vklein

  • Status changed from needs_work to needs_review

Patch are done. The corresponding PRs are merged.

Last edited 21 months ago by vklein (previous) (diff)

comment:155 Changed 21 months ago by jdemeyer

  • Status changed from needs_review to needs_work

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

comment:156 Changed 21 months ago by git

  • Commit changed from dcc7e9567a7df1f37aa795bb7f58ba184985745c to e3a3bf49b965b6f81814efe36d9b0bbe49a43156

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

e3a3bf4Trac #22928 : update version patch p0 to p1

comment:157 Changed 21 months ago by vklein

  • Status changed from needs_work to needs_review

I see, thanks.

comment:158 Changed 21 months ago by vdelecroix

The following line misses a # optional - gmpy2

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

comment:159 Changed 21 months ago by 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

comment:160 Changed 21 months ago by vklein

  • Status changed from needs_review to needs_work

comment:161 Changed 21 months ago by git

  • Commit changed from e3a3bf49b965b6f81814efe36d9b0bbe49a43156 to e9a6fdaf5e130e826929c4d9a6617a2128afe3d0

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

e9a6fdaTrac #22928 : add some missings # optional - gmpy2

comment:162 Changed 21 months ago by vklein

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

comment:163 Changed 21 months ago by vdelecroix

  • Reviewers changed from Jeroen Demeyer to Jeroen Demeyer, Vincent Delecroix

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

comment:164 Changed 21 months ago by vklein

  • Status changed from needs_work to needs_review

Okay then.

comment:165 Changed 21 months ago by vdelecroix

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

comment:166 follow-up: Changed 21 months ago by vklein

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

comment:167 in reply to: ↑ 166 Changed 21 months ago by 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.

comment:168 Changed 21 months ago by vdelecroix

Is it ok Jeroen?

comment:169 Changed 21 months ago by jdemeyer

  • Status changed from needs_review to positive_review

comment:170 Changed 21 months ago by vbraun

  • Branch changed from u/vklein/conversion_between_gmpy2_and_sage_integers to e9a6fdaf5e130e826929c4d9a6617a2128afe3d0
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:171 follow-up: Changed 18 months ago by slabbe

  • Commit e9a6fdaf5e130e826929c4d9a6617a2128afe3d0 deleted

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?

comment:172 Changed 18 months ago by vklein

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

comment:173 in reply to: ↑ 171 Changed 18 months ago by 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.

Note: See TracTickets for help on using tickets.