#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, 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 set to #22927
comment:2 Changed 5 years ago by
- Owner changed from (none) 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 set to u/vklein/conversion_between_gmpy2_and_sage_integers
comment:7 Changed 5 years ago by
- Commit set to 48daf5857197f66b880a563ce27c34cf21959c87
comment:8 follow-up: ↓ 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 changed from 48daf5857197f66b880a563ce27c34cf21959c87 to 54c04ae7c0bf7d48ca89c1e21c4184133e4c6a9c
Branch pushed to git repo; I updated commit sha1. New commits:
54c04ae | Minor improvements
|
comment:10 in reply to: ↑ 8 Changed 5 years ago by
Replying to vdelecroix: Done with commit 54c04ae
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 follow-up: ↓ 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 in reply to: ↑ 18 ; follow-up: ↓ 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 in reply to: ↑ 19 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 changed from 54c04ae7c0bf7d48ca89c1e21c4184133e4c6a9c to 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 changed from 87578d9b02c6f2b88b0cd55d1d27a574316439e7 to ea8aeabc90ff7479e805ca5aa6657bd5c7cf2597
comment:25 Changed 5 years ago by
- Commit changed from ea8aeabc90ff7479e805ca5aa6657bd5c7cf2597 to 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 changed from packages: optional to 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 changed from c28a1eee830fa3937e58f3657814f7c0aaa187dc to 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 changed from 7edcfe46cb8d4abdb922f98d39604475a1350c78 to 6bf31bab5e23764516042ad45047ef633886e01d
Branch pushed to git repo; I updated commit sha1. New commits:
6bf31ba | Use of MPZ_Check and MPQ_Check functions from gmpy2 C-API
|
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 follow-up: ↓ 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 in reply to: ↑ 33 ; follow-up: ↓ 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 in reply to: ↑ 34 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 changed from 6bf31bab5e23764516042ad45047ef633886e01d to 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 changed from 53b8d500d1cfa1d837cb02f3779be15f106e4cb2 to 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 C-API
|
504ab68 | Change way of creating a Rational from a mpz
|
072a15c | add __mpz__() to Rational
|
comment:38 Changed 5 years ago by
I thought that we agreed on comment:32...
comment:40 Changed 5 years ago by
- Commit changed from 072a15c0456d864dfacc21c068f7cc6b98432a9c to 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 changed from Conversion between gmpy2 and sage integers to Conversion between gmpy2 and sage objects
comment:42 Changed 5 years ago by
- Commit changed from 1e404b0b9ef9e2fc2cb6ed4c9b9ec0f44512465c to 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 changed from packages: standard to interfaces
comment:44 Changed 5 years ago by
- Dependencies changed from #22927 to #22927, #23309
comment:45 Changed 5 years ago by
- Commit changed from 09288c73a47d1ce44b5f9e27aab620b75b71c977 to 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 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
|
comment:46 Changed 5 years ago by
- Commit changed from 3e4c6f6267c3e405d6cd4612b616c7d7d14ff63d to 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 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
|
comment:47 Changed 5 years ago by
rebase on 8.0.beta12
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 changed from 043250a6637cb989e53d0852d841367423dc3436 to 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 changed from 3af335aa159ce276538f9d8347baa3a45d1af2bb to 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 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
|
comment:52 in reply to: ↑ description 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 changed from f5a37994b02d98e73cfad3322348b1aaa57c40de to 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 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
|
comment:55 Changed 5 years ago by
rebase on 8.1.rc0
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:57 Changed 5 years ago by
Working on 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 changed from u/vklein/conversion_between_gmpy2_and_sage_integers to u/jdemeyer/conversion_between_gmpy2_and_sage_integers
comment:61 Changed 5 years ago by
- 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:
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 changed from #22927, #23309 to #22927
Now working on making gmpy2 optional...
comment:64 Changed 5 years ago by
- Dependencies changed from #22927 to #22927, #24215
comment:65 Changed 5 years ago by
- Commit changed from c08a7102060473a105ff4106ed75fc615c79c7f4 to 480a79d37226e927b25508f706b1b67717e2e2ee
comment:66 Changed 5 years ago by
working on complex types (__mpc__
function and constructor)
comment:67 Changed 5 years ago by
- Branch changed from u/jdemeyer/conversion_between_gmpy2_and_sage_integers to u/vklein/conversion_between_gmpy2_and_sage_integers
comment:68 Changed 5 years ago by
- Commit changed from 480a79d37226e927b25508f706b1b67717e2e2ee to 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 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
|
comment:69 Changed 5 years ago by
- 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 5 years ago by
- Commit changed from 0ecbffda255da8d6ab042eab194cd295b72e5d13 to 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 changed from 6d674fc2956a9674ccb7349b47c316193788ee84 to 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 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
|
comment:73 Changed 5 years ago by
- Status changed from new to needs_review
comment:74 Changed 5 years ago by
- Description modified (diff)
comment:75 Changed 5 years ago by
- 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 5 years ago by
- Commit changed from e406a9e61f43e832baecf6d2aa300f01362ca8d7 to ddd29a839bd97eabf21deef796472384441e9755
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
|
comment:78 follow-up: ↓ 79 Changed 5 years ago by
- Status changed from needs_review to needs_work
sage: import gmpy2 sage: gmpy2.mpz(1/2) --> SEGFAULT <--
comment:79 in reply to: ↑ 78 Changed 5 years ago by
- 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:80 Changed 5 years ago by
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 changed from u/vklein/conversion_between_gmpy2_and_sage_integers to u/jdemeyer/conversion_between_gmpy2_and_sage_integers
comment:83 Changed 5 years ago by
- Commit changed from ddd29a839bd97eabf21deef796472384441e9755 to 9528b59e0936668ff3c05ec9ced10c322c71e8ba
- Dependencies #22927, #24215 deleted
comment:84 Changed 5 years ago by
Thanks for the rebase.
comment:85 Changed 5 years ago by
- Dependencies set to #24417
comment:86 Changed 5 years ago by
- Status changed from needs_review to needs_work
rebase on 24417 in progress.
comment:87 Changed 5 years ago by
- Branch changed from u/jdemeyer/conversion_between_gmpy2_and_sage_integers to u/vklein/conversion_between_gmpy2_and_sage_integers
comment:88 Changed 5 years ago by
- Commit changed from 9528b59e0936668ff3c05ec9ced10c322c71e8ba to 01978e3951497753e85d0c9a2180a54030cb4732
- Status changed from needs_work to needs_review
comment:89 Changed 5 years ago by
- Status changed from needs_review to needs_work
Some trivial merge/rebase to be done on 8.2.beta2
comment:90 Changed 5 years ago by
- Commit changed from 01978e3951497753e85d0c9a2180a54030cb4732 to b1481a2aed0ca67bf974ef85a772a97d587a7a2d
comment:91 Changed 5 years ago by
- Status changed from needs_work to needs_review
Rebase on 8.2.beta2
comment:92 follow-ups: ↓ 96 ↓ 99 Changed 5 years ago by
- 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 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 follow-ups: ↓ 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 follow-up: ↓ 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 in reply to: ↑ 92 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 in reply to: ↑ 95 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 in reply to: ↑ 92 Changed 5 years ago by
- 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 1 3 from sage.libs.gmp.types cimport * 2 4 from sage.libs.mpfr.types cimport *
See #24549 for that.
comment:100 in reply to: ↑ 94 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 in reply to: ↑ 94 ; follow-up: ↓ 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 in reply to: ↑ 102 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 floating-point 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 changed from b1481a2aed0ca67bf974ef85a772a97d587a7a2d to 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 changed from needs_work to needs_review
Add some test with different precision comment:93.
Rebase on #24549.
comment:110 follow-up: ↓ 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 in reply to: ↑ 110 Changed 5 years ago by
- Status changed from needs_review to needs_work
Ok i will add this type of doctest.
comment:112 follow-up: ↓ 113 Changed 5 years ago by
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 ?
comment:113 in reply to: ↑ 112 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:114 Changed 5 years ago by
Are you sure that precision
means the same thing on both sides?
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 follow-up: ↓ 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 changed from e9188bae6e2e5407101d1ef46f3343b7250ae375 to 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 changed from needs_work to 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 follow-up: ↓ 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 in reply to: ↑ 122 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 in reply to: ↑ 118 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 follow-up: ↓ 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 follow-up: ↓ 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 changed from needs_review to needs_work
comment:129 Changed 5 years ago by
- Reviewers set to Jeroen Demeyer
comment:130 in reply to: ↑ 127 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 changed from 5d044bfb4c8ada79db6e8b0147a1f1120cf49a9d to b6e2c05634879e77912848a89a45a145c1df526f
Branch pushed to git repo; I updated commit sha1. New commits:
b6e2c05 | Trac #22928: replace cimport * by cimport mpc_t in
|
comment:132 in reply to: ↑ 126 ; follow-up: ↓ 133 Changed 5 years ago by
- Status changed from needs_work to 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 in reply to: ↑ 132 Changed 5 years ago by
- Status changed from needs_review to 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/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 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:136 Changed 5 years ago by
But who is calling mpc_set_fr_fr
in that file?
comment:137 Changed 5 years ago by
I see. The gmpy2
function GMPy_MPC_From_mpfr
requires MPC.
comment:138 follow-up: ↓ 139 Changed 5 years ago by
Should we add a distutils directive to the gmpy2.pxd
header?
comment:139 in reply to: ↑ 138 ; follow-up: ↓ 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 in reply to: ↑ 139 ; follow-up: ↓ 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 in reply to: ↑ 140 ; follow-up: ↓ 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 in reply to: ↑ 142 ; follow-up: ↓ 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 follow-up: ↓ 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 in reply to: ↑ 143 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 in reply to: ↑ 144 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 follow-up: ↓ 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 in reply to: ↑ 148 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:151 Changed 5 years ago by
Fine for me!
comment:152 Changed 5 years ago by
- Commit changed from b6e2c05634879e77912848a89a45a145c1df526f to 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 changed from 1c5685f5f11c87b28ec3ad9e76d2e3ae54eef4df to 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 changed from needs_work to needs_review
Patch are done. The corresponding PRs are merged.
comment:155 Changed 5 years ago by
- Status changed from needs_review to needs_work
You need to bump the gmpy2
version number for the patches to apply.
comment:156 Changed 5 years ago by
- Commit changed from dcc7e9567a7df1f37aa795bb7f58ba184985745c to 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 changed from needs_review to needs_work
comment:161 Changed 5 years ago by
- Commit changed from e3a3bf49b965b6f81814efe36d9b0bbe49a43156 to e9a6fdaf5e130e826929c4d9a6617a2128afe3d0
Branch pushed to git repo; I updated commit sha1. New commits:
e9a6fda | Trac #22928 : add some missings # optional - gmpy2
|
comment:162 Changed 5 years ago by
All test without gmpy2 passed, tests with gmpy2 are runnings.
comment:163 Changed 5 years ago by
- Reviewers changed from Jeroen Demeyer to 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 follow-up: ↓ 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 in reply to: ↑ 166 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:168 Changed 5 years ago by
Is it ok Jeroen?
comment:169 Changed 5 years ago by
- Status changed from needs_review to positive_review
comment:170 Changed 5 years ago by
- 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: ↓ 173 Changed 4 years ago by
- 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 4 years ago by
According to Jeroen it's harmless and will require a cython patch to fix it.
comment:173 in reply to: ↑ 171 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