Provide diagonal_matrix as a method of matrix spaces

[slel]

Sage provides two functions identity_matrix
and diagonal_matrix in the global namespace.

By contrast, matrix spaces have an identity_matrix
method but no diagonal_matrix method.

Global namespace functions:

sage: identity_matrix(ZZ, 2)
[1 0]
[0 1]
sage: diagonal_matrix(ZZ, [3, 2])
[3 0]
[0 2]

Matrix space methods:

sage: M = MatrixSpace(ZZ, 2)
sage: M.identity_matrix()
[1 0]
[0 1]

Before this ticket:

sage: M = MatrixSpace(ZZ, 2)
sage: M.diagonal_matrix([3, 2])
Traceback (most recent call last)
...
AttributeError: 'MatrixSpace_with_category' object has no attribute 'diagonal_matrix'

After this ticket:

sage: M = MatrixSpace(ZZ, 2)
sage: M.diagonal_matrix([3, 2])
[3 0]
[0 2]

Component: linear algebra

Keywords: diagonal matrix

Author: Sagnik Dey

Branch/Commit: f2d8ea9

Reviewer: John Palmieri

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

[embray]

comment:1

Ticket retargeted after milestone closed

[Tinkidinki]

Branch: u/gh-Tinkidinki/provide_diagonal_matrix_as_a_method_of_matrix_spaces

[Tinkidinki]

comment:3

I added a diagonal_matrix method in matrix_space.py pointing it to the original diagonal_matrix method in special.py.

So, I tried to think of reasons why we would be using matrix_space methods instead of the regular matrix() methods (crosspost: https://math.stackexchange.com/q/3537612/287775) but couldn't find any.

Are there practical reasons, so that I can write better code for this file now/ in the future?

Also would like a review on this patch.

---

New commits:

b9d128b	Diagonal matrix function done
2913be1	Documentation for diagonal matrix done
744d8f2	Pointed ms.diagonal_matrix() to the diagonal_matrix() implementation

[Tinkidinki]

Commit: 744d8f2

[jhpalmieri]

comment:5

It would be better to model this on the identity_matrix method. In particular: it should raise an error if the matrix space is for non-square matrices, and there should be doctests. And it should raise an error if entries doesn't have the right length.

[Tinkidinki]

comment:6

Yes, this needs to be fixed for non-square matrices and requires doc-tests. However, ensuring the length of entries is correct is taken care of by the diagonal_matrix() method in special.py.

[SagnikDey92]

Changed branch from u/gh-Tinkidinki/provide_diagonal_matrix_as_a_method_of_matrix_spaces to u/gh-SagnikDey92/provide_diagonal_matrix_as_a_method_of_matrix_spaces

[SagnikDey92]

comment:10

Please review. Currently, this doesn't work:

sage: M1 = MatrixSpace(ZZ, 2, implementation='flint')

sage: M2 = MatrixSpace(ZZ, 2, implementation='generic')

sage: type(M1.diagonal_matrix([1, 2]))

  <type 'sage.matrix.matrix_integer_dense.Matrix_integer_dense'>

sage: type(M2.diagonal_matrix([1, 2]))

  <type 'sage.matrix.matrix_generic_dense.Matrix_generic_dense'>

Both the types are currently:

<class 'sage.matrix.matrix_integer_sparse.Matrix_integer_sparse'>

---

New commits:

bccdee3

Added non square matrix error and some documentation. Made diagonal matrix returned immutable

[SagnikDey92]

Changed commit from 744d8f2 to bccdee3

[jhpalmieri]

comment:11

Matrix spaces can be created using the keyword sparse, with False the default, and you can recover this value using M.is_sparse(). So you should call diagonal_matrix with the argument sparse=self.is_sparse():

diff --git a/src/sage/matrix/matrix_space.py b/src/sage/matrix/matrix_space.py
index b4cb724e3b..3712806364 100644
--- a/src/sage/matrix/matrix_space.py
+++ b/src/sage/matrix/matrix_space.py
@@ -1600,7 +1600,7 @@ class MatrixSpace(UniqueRepresentation, Parent):
         """
         if self.__nrows != self.__ncols:
             raise TypeError("diagonal matrix must be square")
-        A = sage.matrix.special.diagonal_matrix(self.base_ring(), self.dims()[0], entries)
+        A = sage.matrix.special.diagonal_matrix(self.base_ring(), self.dims()[0], entries, sparse=self.is_sparse())
         A.set_immutable()
         return A

Why make it immutable by default?

[sagetrac-git]

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

12eca57

Modeled diagonal_matrix on identity_matrix. Changed the documentation accordingly. No longer uses the function in special.py.

[sagetrac-git]

Changed commit from bccdee3 to 12eca57

[SagnikDey92]

comment:13

Replying to @jhpalmieri:

Matrix spaces can be created using the keyword sparse, with False the default, and you can recover this value using M.is_sparse(). So you should call diagonal_matrix with the argument sparse=self.is_sparse():

diff --git a/src/sage/matrix/matrix_space.py b/src/sage/matrix/matrix_space.py
index b4cb724e3b..3712806364 100644
--- a/src/sage/matrix/matrix_space.py
+++ b/src/sage/matrix/matrix_space.py
@@ -1600,7 +1600,7 @@ class MatrixSpace(UniqueRepresentation, Parent):
         """
         if self.__nrows != self.__ncols:
             raise TypeError("diagonal matrix must be square")
-        A = sage.matrix.special.diagonal_matrix(self.base_ring(), self.dims()[0], entries)
+        A = sage.matrix.special.diagonal_matrix(self.base_ring(), self.dims()[0], entries, sparse=self.is_sparse())
         A.set_immutable()
         return A

Why make it immutable by default?

The above discussion thread suggested to model diagonal_matrix on the identity_matrix function. Since that was immutable by default, I made it so in diagonal_matrix as well. I'll remove it if required. I've now made a change that keeps the implementation method the same in the diagonal matrix. Please review.

[jhpalmieri]

comment:14

The identity_matrix is immutable because it is cached. If it were not immutable, this could happen:

sage: M = MatrixSpace(ZZ, 2)
sage: a = M.identity_matrix()
sage: a[0,0] = 2
sage: M.identity_matrix()
[2 0]
[0 1]

This is not an issue with diagonal_matrix, and I don't see a good reason for it to be immutable.

[SagnikDey92]

comment:15

Ok. Thank you for the clarification. I'm new to this library. I'll remove the immutability part and send the PR again.

[sagetrac-git]

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

f48d915

Removed immutability of diagonal matrix and edited documentation appropriately

[sagetrac-git]

Changed commit from 12eca57 to f48d915

[SagnikDey92]

comment:17

Replying to @sagetrac-git:

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

f48d915

Removed immutability of diagonal matrix and edited documentation appropriately

Please review

[jhpalmieri]

Changed branch from u/gh-SagnikDey92/provide_diagonal_matrix_as_a_method_of_matrix_spaces to u/jhpalmieri/provide_diagonal_matrix_as_a_method_of_matrix_spaces

[jhpalmieri]

Reviewer: John Palmieri

[jhpalmieri]

Changed commit from f48d915 to a214fd2

[jhpalmieri]

comment:19

I made a few changes:

• edited the docstring so that there is an "INPUTS" block and so that the lines are under 80 characters.
• added a doctest showing what happens if you try to create a diagonal matrix with entries from the wrong ring
• removed the alias diag: I think diagonal_matrix is completely clear, whereas diag is ambiguous. People can easily get diagonal_matrix using tab-completion, so I'm not concerned about the extra characters.

If you are happy with these changes, feel free to set to positive review. Make sure to add your real name to the "Authors" field.

---

New commits:

86e5d33	Diagonal matrix function done
68ce495	Documentation for diagonal matrix done
242a5d7	Pointed ms.diagonal_matrix() to the diagonal_matrix() implementation
42a4810	Added non square matrix error and some documentation. Made diagonal matrix returned immutable
cb656f3	Modeled diagonal_matrix on identity_matrix. Changed the documentation accordingly. No longer uses the function in special.py.
29f3805	Removed immutability of diagonal matrix and edited documentation appropriately
a214fd2	trac 28882: reviewer changes

[SagnikDey92]

comment:20

Thank you for the corrections u/jhpalmieri. Setting status to 'positive review'.

[SagnikDey92]

Author: Sagnik Dey

[vbraun]

comment:21

sage -t --long --warn-long 35.4 src/doc/en/thematic_tutorials/coercion_and_categories.rst
**********************************************************************
File "src/doc/en/thematic_tutorials/coercion_and_categories.rst", line 450, in doc.en.thematic_tutorials.coercion_and_categories
Failed example:
    len([s for s in dir(MS1) if inspect.ismethod(getattr(MS1,s,None))])
Expected:
    81
Got:
    82
**********************************************************************
File "src/doc/en/thematic_tutorials/coercion_and_categories.rst", line 452, in doc.en.thematic_tutorials.coercion_and_categories
Failed example:
    len([s for s in dir(MS2) if inspect.ismethod(getattr(MS2,s,None))])
Expected:
    121
Got:
    122
**********************************************************************
1 item had failures:
   2 of 192 in doc.en.thematic_tutorials.coercion_and_categories
    [191 tests, 2 failures, 0.28 s]
----------------------------------------------------------------------
sage -t --long --warn-long 35.4 src/doc/en/thematic_tutorials/coercion_and_categories.rst  # 2 doctests failed
----------------------------------------------------------------------

[SagnikDey92]

comment:22

Replying to @vbraun:

sage -t --long --warn-long 35.4 src/doc/en/thematic_tutorials/coercion_and_categories.rst
**********************************************************************
File "src/doc/en/thematic_tutorials/coercion_and_categories.rst", line 450, in doc.en.thematic_tutorials.coercion_and_categories
Failed example:
    len([s for s in dir(MS1) if inspect.ismethod(getattr(MS1,s,None))])
Expected:
    81
Got:
    82
**********************************************************************
File "src/doc/en/thematic_tutorials/coercion_and_categories.rst", line 452, in doc.en.thematic_tutorials.coercion_and_categories
Failed example:
    len([s for s in dir(MS2) if inspect.ismethod(getattr(MS2,s,None))])
Expected:
    121
Got:
    122
**********************************************************************
1 item had failures:
   2 of 192 in doc.en.thematic_tutorials.coercion_and_categories
    [191 tests, 2 failures, 0.28 s]
----------------------------------------------------------------------
sage -t --long --warn-long 35.4 src/doc/en/thematic_tutorials/coercion_and_categories.rst  # 2 doctests failed
----------------------------------------------------------------------

Hey @vbraun, thanks for the pointer. I'm new to contributing to sage and this makes me realise I should run all the doctests before sending PRs. Just a clarification, should I also run the "long" tests?

Also, regarding this change, clearly, the error is because I added a new function so the number of expected functions should increase by one. I can make the change in the test file and send it if that's what is required. However, I tried rebasing with current develop branch and then running the doc test and this gives expected and got values different:

Failed example:
    len([s for s in dir(MS1) if inspect.ismethod(getattr(MS1,s,None))])
Expected:
    81
Got:
    83
**********************************************************************
File "src/doc/en/thematic_tutorials/coercion_and_categories.rst", line 452, in doc.en.thematic_tutorials.coercion_and_categories
Failed example:
    len([s for s in dir(MS2) if inspect.ismethod(getattr(MS2,s,None))])
Expected:
    120
Got:
    122

Should I change the doctest before or after rebase?

[jhpalmieri]

comment:23

I would propose this change:

diff --git a/src/doc/en/thematic_tutorials/coercion_and_categories.rst b/src/doc/en/thematic_tutorials/coercion_and_categories.rst
index bd76813e9c..b58c9c59de 100644
--- a/src/doc/en/thematic_tutorials/coercion_and_categories.rst
+++ b/src/doc/en/thematic_tutorials/coercion_and_categories.rst
@@ -447,10 +447,10 @@ Sage's category framework can differentiate the two cases::
 And indeed, ``MS2`` has *more* methods than ``MS1``::
 
     sage: import inspect
-    sage: len([s for s in dir(MS1) if inspect.ismethod(getattr(MS1,s,None))])
-    81
-    sage: len([s for s in dir(MS2) if inspect.ismethod(getattr(MS2,s,None))])
-    121
+    sage: L1 = len([s for s in dir(MS1) if inspect.ismethod(getattr(MS1,s,None))])
+    sage: L2 = len([s for s in dir(MS2) if inspect.ismethod(getattr(MS2,s,None))])
+    sage: L1 < L2
+    True
 
 This is because the class of ``MS2`` also inherits from the parent
 class for algebras::

The doctest in question is supposed to demonstrate that there are more methods for square matrices than non-square matrices, but I don't see why the actual numbers should be important. And as we see here, the actual numbers are sensitive to changes in the Sage library; testing L1 < L2 is more robust.

[SagnikDey92]

comment:24

Replying to @jhpalmieri:

I would propose this change:

diff --git a/src/doc/en/thematic_tutorials/coercion_and_categories.rst b/src/doc/en/thematic_tutorials/coercion_and_categories.rst
index bd76813e9c..b58c9c59de 100644
--- a/src/doc/en/thematic_tutorials/coercion_and_categories.rst
+++ b/src/doc/en/thematic_tutorials/coercion_and_categories.rst
@@ -447,10 +447,10 @@ Sage's category framework can differentiate the two cases::
 And indeed, ``MS2`` has *more* methods than ``MS1``::
 
     sage: import inspect
-    sage: len([s for s in dir(MS1) if inspect.ismethod(getattr(MS1,s,None))])
-    81
-    sage: len([s for s in dir(MS2) if inspect.ismethod(getattr(MS2,s,None))])
-    121
+    sage: L1 = len([s for s in dir(MS1) if inspect.ismethod(getattr(MS1,s,None))])
+    sage: L2 = len([s for s in dir(MS2) if inspect.ismethod(getattr(MS2,s,None))])
+    sage: L1 < L2
+    True
 
 This is because the class of ``MS2`` also inherits from the parent
 class for algebras::

The doctest in question is supposed to demonstrate that there are more methods for square matrices than non-square matrices, but I don't see why the actual numbers should be important. And as we see here, the actual numbers are sensitive to changes in the Sage library; testing L1 < L2 is more robust.

Ok thank you, I'll make this change. Also, you recently changed the status of #27636 from positive_review to needs work. Can you please make a note there as to what change is required?

[jhpalmieri]

comment:25

That was vbraun, not me.

[SagnikDey92]

Changed branch from u/jhpalmieri/provide_diagonal_matrix_as_a_method_of_matrix_spaces to u/gh-SagnikDey92/provide_diagonal_matrix_as_a_method_of_matrix_spaces

[SagnikDey92]

comment:27

Replying to @jhpalmieri:

That was vbraun, not me.

I'm so sorry I got mixed up...
I've made the change now. Also, can you confirm if running all the doctests is necessary while sending the PRs? Since it takes some time on my local machine. Clearly only running it in the project folder I made changes to is not enough because this error was completely outside it?

---

New commits:

9bc5ef5	Diagonal matrix function done
c20ffce	Documentation for diagonal matrix done
5463512	Pointed ms.diagonal_matrix() to the diagonal_matrix() implementation
a9308bf	Added non square matrix error and some documentation. Made diagonal matrix returned immutable
4fa5373	Modeled diagonal_matrix on identity_matrix. Changed the documentation accordingly. No longer uses the function in special.py.
091cd1d	Removed immutability of diagonal matrix and edited documentation appropriately
cd62762	Modified a doctest that replaces checking for exact number of methods for Matrix spaces with a comparison test.

[SagnikDey92]

Changed commit from a214fd2 to cd62762

[jhpalmieri]

comment:28

First, the current branch fails to merge.

Second, it is always best if you run all doctests (make ptestlong), but there are bots that run tests as well. You can see their reports by clicking on the circles at the top right of the ticket description box; for this ticket, they take you to the page https://patchbot.sagemath.org/ticket/28882.

[jhpalmieri]

Changed branch from u/gh-SagnikDey92/provide_diagonal_matrix_as_a_method_of_matrix_spaces to u/jhpalmieri/provide_diagonal_matrix_as_a_method_of_matrix_spaces

[jhpalmieri]

comment:30

Here's a branch including that change.

---

New commits:

c279781	Diagonal matrix function done
b132688	Documentation for diagonal matrix done
f56aee3	Pointed ms.diagonal_matrix() to the diagonal_matrix() implementation
54620b2	Added non square matrix error and some documentation. Made diagonal matrix returned immutable
5c2a5b2	Modeled diagonal_matrix on identity_matrix. Changed the documentation accordingly. No longer uses the function in special.py.
14c422a	Removed immutability of diagonal matrix and edited documentation appropriately
758e943	trac 28882: reviewer changes
f2d8ea9	trac 28882: fix doctest in thematic_tutorials

[jhpalmieri]

Changed commit from cd62762 to f2d8ea9

[SagnikDey92]

comment:31

Replying to @jhpalmieri:

First, the current branch fails to merge.

Oh sorry about that. Thank you for correcting it. I'll pull and rebase with current develop on my other branches as well.

Second, it is always best if you run all doctests (make ptestlong), but there are bots that run tests as well. You can see their reports by clicking on the circles at the top right of the ticket description box; for this ticket, they take you to the page https://patchbot.sagemath.org/ticket/28882.

Thank you!

[vbraun]

Changed branch from u/jhpalmieri/provide_diagonal_matrix_as_a_method_of_matrix_spaces to f2d8ea9