Opened 3 years ago

Closed 2 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) Commit: 8b0cf3903f70460883cd4b8ba4ba90b0c30f7d8f
Dependencies: #18121 Stopgaps:

Description (last modified by vdelecroix)

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 3 years ago by vdelecroix

  • Owner changed from (none) to vdelecroix

comment:2 Changed 3 years ago by vdelecroix

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

New commits:

f739d53Trac 18076: be nicer with numpy

comment:3 Changed 3 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 3 years ago by git

  • Commit changed from f739d53004dd326ef6412dc4f0ec283e59970586 to 1a76757d67024985b529cc54a7eb399305c15126

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

1a76757Trac 18076: fix coercions of numpy arrays

comment:5 Changed 3 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:

b0c8375Trac 18076: fix coercions of numpy arrays

comment:6 Changed 3 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:

1d2148eTrac 18076: fix coercions of numpy arrays

comment:7 Changed 3 years ago by vdelecroix

  • Status changed from new to needs_review

comment:8 Changed 3 years ago by vdelecroix

  • Description modified (diff)

comment:9 follow-up: Changed 3 years ago by chapoton

  • Status changed from needs_review to needs_work

3 failing doctests, see patchbot report

comment:10 Changed 3 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:

f187d02Trac 18076: be nicer with numpy
fd5601eTrac 18076: fix coercions of numpy arrays
dbe635fTrac 18076: fix some doctests

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

Replying to chapoton:

3 failing doctests, see patchbot report

Is that all?

comment:12 Changed 3 years ago by vdelecroix

  • Status changed from needs_work to needs_review

comment:13 follow-up: Changed 3 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 3 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:

6b51a65Trac 18076: fix some doctests

comment:15 in reply to: ↑ 13 Changed 3 years ago by vdelecroix

  • 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 3 years ago by slabbe

There is a incomplete trac number XYZ:

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

comment:17 Changed 3 years ago by jdemeyer

My intention is to make this depend on #18121.

comment:18 Changed 3 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 3 years ago by jdemeyer

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

comment:20 follow-up: Changed 3 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:

5532969Use PyTypeObject from Cython
fd2531dMerge commit '5532969cc7c462d24788585030ab662cf05b024b' into t/18076/18076
c3e4671Introduce new function "is_numpy_type"

comment:21 in reply to: ↑ 20 ; follow-up: Changed 3 years ago by vdelecroix

Replying to 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. 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 3 years ago by vdelecroix

see also #8824

comment:23 in reply to: ↑ 21 Changed 3 years ago by jdemeyer

Replying to vdelecroix:

Replying to 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 3 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 3 years ago by vdelecroix

  • Branch changed from u/jdemeyer/18076 to u/vdelecroix/18076
  • Commit changed from c3e467149631917bb5ea7d753ef5a18333dd29ac to 9c7a6f517c54dfd70d36dd29828e0efd30fcb9a4

New commits:

9c7a6f5Trac 18076: precision about numpy and Sage coercion

comment:26 Changed 3 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 3 years ago by git

  • Commit changed from 9c7a6f517c54dfd70d36dd29828e0efd30fcb9a4 to 8af0216f6b910e29632b5c0b1fc105176d5f5df2

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

8af0216Trac 18076: document all corrected bugs

comment:28 Changed 3 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 3 years ago by chapoton

  • Status changed from needs_work to needs_review

comment:30 Changed 2 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 2 years ago by jdemeyer

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

comment:32 Changed 2 years ago by jdemeyer

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

New commits:

a10dcbaUse PyTypeObject from Cython
77e38d2Define 3-argument version Py_VISIT3 of Py_VISIT
58c17fbcimport PyWeakref_GetObject() from cpython.weakref
d4c53acCoercion for numpy types

comment:33 Changed 2 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 2 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 2 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 2 years ago by jdemeyer (previous) (diff)

comment:36 Changed 2 years ago by jdemeyer

  • Status changed from needs_review to needs_work

comment:37 follow-up: Changed 2 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: Changed 2 years ago by vdelecroix

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 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:

dc77040Trac 18076: review commit

comment:40 in reply to: ↑ 38 Changed 2 years ago by jdemeyer

  • 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 2 years ago by git

  • Commit changed from dc770406c96a27d7925a149f01bdc4e88123392b to ccaf80c11d5ad4cc7207f7d03d42a947a72aa9c7

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

ccaf80cTrac 18076: more fixes in structure.coerce

comment:42 Changed 2 years ago by vdelecroix

  • Status changed from needs_work to needs_review

comment:43 Changed 2 years ago by jdemeyer

  • Status changed from needs_review to positive_review

comment:44 Changed 2 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 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 jdemeyer

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

comment:46 Changed 2 years ago by jdemeyer

  • Commit changed from ccaf80c11d5ad4cc7207f7d03d42a947a72aa9c7 to 8b0cf3903f70460883cd4b8ba4ba90b0c30f7d8f
  • Status changed from needs_work to needs_review

New commits:

8b0cf39Fix doctest different on 32 vs. 64 bits

comment:47 Changed 2 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 2 years ago by vdelecroix

  • Status changed from needs_review to positive_review

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

comment:49 Changed 2 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.