Opened 2 years ago
Closed 2 years ago
#18076 closed enhancement (fixed)
Coercion for numpy types
Reported by:  vdelecroix  Owned by:  vdelecroix 

Priority:  major  Milestone:  sage6.7 
Component:  coercion  Keywords:  sd66 
Cc:  tmonteil  Merged in:  
Authors:  Vincent Delecroix, Jeroen Demeyer  Reviewers:  Jeroen Demeyer, Vincent Delecroix 
Report Upstream:  N/A  Work issues:  
Branch:  8b0cf39 (Commits)  Commit:  8b0cf3903f70460883cd4b8ba4ba90b0c30f7d8f 
Dependencies:  #18121  Stopgaps: 
Description (last modified by )
Plenty of bugs have been reported for the interaction with numpy:
 #8426: polynomial * constant does not work if constant is a numpy type
 #8949: symbolic functions dont work with numpy.int32
 #9769: symbolic function do not work with numpy.int64 arguments
 #13386: comparison of Sage integer with Numpy integer
 #15695: Coercion problems between numpy and sage floats
 #17758: Intervals and numpy floats do not compare correctly
 #17865: get rid of _native_coercion_ranks_inv and _native_coercion_ranks
We solve them all by defining coercions of numpy integers to ZZ
, numpy floating to RDF
and numpy complex floating to CDF
. Also, coercion between numerical types (in sage.structure.coerce
) are now done directly via the addition. In particular, all of this used to fail
sage: import numpy sage: f(t) = t^2 sage: f(numpy.int32('1')) 1 sage: sin(numpy.int32(10)) sin(10) sage: 123 == numpy.int32(123) True sage: 1j + numpy.float(2) 2.00000000000000 + 1.00000000000000*I sage: parent(_) Complex Field with 53 bits of precision sage: RIF(1) <= numpy.float64(2.0) True
Some of them depend on modifying some internal in numpy and will not be solved here:
 #8824: Make it so that numpy datatypes are integrated into the coercion model
Change History (49)
comment:1 Changed 2 years ago by
 Owner changed from (none) to vdelecroix
comment:2 Changed 2 years ago by
 Branch set to u/vdelecroix/18076
 Commit set to f739d53004dd326ef6412dc4f0ec283e59970586
 Description modified (diff)
comment:3 Changed 2 years ago by
Not so bad... got three problematic files
sage t long src/sage/plot/plot.py # 2 doctests failed sage t long src/sage/plot/graphics.py # 1 doctest failed sage t long src/doc/en/thematic_tutorials/numerical_sage/numpy.rst # 1 doctest failed
comment:4 Changed 2 years ago by
 Commit changed from f739d53004dd326ef6412dc4f0ec283e59970586 to 1a76757d67024985b529cc54a7eb399305c15126
Branch pushed to git repo; I updated commit sha1. New commits:
1a76757  Trac 18076: fix coercions of numpy arrays

comment:5 Changed 2 years ago by
 Commit changed from 1a76757d67024985b529cc54a7eb399305c15126 to b0c83753b4eee2f5ba3c9558acae517d8508919c
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
b0c8375  Trac 18076: fix coercions of numpy arrays

comment:6 Changed 2 years ago by
 Commit changed from b0c83753b4eee2f5ba3c9558acae517d8508919c to 1d2148e8a0eb2dbfd3575f209ced87626e84c6b1
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
1d2148e  Trac 18076: fix coercions of numpy arrays

comment:7 Changed 2 years ago by
 Status changed from new to needs_review
comment:8 Changed 2 years ago by
 Description modified (diff)
comment:9 followup: ↓ 11 Changed 2 years ago by
 Status changed from needs_review to needs_work
3 failing doctests, see patchbot report
comment:10 Changed 2 years ago by
 Commit changed from 1d2148e8a0eb2dbfd3575f209ced87626e84c6b1 to dbe635f3401ae345516330c0b1d9f570352f31db
comment:11 in reply to: ↑ 9 Changed 2 years ago by
comment:12 Changed 2 years ago by
 Status changed from needs_work to needs_review
comment:13 followup: ↓ 15 Changed 2 years ago by
 Status changed from needs_review to needs_work
still one failing doctest, see patchbot report.
Désolé de ne pas faire de commentaires plus constructifs, vraiment.
comment:14 Changed 2 years ago by
 Commit changed from dbe635f3401ae345516330c0b1d9f570352f31db to 6b51a65164956dbd398e8af21b843f39931cadd5
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
6b51a65  Trac 18076: fix some doctests

comment:15 in reply to: ↑ 13 Changed 2 years ago by
 Status changed from needs_work to needs_review
Replying to chapoton:
still one failing doctest, see patchbot report.
Désolé de ne pas faire de commentaires plus constructifs, vraiment.
Done!
comment:16 Changed 2 years ago by
There is a incomplete trac number XYZ:
+ This used to fail (see :trac:`XYZ`)::
comment:17 Changed 2 years ago by
My intention is to make this depend on #18121.
comment:18 Changed 2 years ago by
 Dependencies set to #18121
 Reviewers set to Jeroen Demeyer
comment:19 Changed 2 years ago by
 Branch changed from u/vdelecroix/18076 to u/jdemeyer/18076
comment:20 followup: ↓ 21 Changed 2 years ago by
 Commit changed from 6b51a65164956dbd398e8af21b843f39931cadd5 to c3e467149631917bb5ea7d753ef5a18333dd29ac
 Status changed from needs_review to needs_work
comment:21 in reply to: ↑ 20 ; followup: ↓ 23 Changed 2 years ago by
Replying to jdemeyer:
This looks wrong to me:
sage: numpy.int8('12') + 1/3 12.333333333333334Why is the answer a float???
This is because the addition is handled by numpy and not by the rational. Simlarly
sage: import numpy sage: numpy.int64('1') + 1 2 sage: parent(_) <type 'numpy.int64'>
There is a comment somewhere that this can be an upstream request.
comment:22 Changed 2 years ago by
see also #8824
comment:23 in reply to: ↑ 21 Changed 2 years ago by
Replying to vdelecroix:
Replying to jdemeyer:
This looks wrong to me:
sage: numpy.int8('12') + 1/3 12.333333333333334Why is the answer a float???
This is because the addition is handled by numpy and not by the rational.
I see. It doesn't use the Sage coercion framework at all.
Could you clarify this in the docstring when you write this example?
comment:24 Changed 2 years ago by
Could you please add doctests for all the tickets you're marking as duplicate of this one? For example, there is no doctest for #8949.
comment:25 Changed 2 years ago by
 Branch changed from u/jdemeyer/18076 to u/vdelecroix/18076
 Commit changed from c3e467149631917bb5ea7d753ef5a18333dd29ac to 9c7a6f517c54dfd70d36dd29828e0efd30fcb9a4
New commits:
9c7a6f5  Trac 18076: precision about numpy and Sage coercion

comment:26 Changed 2 years ago by
 Component changed from interfaces to coercion
 Summary changed from Be nicer with numpy to Coercion for numpy types
comment:27 Changed 2 years ago by
 Commit changed from 9c7a6f517c54dfd70d36dd29828e0efd30fcb9a4 to 8af0216f6b910e29632b5c0b1fc105176d5f5df2
Branch pushed to git repo; I updated commit sha1. New commits:
8af0216  Trac 18076: document all corrected bugs

comment:28 Changed 2 years ago by
Feel free to set this ticket back to needs_review (although somebody needs to review #18121 before this ticket can be formally reviewed).
comment:29 Changed 2 years ago by
 Status changed from needs_work to needs_review
comment:30 Changed 2 years ago by
 Milestone changed from sage6.6 to sage6.7
 Status changed from needs_review to needs_work
Various conflicts, rebasing...
comment:31 Changed 2 years ago by
 Branch changed from u/vdelecroix/18076 to u/jdemeyer/18076
comment:32 Changed 2 years ago by
 Commit changed from 8af0216f6b910e29632b5c0b1fc105176d5f5df2 to d4c53ac38372916a2e81100e2c4dd360b5afcf51
 Status changed from needs_work to needs_review
comment:33 Changed 2 years ago by
 Reviewers changed from Jeroen Demeyer to Jeroen Demeyer, Vincent Delecroix
 Status changed from needs_review to positive_review
This is fine for me. And all test pass.
Vincent
comment:34 Changed 2 years ago by
 Status changed from positive_review to needs_review
Not so fast, I haven't actually reviewed all your changes.
comment:35 Changed 2 years ago by
My comments (all easy to fix):
 Conversion
numpy.floating
>QQ
should useRR
directly, instead ofnumpy.floating
>RDF
which then converts toRR
.
 The reference to #15965 is wrong.
 In
src/sage/rings/real_double.pyx
, I would prefer to split upif S is ZZ or S is QQ or S is RLF or (isinstance(S, RealField_class) and S.prec() >= 53)
in several cases, just like forCDF
(in particular, if the precision is too small, we can returnNone
immediately).
 Python considers
bool
a subclass ofint
, soissubclass(py_type, int) or issubclass(py_type, long) or issubclass(py_type, bool)
can be replaced by
issubclass(py_type, int) or issubclass(py_type, long)
 In doctests, please replace
1jr
by the more naturalI
(unless you have a good reason to not do this)
 In
NumpyToSRMorphism.__init__
, could you raise aTypeError
ifnumpy_type
doesn't satisfy any of theissubclass()
checks (and add a doctest for this case). Also, the secondif
should better be anelif
.
comment:36 Changed 2 years ago by
 Status changed from needs_review to needs_work
comment:37 followup: ↓ 38 Changed 2 years ago by
I'm also wondering if this check is really needed:
if ty != type(y + x): raise RuntimeError("x + y and y + x have different types...")
First of all, I cannot find any case where this happens. Second, even if it does happen, would it really be a problem?
comment:38 in reply to: ↑ 37 ; followup: ↓ 40 Changed 2 years ago by
Replying to jdemeyer:
I'm also wondering if this check is really needed:
if ty != type(y + x): raise RuntimeError("x + y and y + x have different types...")First of all, I cannot find any case where this happens.
sage: numpy.int16(1) + 1.0 2.0 sage: type(_) <type 'numpy.float64'> sage: 1.0 + numpy.int16(1) 2.00000000000000 sage: type(_) <type 'sage.rings.real_mpfr.RealNumber'>
But the canonical coercion is fine since we do have a coercion from numpy types to real numbers.
Second, even if it does happen, would it really be a problem?
I don't know. You would prefer that we get rid of it?
Vincent
comment:39 Changed 2 years ago by
 Branch changed from u/jdemeyer/18076 to u/vdelecroix/18076
 Commit changed from d4c53ac38372916a2e81100e2c4dd360b5afcf51 to dc770406c96a27d7925a149f01bdc4e88123392b
 Status changed from needs_work to needs_review
New commits:
dc77040  Trac 18076: review commit

comment:40 in reply to: ↑ 38 Changed 2 years ago by
 Status changed from needs_review to needs_work
Replying to vdelecroix:
I don't know. You would prefer that we get rid of it?
Given that I see no reason to have the check, I'd rather remove it.
I think you did your searchandreplace a bit too naive, this is a syntax error: numpy.complex128(2+.I)
comment:41 Changed 2 years ago by
 Commit changed from dc770406c96a27d7925a149f01bdc4e88123392b to ccaf80c11d5ad4cc7207f7d03d42a947a72aa9c7
Branch pushed to git repo; I updated commit sha1. New commits:
ccaf80c  Trac 18076: more fixes in structure.coerce

comment:42 Changed 2 years ago by
 Status changed from needs_work to needs_review
comment:43 Changed 2 years ago by
 Status changed from needs_review to positive_review
comment:44 Changed 2 years ago by
 Status changed from positive_review to needs_work
On 32bit:
sage t long src/sage/structure/coerce.pyx ********************************************************************** File "src/sage/structure/coerce.pyx", line 334, in sage.structure.coerce.CoercionModel_cache_maps Failed example: type(_) Expected: <type 'numpy.int64'> Got: <type 'numpy.int32'> ********************************************************************** 1 item had failures: 1 of 22 in sage.structure.coerce.CoercionModel_cache_maps [272 tests, 1 failure, 0.41 s]
comment:45 Changed 2 years ago by
 Branch changed from u/vdelecroix/18076 to u/jdemeyer/18076
comment:46 Changed 2 years ago by
 Commit changed from ccaf80c11d5ad4cc7207f7d03d42a947a72aa9c7 to 8b0cf3903f70460883cd4b8ba4ba90b0c30f7d8f
 Status changed from needs_work to needs_review
New commits:
8b0cf39  Fix doctest different on 32 vs. 64 bits

comment:47 Changed 2 years ago by
It does not affect floating point?
sage: numpy.int8('12') + 1/3 12.333333333333334 sage: type(_) <type 'numpy.float64'>
comment:48 Changed 2 years ago by
 Status changed from needs_review to positive_review
Let's assume that it's right...
comment:49 Changed 2 years ago by
 Branch changed from u/jdemeyer/18076 to 8b0cf3903f70460883cd4b8ba4ba90b0c30f7d8f
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
Trac 18076: be nicer with numpy