Refactor {Matrix,Vector}_double_dense through ..._numpy_dense, add ..._numpy_integer_dense

Question

Refactor {Matrix,Vector}_double_dense through ..._numpy_dense, add ..._numpy_integer_dense

Closed this issue 4 years ago · 30 comments

... in order to define matrix spaces backed by numpy arrays of various types
https://numpy.org/doc/stable/reference/arrays.scalars.html#signed-integer-types

For ZZ, this could for example provide a separate element implementation that is never used for arithmetic, but is good enough for creating matrices in methods such as vertex_adjacency_matrix (#32666)

Related earlier ticket: #7920

CC: @kliem @orlitzky

Component: linear algebra

Author: Matthias Koeppe

Branch/Commit: b0a1a04

Reviewer: Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/32465

Answer 1 · 2021-09-07T07:39:31.000Z

Description changed:

--- 
+++ 
@@ -2,3 +2,4 @@
 https://numpy.org/doc/stable/reference/arrays.scalars.html#signed-integer-types
 
 
+Related earlier ticket: #7920

Answer 2 · 2021-10-11T20:05:06.000Z

Description changed:

--- 
+++ 
@@ -1,5 +1,7 @@
 ... in order to define matrix spaces backed by numpy arrays of various types
 https://numpy.org/doc/stable/reference/arrays.scalars.html#signed-integer-types
 
+For `ZZ`, this could for example provide a separate element implementation that is never used for arithmetic, but is good enough for creating matrices in methods such as `vertex_adjacency_matrix` (#32666)
+
 
 Related earlier ticket: #7920

Answer 3 · 2021-10-12T05:55:17.000Z

Branch: u/mkoeppe/refactor_matrix_double_dense_through_matrix_numpy_dense

Answer 4 · 2021-10-12T05:56:22.000Z

comment:4

setting to needs_review for the patchbot.

Still needs work to hook it into the constructor etc.

New commits:

`d66b7bc`	`src/sage/matrix/matrix_double_dense.pyx: Update copyright according to 'git blame -w --date=format:%Y src/sage/matrix/matrix_double_dense.pyx \| sort -k2'`
`d33d173`	`sage.matrix: Factor Matrix_double_dense through Matrix_numpy_dense, add Matrix_numpy_integer_dense`

Answer 5 · 2021-10-12T05:56:22.000Z

Commit: d33d173

Answer 6 · 2021-10-12T05:56:22.000Z

Author: Matthias Koeppe

Answer 7 · 2021-10-12T19:39:43.000Z

Changed commit from d33d173 to 98ced3c

Answer 8 · 2021-10-12T19:39:43.000Z

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

`98ced3c`	`src/sage/matrix/matrix_numpy_integer_dense.pyx: Fix doctest`

Answer 9 · 2021-10-12T22:23:59.000Z

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

`cde4e54`	`src/sage/modules/vector_double_dense.pyx: Update copyright according to 'git blame -w --date=format:%Y src/sage/modules/vector_double_dense.pyx \| sort -k2'`
`2ffb118`	`Vector_double_dense: Factor through new class Vector_numpy_dense`

Answer 10 · 2021-10-12T22:23:59.000Z

Changed commit from 98ced3c to 2ffb118

Answer 11 · 2021-10-12T22:33:32.000Z

Changed commit from 2ffb118 to 5b288c3

Answer 12 · 2021-10-12T22:33:32.000Z

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

`5b288c3`	`sage.modules.vector_numpy_integer_dense: New`

Answer 13 · 2021-11-16T20:28:37.000Z

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

`c8eec7e`	`Merge tag '9.5.beta6' into t/32465/refactor_matrix_double_dense_through_matrix_numpy_dense`
`5323b25`	`src/sage/matrix/matrix_numpy_dense.pyx: Returns -> Return`

Answer 14 · 2021-11-16T20:28:37.000Z

Changed commit from 5b288c3 to 5323b25

Answer 15 · 2021-12-18T19:53:12.000Z

comment:10

Stalled in needs_review or needs_info; likely won't make it into Sage 9.5.

Answer 16 · 2022-02-03T01:32:43.000Z

Changed commit from 5323b25 to d07cf93

Answer 17 · 2022-02-03T01:32:43.000Z

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

`d07cf93`	`Merge tag '9.5' into t/32465/refactor_matrix_double_dense_through_matrix_numpy_dense`

Answer 18 · 2022-02-16T02:58:32.000Z

comment:13

Do I understand this ticket correctly that it is basically refactoring out methods into base classes?

Why isn't, e.g., _numpy_dtypeint set for Matrix_double_dense? I would think this would cause problems when using this class with numpy.

Answer 19 · 2022-02-16T03:11:38.000Z

comment:14

Replying to @tscrim:

Do I understand this ticket correctly that it is basically refactoring out methods into base classes?

Yes

Answer 20 · 2022-02-16T03:13:33.000Z

comment:15

Replying to @tscrim:

Why isn't, e.g., _numpy_dtypeint set for Matrix_double_dense? I would think this would cause problems when using this class with numpy.

Matrix_double_dense is also abstract. _numpy_dtypeint is set in the subclasses, see src/sage/matrix/matrix_real_double_dense.pyx and src/sage/matrix/matrix_complex_double_dense.pyx

Answer 21 · 2022-02-16T03:29:45.000Z

Reviewer: Travis Scrimshaw

Answer 22 · 2022-02-16T03:29:45.000Z

comment:16

Ah, I see. Thank you. This is essentially a positive review. One little change I saw when reading it over (a while-we-are-at-it):

In matrix_numpy_dense is_symmetric():

-        The tolerance inequality is strict:
+        The tolerance inequality is strict::
+
             sage: m.is_symmetric(tol=0.1)
             False
             sage: m.is_symmetric(tol=0.11)
             True

Once done, you can set a positive review on my behalf.

Answer 23 · 2022-02-16T04:46:39.000Z

Changed commit from d07cf93 to 21ed44a

Answer 24 · 2022-02-16T04:46:39.000Z

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

`93f3925`	`src/sage/matrix/matrix_numpy_dense.pyx: Fix doc markup, whitespace`
`21ed44a`	`Merge tag '9.6.beta1' into t/32465/refactor_matrix_double_dense_through_matrix_numpy_dense`

Answer 25 · 2022-02-16T04:52:23.000Z

comment:18

Thank you!

Answer 26 · 2022-02-20T10:05:54.000Z

comment:19

On 32-bit:

**********************************************************************
File "src/sage/matrix/matrix_numpy_integer_dense.pyx", line 15, in sage.matrix.matrix_numpy_integer_dense
Failed example:
    M.numpy()
Expected:
    array([[ 0,  0,  0],
           [ 0,  0, 47]])
Got:
    array([[ 0,  0,  0],
           [ 0,  0, 47]], dtype=int64)
**********************************************************************
1 item had failures:
   1 of   6 in sage.matrix.matrix_numpy_integer_dense
    [8 tests, 1 failure, 0.01 s]
**********************************************************************
File "src/sage/modules/vector_numpy_integer_dense.pyx", line 12, in sage.modules.vector_numpy_integer_dense
Failed example:
    v.numpy()
Expected:
    array([ 0, 42,  0])
Got:
    array([ 0, 42,  0], dtype=int64)
**********************************************************************
1 item had failures:
   1 of   6 in sage.modules.vector_numpy_integer_dense
    [5 tests, 1 failure, 0.01 s]

Answer 27 · 2022-02-20T17:53:28.000Z

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

`b0a1a04`	`src/sage/{matrix/matrix,modules/vector}_numpy_integer_dense.pyx: Fix doctest output in 32bit`

Answer 28 · 2022-02-20T17:53:28.000Z

Changed commit from 21ed44a to b0a1a04

Answer 29 · 2022-02-20T17:54:06.000Z

comment:22

Nice catch, fixed now

Answer 30 · 2022-02-21T21:56:16.000Z

Changed branch from u/mkoeppe/refactor_matrix_double_dense_through_matrix_numpy_dense to b0a1a04