Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
mkoeppe opened this issue Sep 4, 2021 · 30 comments

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Sep 4, 2021

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

@mkoeppe mkoeppe added this to the sage-9.5 milestone Sep 4, 2021
@mkoeppe

This comment has been minimized.

@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 12, 2021

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 12, 2021

comment:4

setting to needs_review for the patchbot.

Still needs work to hook it into the constructor etc.


New commits:

d66b7bcsrc/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'
d33d173sage.matrix: Factor Matrix_double_dense through Matrix_numpy_dense, add Matrix_numpy_integer_dense

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 12, 2021

Commit: d33d173

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 12, 2021

Author: Matthias Koeppe

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 12, 2021

Changed commit from d33d173 to 98ced3c

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 12, 2021

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

98ced3csrc/sage/matrix/matrix_numpy_integer_dense.pyx: Fix doctest

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 12, 2021

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

cde4e54src/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'
2ffb118Vector_double_dense: Factor through new class Vector_numpy_dense

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 12, 2021

Changed commit from 98ced3c to 2ffb118

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 12, 2021

Changed commit from 2ffb118 to 5b288c3

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 12, 2021

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

5b288c3sage.modules.vector_numpy_integer_dense: New

@mkoeppe mkoeppe changed the title Refactor Matrix_double_dense through Matrix_numpy_dense Refactor {Matrix,Vector}_double_dense through ..._numpy_dense, add ..._numpy_integer_dense Oct 12, 2021
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 16, 2021

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

c8eec7eMerge tag '9.5.beta6' into t/32465/refactor_matrix_double_dense_through_matrix_numpy_dense
5323b25src/sage/matrix/matrix_numpy_dense.pyx: Returns -> Return

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Nov 16, 2021

Changed commit from 5b288c3 to 5323b25

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Dec 18, 2021

comment:10

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

@mkoeppe mkoeppe modified the milestones: sage-9.5, sage-9.6 Dec 18, 2021
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 3, 2022

Changed commit from 5323b25 to d07cf93

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 3, 2022

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

d07cf93Merge tag '9.5' into t/32465/refactor_matrix_double_dense_through_matrix_numpy_dense

@tscrim
Copy link
Collaborator

tscrim commented Feb 16, 2022

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.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 16, 2022

comment:14

Replying to @tscrim:

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

Yes

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 16, 2022

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

@tscrim
Copy link
Collaborator

tscrim commented Feb 16, 2022

Reviewer: Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Feb 16, 2022

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 16, 2022

Changed commit from d07cf93 to 21ed44a

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 16, 2022

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

93f3925src/sage/matrix/matrix_numpy_dense.pyx: Fix doc markup, whitespace
21ed44aMerge tag '9.6.beta1' into t/32465/refactor_matrix_double_dense_through_matrix_numpy_dense

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 16, 2022

comment:18

Thank you!

@vbraun
Copy link
Member

vbraun commented Feb 20, 2022

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]

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 20, 2022

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

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 20, 2022

Changed commit from 21ed44a to b0a1a04

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 20, 2022

comment:22

Nice catch, fixed now

@vbraun
Copy link
Member

vbraun commented Feb 21, 2022

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants