Opened 6 years ago
Closed 6 years ago
#17862 closed enhancement (fixed)
Remove use of PY_IS_NUMERIC
Reported by:  jdemeyer  Owned by:  

Priority:  major  Milestone:  sage6.6 
Component:  c_lib  Keywords:  
Cc:  Merged in:  
Authors:  Jeroen Demeyer  Reviewers:  Vincent Delecroix 
Report Upstream:  N/A  Work issues:  
Branch:  8300f59 (Commits, GitHub, GitLab)  Commit:  8300f591ae2987e01d609e856d38758b28c6bf42 
Dependencies:  #17800  Stopgaps: 
Description
Change History (14)
comment:1 Changed 6 years ago by
 Branch set to u/jdemeyer/ticket/17862
comment:2 Changed 6 years ago by
 Commit set to 7083f19ecfc83426442aa98c78d50c520337e82e
 Status changed from new to needs_review
comment:3 Changed 6 years ago by
Could we get rid in the same ticket of _native_coercion_ranks_inv
and _native_coercion_ranks
and use coerce
from Python
>>> coerce(True, 1l) (1L, 1L) >>> coerce(complex(0,1), 1.0) (1j, (1+0j))
Or even better PyNumber_Coerce from the C API.
comment:4 followup: ↓ 5 Changed 6 years ago by
Can you make a new ticket for that instead?
comment:5 in reply to: ↑ 4 Changed 6 years ago by
comment:6 followup: ↓ 8 Changed 6 years ago by
I just have two remarks but not for this ticket I guess:
 Don't we want to use faster functions in
py_scalar_to_element
?
 It is clear that we want compatibility between
structure.coerce.py_scalar_parent
andstructure.element.py_scalar_to_element
. I do not like the fact that these two functions belong to two different files and do not call each other in their doctests.
comment:7 Changed 6 years ago by
 Reviewers set to Vincent Delecroix
comment:8 in reply to: ↑ 6 ; followup: ↓ 9 Changed 6 years ago by
Replying to vdelecroix:
I just have two remarks but not for this ticket I guess:
 Don't we want to use faster functions in
py_scalar_to_element
?
If you mean functions like PyFoo_Check()
: Cython knows them too, it will generate calls to PyFoo_Check()
in isinstance
.
 It is clear that we want compatibility between
structure.coerce.py_scalar_parent
andstructure.element.py_scalar_to_element
. I do not like the fact that these two functions belong to two different files and do not call each other in their doctests.
Good point.
comment:9 in reply to: ↑ 8 Changed 6 years ago by
Replying to jdemeyer:
Replying to vdelecroix:
I just have two remarks but not for this ticket I guess:
 Don't we want to use faster functions in
py_scalar_to_element
?If you mean functions like
PyFoo_Check()
: Cython knows them too, it will generate calls toPyFoo_Check()
inisinstance
.
Nope. I meant the conversion Python int
> Sage integer
and such. The import
takes quite a long time and ZZ(x)
is suboptimal to create an integer.
comment:10 Changed 6 years ago by
 Commit changed from 7083f19ecfc83426442aa98c78d50c520337e82e to 7eb0e93b6c74b1e64d11a7ea1ac7fd1cb4c56aa6
Branch pushed to git repo; I updated commit sha1. New commits:
7eb0e93  Move py_scalar_to_element to coerce.pyx

comment:11 Changed 6 years ago by
I moved py_scalar_to_element
to coerce.pyx
, but I couldn't improve the conversion because of circular import problems.
comment:12 Changed 6 years ago by
 Branch changed from u/jdemeyer/ticket/17862 to u/vdelecroix/17862
 Commit changed from 7eb0e93b6c74b1e64d11a7ea1ac7fd1cb4c56aa6 to 8300f591ae2987e01d609e856d38758b28c6bf42
I did not know
sage: isinstance(True,int) True
Do we really want the bool > Integer
to happen within Sage? We can do an exact type check instead. That would break support for subtyped Python base classes though. Another drawback is that it would not be possible anymore to count like
sage: sum((x >= 3 for x in my_list), ZZ(0))
So I am in favor of your solution.
For the speedup, that is not dramatic.
I changed the doc to make the link :class:`Element`
works. I am ok for positive review.
New commits:
8300f59  trac #17862 (review): doc typo

comment:13 Changed 6 years ago by
 Status changed from needs_review to positive_review
comment:14 Changed 6 years ago by
 Branch changed from u/vdelecroix/17862 to 8300f591ae2987e01d609e856d38758b28c6bf42
 Resolution set to fixed
 Status changed from positive_review to closed
Last 10 new commits:
Another polynomial division action test.
trac #17740: review 1 (documentation)
trac #17740: review 2 (clean Errors)
trac #17740: review 3 (less in try/except block)
Reintroduce action of fraction field as fallback for division action.
Better _pseudo_fraction_field default implementation.
trac #17740: merge sage6.6.beta1
trac #17740: avoid parent deaths
Merge commit '2075e2e' into t/17800/ticket/17800
Remove use of PY_IS_NUMERIC