Opened 7 years ago

Closed 7 years ago

# Coercion for numpy types

Reported by: Owned by: vdelecroix vdelecroix major sage-6.7 coercion sd66 tmonteil Vincent Delecroix, Jeroen Demeyer Jeroen Demeyer, Vincent Delecroix N/A 8b0cf39 8b0cf3903f70460883cd4b8ba4ba90b0c30f7d8f #18121

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

### comment:1 Changed 7 years ago by vdelecroix

• Owner changed from (none) to vdelecroix

### comment:2 Changed 7 years ago by vdelecroix

• Branch set to u/vdelecroix/18076
• Commit set to f739d53004dd326ef6412dc4f0ec283e59970586
• Description modified (diff)

New commits:

 ​f739d53 `Trac 18076: be nicer with numpy`

### comment:3 Changed 7 years ago by vdelecroix

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 git

• 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 git

• 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 git

• 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 vdelecroix

• Status changed from new to needs_review

### comment:8 Changed 7 years ago by vdelecroix

• Description modified (diff)

### comment:9 follow-up: ↓ 11 Changed 7 years ago by chapoton

• Status changed from needs_review to needs_work

3 failing doctests, see patchbot report

### comment:10 Changed 7 years ago by git

• Commit changed from 1d2148e8a0eb2dbfd3575f209ced87626e84c6b1 to dbe635f3401ae345516330c0b1d9f570352f31db

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

 ​f187d02 `Trac 18076: be nicer with numpy` ​fd5601e `Trac 18076: fix coercions of numpy arrays` ​dbe635f `Trac 18076: fix some doctests`

### comment:11 in reply to: ↑ 9 Changed 7 years ago by vdelecroix

3 failing doctests, see patchbot report

Is that all?

### comment:12 Changed 7 years ago by vdelecroix

• Status changed from needs_work to needs_review

### comment:13 follow-up: ↓ 15 Changed 7 years ago by chapoton

• 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 git

• 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 vdelecroix

• Status changed from needs_work to needs_review

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 slabbe

There is a incomplete trac number XYZ:

```+ This used to fail (see :trac:`XYZ`)::
```

### comment:17 Changed 7 years ago by jdemeyer

My intention is to make this depend on #18121.

### comment:18 Changed 7 years ago by jdemeyer

• Authors changed from Vincent Delecroix to Vincent Delecroix, Jeroen Demeyer
• Dependencies set to #18121
• Reviewers set to Jeroen Demeyer

### comment:19 Changed 7 years ago by jdemeyer

• Branch changed from u/vdelecroix/18076 to u/jdemeyer/18076

### comment:20 follow-up: ↓ 21 Changed 7 years ago by jdemeyer

• Commit changed from 6b51a65164956dbd398e8af21b843f39931cadd5 to c3e467149631917bb5ea7d753ef5a18333dd29ac
• Status changed from needs_review to needs_work

This looks wrong to me:

```sage: numpy.int8('12') + 1/3
12.333333333333334
```

Why is the answer a float???

New commits:

 ​5532969 `Use PyTypeObject from Cython` ​fd2531d `Merge commit '5532969cc7c462d24788585030ab662cf05b024b' into t/18076/18076` ​c3e4671 `Introduce new function "is_numpy_type"`

### comment:21 in reply to: ↑ 20 ; follow-up: ↓ 23 Changed 7 years ago by vdelecroix

This looks wrong to me:

```sage: numpy.int8('12') + 1/3
12.333333333333334
```

Why 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:23 in reply to: ↑ 21 Changed 7 years ago by jdemeyer

This looks wrong to me:

```sage: numpy.int8('12') + 1/3
12.333333333333334
```

Why 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 jdemeyer

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 vdelecroix

• 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 jdemeyer

• 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 git

• 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 jdemeyer

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 chapoton

• Status changed from needs_work to needs_review

### comment:30 Changed 7 years ago by jdemeyer

• 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 jdemeyer

• Branch changed from u/vdelecroix/18076 to u/jdemeyer/18076

### comment:32 Changed 7 years ago by jdemeyer

• Commit changed from 8af0216f6b910e29632b5c0b1fc105176d5f5df2 to d4c53ac38372916a2e81100e2c4dd360b5afcf51
• Status changed from needs_work to needs_review

New commits:

 ​a10dcba `Use PyTypeObject from Cython` ​77e38d2 `Define 3-argument version Py_VISIT3 of Py_VISIT` ​58c17fb `cimport PyWeakref_GetObject() from cpython.weakref` ​d4c53ac `Coercion for numpy types`

### comment:33 Changed 7 years ago by vdelecroix

• 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 jdemeyer

• 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 jdemeyer

My comments (all easy to fix):

1. Conversion `numpy.floating` -> `QQ` should use `RR` directly, instead of `numpy.floating` -> `RDF` which then converts to `RR`.
1. The reference to #15965 is wrong.
1. In `src/sage/rings/real_double.pyx`, I would prefer to split up `if 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 for `CDF` (in particular, if the precision is too small, we can return `None` immediately).
1. Python considers `bool` a subclass of `int`, so
```issubclass(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)
```
1. In doctests, please replace `1jr` by the more natural `I` (unless you have a good reason to not do this)
1. In `NumpyToSRMorphism.__init__`, could you raise a `TypeError` if `numpy_type` doesn't satisfy any of the `issubclass()` checks (and add a doctest for this case). Also, the second `if` should better be an `elif`.
Last edited 7 years ago by jdemeyer (previous) (diff)

### comment:36 Changed 7 years ago by jdemeyer

• Status changed from needs_review to needs_work

### comment:37 follow-up: ↓ 38 Changed 7 years ago by 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. 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 vdelecroix

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 vdelecroix

• 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 jdemeyer

• Status changed from needs_review to needs_work

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 git

• 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 vdelecroix

• Status changed from needs_work to needs_review

### comment:43 Changed 7 years ago by jdemeyer

• Status changed from needs_review to positive_review

### comment:44 Changed 7 years ago by vbraun

• 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 of  22 in sage.structure.coerce.CoercionModel_cache_maps
[272 tests, 1 failure, 0.41 s]
```

### comment:45 Changed 7 years ago by jdemeyer

• Branch changed from u/vdelecroix/18076 to u/jdemeyer/18076

### comment:46 Changed 7 years ago by jdemeyer

• 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 vdelecroix

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 vdelecroix

• Status changed from needs_review to positive_review

Let's assume that it's right...

### comment:49 Changed 7 years ago by vbraun

• Branch changed from u/jdemeyer/18076 to 8b0cf3903f70460883cd4b8ba4ba90b0c30f7d8f
• Resolution set to fixed
• Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.