Opened 7 years ago
Closed 7 years ago
#18076 closed enhancement (fixed)
Coercion for numpy types
Reported by: | vdelecroix | Owned by: | vdelecroix |
---|---|---|---|
Priority: | major | Milestone: | sage-6.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, GitHub, GitLab) | 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 7 years ago by
- Owner changed from (none) to vdelecroix
comment:2 Changed 7 years ago by
- Branch set to u/vdelecroix/18076
- Commit set to f739d53004dd326ef6412dc4f0ec283e59970586
- Description modified (diff)
comment:3 Changed 7 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 7 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 7 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 7 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 7 years ago by
- Status changed from new to needs_review
comment:8 Changed 7 years ago by
- Description modified (diff)
comment:9 follow-up: ↓ 11 Changed 7 years ago by
- Status changed from needs_review to needs_work
3 failing doctests, see patchbot report
comment:10 Changed 7 years ago by
- Commit changed from 1d2148e8a0eb2dbfd3575f209ced87626e84c6b1 to dbe635f3401ae345516330c0b1d9f570352f31db
comment:11 in reply to: ↑ 9 Changed 7 years ago by
comment:12 Changed 7 years ago by
- Status changed from needs_work to needs_review
comment:13 follow-up: ↓ 15 Changed 7 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 7 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 7 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 7 years ago by
There is a incomplete trac number XYZ:
+ This used to fail (see :trac:`XYZ`)::
comment:17 Changed 7 years ago by
My intention is to make this depend on #18121.
comment:18 Changed 7 years ago by
- Dependencies set to #18121
- Reviewers set to Jeroen Demeyer
comment:19 Changed 7 years ago by
- Branch changed from u/vdelecroix/18076 to u/jdemeyer/18076
comment:20 follow-up: ↓ 21 Changed 7 years ago by
- Commit changed from 6b51a65164956dbd398e8af21b843f39931cadd5 to c3e467149631917bb5ea7d753ef5a18333dd29ac
- Status changed from needs_review to needs_work
comment:21 in reply to: ↑ 20 ; follow-up: ↓ 23 Changed 7 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 7 years ago by
see also #8824
comment:23 in reply to: ↑ 21 Changed 7 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 7 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 7 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 7 years ago by
- Component changed from interfaces to coercion
- Summary changed from Be nicer with numpy to Coercion for numpy types
comment:27 Changed 7 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 7 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 7 years ago by
- Status changed from needs_work to needs_review
comment:30 Changed 7 years ago by
- Milestone changed from sage-6.6 to sage-6.7
- Status changed from needs_review to needs_work
Various conflicts, rebasing...
comment:31 Changed 7 years ago by
- Branch changed from u/vdelecroix/18076 to u/jdemeyer/18076
comment:32 Changed 7 years ago by
- Commit changed from 8af0216f6b910e29632b5c0b1fc105176d5f5df2 to d4c53ac38372916a2e81100e2c4dd360b5afcf51
- Status changed from needs_work to needs_review
comment:33 Changed 7 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 7 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 7 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 7 years ago by
- Status changed from needs_review to needs_work
comment:37 follow-up: ↓ 38 Changed 7 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 ; follow-up: ↓ 40 Changed 7 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 7 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 7 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 search-and-replace a bit too naive, this is a syntax error: numpy.complex128(-2+.I)
comment:41 Changed 7 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 7 years ago by
- Status changed from needs_work to needs_review
comment:43 Changed 7 years ago by
- Status changed from needs_review to positive_review
comment:44 Changed 7 years ago by
- Status changed from positive_review to needs_work
On 32-bit:
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 7 years ago by
- Branch changed from u/vdelecroix/18076 to u/jdemeyer/18076
comment:46 Changed 7 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 7 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 7 years ago by
- Status changed from needs_review to positive_review
Let's assume that it's right...
comment:49 Changed 7 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