Opened 5 years ago
Closed 5 years ago
#25128 closed enhancement (fixed)
Have py_scalar_to_element convert gmpy2 numbers
Reported by:  vklein  Owned by:  vklein 

Priority:  major  Milestone:  sage8.2 
Component:  coercion  Keywords:  gmpy2 
Cc:  vdelecroix  Merged in:  
Authors:  Vincent Klein  Reviewers:  Vincent Delecroix, Jeroen Demeyer 
Report Upstream:  N/A  Work issues:  
Branch:  ff11b13 (Commits, GitHub, GitLab)  Commit:  ff11b132b88f15ee73cc728e480395030728f17f 
Dependencies:  Stopgaps: 
Description (last modified by )
Currently py_scalar_to_element
function (coerce.pyx
) doesn't convert gmpy2 numbers
. py_scalar_to_element
should work with gmpy2
the same way it does with numpy
.
Change History (23)
comment:1 Changed 5 years ago by
Owner:  set to vklein 

comment:2 Changed 5 years ago by
Branch:  → u/vklein/have_py_scalar_to_element_convert_gmpy2_numbers 

comment:3 Changed 5 years ago by
Authors:  → Vincent Klein 

Commit:  → 8555214b0b83bffe03ea7001e68b685ce8a1f458 
Status:  new → needs_review 
comment:4 Changed 5 years ago by
Reviewers:  → Vincent Delecroix 

Status:  needs_review → needs_work 
You have C functions for type checking: MPZ_Check
, MPQ_Check
, MPFR_Check
and MPC_Check
.
comment:5 Changed 5 years ago by
(this is the kind of things that are of course not optimized by Cython)
comment:6 Changed 5 years ago by
Commit:  8555214b0b83bffe03ea7001e68b685ce8a1f458 → c3a55ec6e3ed5f1c2820d7438ab02bc88f04ebb4 

Branch pushed to git repo; I updated commit sha1. New commits:
c3a55ec  Trac #25128: Change ``gmpy2``'s type checking with ...

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

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

comment:9 Changed 5 years ago by
Commit:  c3a55ec6e3ed5f1c2820d7438ab02bc88f04ebb4 → 95da017b881c8c44ae135a0105c067d0529e3657 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
95da017  Trac #25128: Change ``gmpy2``'s type checking with ...

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

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

There needs to be a linebreak after ::
+ Test gmpy2's types:: + sage: import gmpy2 # optional  gmpy2
comment:12 Changed 5 years ago by
Commit:  95da017b881c8c44ae135a0105c067d0529e3657 → 783c01de5c2ae05fd5245a8d0ffef4363981adf4 

Branch pushed to git repo; I updated commit sha1. New commits:
783c01d  Trac #25128: Fix missing linebreak after ``::``

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

Status:  needs_review → positive_review 
comment:15 Changed 5 years ago by
Why MPZ_Check(x)
instead of type(x) is gmpy2.mpz
? There is nothing really wrong with MPZ_Check
but all other places in Sage use the type
check. For example in src/sage/rings/complex_double.pyx
:
if HAVE_GMPY2 and type(x) is gmpy2.mpc:
If you use only type checks, you can also remove the import_gmpy2
.
comment:16 Changed 5 years ago by
I just benchmarked it and MPZ_Check()
is slightly slower than the type()
check. This is probably because the import_gmpy2
machinery gives a slight overhead compared to the direct import that Cython does.
comment:17 Changed 5 years ago by
Status:  positive_review → needs_work 

Ok i will reverse to type()
check then. I see no reason to keep MPZ_Check()
if it's not faster.
comment:18 Changed 5 years ago by
Commit:  783c01de5c2ae05fd5245a8d0ffef4363981adf4 → ff11b132b88f15ee73cc728e480395030728f17f 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
ff11b13  Trac #25128: ``gmpy2``'s types and ``py_scalar_to_element``

comment:20 Changed 5 years ago by
Status:  positive_review → needs_review 

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

Status:  needs_review → positive_review 
Sure it's fine.
comment:23 Changed 5 years ago by
Branch:  u/vklein/have_py_scalar_to_element_convert_gmpy2_numbers → ff11b132b88f15ee73cc728e480395030728f17f 

Resolution:  → fixed 
Status:  positive_review → closed 
New commits:
Trac #25128: ``gmpy2``'s types and ``py_scalar_to_element``