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: sage-8.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:

Status badges

Description (last modified by vdelecroix)

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 vklein

Owner: set to vklein

comment:2 Changed 5 years ago by vklein

Branch: u/vklein/have_py_scalar_to_element_convert_gmpy2_numbers

comment:3 Changed 5 years ago by vklein

Authors: Vincent Klein
Commit: 8555214b0b83bffe03ea7001e68b685ce8a1f458
Status: newneeds_review

New commits:

8555214Trac #25128: ``gmpy2``'s types and ``py_scalar_to_element``

comment:4 Changed 5 years ago by vdelecroix

Reviewers: Vincent Delecroix
Status: needs_reviewneeds_work

You have C functions for type checking: MPZ_Check, MPQ_Check, MPFR_Check and MPC_Check.

comment:5 Changed 5 years ago by vdelecroix

(this is the kind of things that are of course not optimized by Cython)

comment:6 Changed 5 years ago by git

Commit: 8555214b0b83bffe03ea7001e68b685ce8a1f458c3a55ec6e3ed5f1c2820d7438ab02bc88f04ebb4

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

c3a55ecTrac #25128: Change ``gmpy2``'s type checking with ...

comment:7 Changed 5 years ago by vklein

Status: needs_workneeds_review

comment:8 Changed 5 years ago by vklein

Status: needs_reviewneeds_work

comment:9 Changed 5 years ago by git

Commit: c3a55ec6e3ed5f1c2820d7438ab02bc88f04ebb495da017b881c8c44ae135a0105c067d0529e3657

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

95da017Trac #25128: Change ``gmpy2``'s type checking with ...

comment:10 Changed 5 years ago by vklein

Status: needs_workneeds_review

comment:11 Changed 5 years ago by vdelecroix

Status: needs_reviewneeds_work

There needs to be a linebreak after ::

+    Test gmpy2's types::
+        sage: import gmpy2                               # optional - gmpy2 

comment:12 Changed 5 years ago by git

Commit: 95da017b881c8c44ae135a0105c067d0529e3657783c01de5c2ae05fd5245a8d0ffef4363981adf4

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

783c01dTrac #25128: Fix missing linebreak after ``::``

comment:13 Changed 5 years ago by vklein

Status: needs_workneeds_review

Fix missing linebreak.

comment:14 Changed 5 years ago by vdelecroix

Description: modified (diff)
Status: needs_reviewpositive_review

comment:15 Changed 5 years ago by jdemeyer

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 jdemeyer

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.

Last edited 5 years ago by jdemeyer (previous) (diff)

comment:17 Changed 5 years ago by vklein

Status: positive_reviewneeds_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 git

Commit: 783c01de5c2ae05fd5245a8d0ffef4363981adf4ff11b132b88f15ee73cc728e480395030728f17f

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

ff11b13Trac #25128: ``gmpy2``'s types and ``py_scalar_to_element``

comment:19 Changed 5 years ago by vklein

Status: needs_workpositive_review

previous commits squashed.

comment:20 Changed 5 years ago by vklein

Status: positive_reviewneeds_review

comment:21 Changed 5 years ago by jdemeyer

Fine for me but I leave the final review to Vincent Delecroix.

comment:22 Changed 5 years ago by vdelecroix

Reviewers: Vincent DelecroixVincent Delecroix, Jeroen Demeyer
Status: needs_reviewpositive_review

Sure it's fine.

comment:23 Changed 5 years ago by vbraun

Branch: u/vklein/have_py_scalar_to_element_convert_gmpy2_numbersff11b132b88f15ee73cc728e480395030728f17f
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.