Opened 3 years ago
Closed 3 years ago
#28882 closed enhancement (fixed)
Provide diagonal_matrix as a method of matrix spaces
Reported by:  Samuel Lelièvre  Owned by:  

Priority:  major  Milestone:  sage9.1 
Component:  linear algebra  Keywords:  diagonal matrix, beginner 
Cc:  Merged in:  
Authors:  Sagnik Dey  Reviewers:  John Palmieri 
Report Upstream:  N/A  Work issues:  
Branch:  f2d8ea9 (Commits, GitHub, GitLab)  Commit:  f2d8ea9acfe88a25856e2f844f66f84d1ebfcc41 
Dependencies:  Stopgaps: 
Description
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]
Change History (32)
comment:1 Changed 3 years ago by
Milestone:  sage9.0 → sage9.1 

comment:2 Changed 3 years ago by
Branch:  → u/ghTinkidinki/provide_diagonal_matrix_as_a_method_of_matrix_spaces 

comment:3 Changed 3 years ago by
Commit:  → 744d8f2eae5ace821bf674998d74023045bc34f6 

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

comment:4 Changed 3 years ago by
Status:  new → needs_review 

comment:5 Changed 3 years ago by
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 nonsquare matrices, and there should be doctests. And it should raise an error if entries
doesn't have the right length.
comment:6 Changed 3 years ago by
Yes, this needs to be fixed for nonsquare matrices and requires doctests. However, ensuring the length of entries
is correct is taken care of by the diagonal_matrix() method in special.py.
comment:7 Changed 3 years ago by
Keywords:  beginner added 

comment:8 Changed 3 years ago by
Status:  needs_review → needs_work 

comment:9 Changed 3 years ago by
Branch:  u/ghTinkidinki/provide_diagonal_matrix_as_a_method_of_matrix_spaces → u/ghSagnikDey92/provide_diagonal_matrix_as_a_method_of_matrix_spaces 

comment:10 Changed 3 years ago by
Commit:  744d8f2eae5ace821bf674998d74023045bc34f6 → bccdee385da0a05a708890989c504d180fbbe562 

Status:  needs_work → needs_review 
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

comment:11 followup: 13 Changed 3 years ago by
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()
:

src/sage/matrix/matrix_space.py
diff git a/src/sage/matrix/matrix_space.py b/src/sage/matrix/matrix_space.py index b4cb724e3b..3712806364 100644
a b class MatrixSpace(UniqueRepresentation, Parent): 1600 1600 """ 1601 1601 if self.__nrows != self.__ncols: 1602 1602 raise TypeError("diagonal matrix must be square") 1603 A = sage.matrix.special.diagonal_matrix(self.base_ring(), self.dims()[0], entries )1603 A = sage.matrix.special.diagonal_matrix(self.base_ring(), self.dims()[0], entries, sparse=self.is_sparse()) 1604 1604 A.set_immutable() 1605 1605 return A 1606 1606
Why make it immutable by default?
comment:12 Changed 3 years ago by
Commit:  bccdee385da0a05a708890989c504d180fbbe562 → 12eca57a2fc73f795a053d836559b12216074cbe 

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.

comment:13 Changed 3 years ago by
Replying to jhpalmieri:
Matrix spaces can be created using the keyword
sparse
, withFalse
the default, and you can recover this value usingM.is_sparse()
. So you should calldiagonal_matrix
with the argumentsparse=self.is_sparse()
:
src/sage/matrix/matrix_space.py
diff git a/src/sage/matrix/matrix_space.py b/src/sage/matrix/matrix_space.py index b4cb724e3b..3712806364 100644
a b class MatrixSpace(UniqueRepresentation, Parent): 1600 1600 """ 1601 1601 if self.__nrows != self.__ncols: 1602 1602 raise TypeError("diagonal matrix must be square") 1603 A = sage.matrix.special.diagonal_matrix(self.base_ring(), self.dims()[0], entries )1603 A = sage.matrix.special.diagonal_matrix(self.base_ring(), self.dims()[0], entries, sparse=self.is_sparse()) 1604 1604 A.set_immutable() 1605 1605 return A 1606 1606 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.
comment:14 Changed 3 years ago by
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.
comment:15 Changed 3 years ago by
Ok. Thank you for the clarification. I'm new to this library. I'll remove the immutability part and send the PR again.
comment:16 followup: 17 Changed 3 years ago by
Commit:  12eca57a2fc73f795a053d836559b12216074cbe → f48d91551e994a81dbe7e6edd639d35b994608d3 

Branch pushed to git repo; I updated commit sha1. New commits:
f48d915  Removed immutability of diagonal matrix and edited documentation appropriately

comment:17 Changed 3 years ago by
comment:18 Changed 3 years ago by
Branch:  u/ghSagnikDey92/provide_diagonal_matrix_as_a_method_of_matrix_spaces → u/jhpalmieri/provide_diagonal_matrix_as_a_method_of_matrix_spaces 

comment:19 Changed 3 years ago by
Commit:  f48d91551e994a81dbe7e6edd639d35b994608d3 → a214fd2c452bb5e168afb78eb6babf78b1052417 

Reviewers:  → John Palmieri 
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 thinkdiagonal_matrix
is completely clear, whereasdiag
is ambiguous. People can easily getdiagonal_matrix
using tabcompletion, 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

comment:20 Changed 3 years ago by
Authors:  → Sagnik Dey 

Status:  needs_review → positive_review 
Thank you for the corrections u/jhpalmieri. Setting status to 'positive review'.
comment:21 followup: 22 Changed 3 years ago by
Status:  positive_review → needs_work 

sage t long warnlong 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 warnlong 35.4 src/doc/en/thematic_tutorials/coercion_and_categories.rst # 2 doctests failed 
comment:22 Changed 3 years ago by
Replying to vbraun:
sage t long warnlong 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 warnlong 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?
comment:23 followup: 24 Changed 3 years ago by
I would propose this change:

src/doc/en/thematic_tutorials/coercion_and_categories.rst
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 b Sage's category framework can differentiate the two cases:: 447 447 And indeed, ``MS2`` has *more* methods than ``MS1``:: 448 448 449 449 sage: import inspect 450 sage: len([s for s in dir(MS1) if inspect.ismethod(getattr(MS1,s,None))])451 81452 sage: len([s for s in dir(MS2) if inspect.ismethod(getattr(MS2,s,None))])453 121450 sage: L1 = len([s for s in dir(MS1) if inspect.ismethod(getattr(MS1,s,None))]) 451 sage: L2 = len([s for s in dir(MS2) if inspect.ismethod(getattr(MS2,s,None))]) 452 sage: L1 < L2 453 True 454 454 455 455 This is because the class of ``MS2`` also inherits from the parent 456 456 class for algebras::
The doctest in question is supposed to demonstrate that there are more methods for square matrices than nonsquare 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.
comment:24 Changed 3 years ago by
Replying to jhpalmieri:
I would propose this change:
src/doc/en/thematic_tutorials/coercion_and_categories.rst
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 b Sage's category framework can differentiate the two cases:: 447 447 And indeed, ``MS2`` has *more* methods than ``MS1``:: 448 448 449 449 sage: import inspect 450 sage: len([s for s in dir(MS1) if inspect.ismethod(getattr(MS1,s,None))])451 81452 sage: len([s for s in dir(MS2) if inspect.ismethod(getattr(MS2,s,None))])453 121450 sage: L1 = len([s for s in dir(MS1) if inspect.ismethod(getattr(MS1,s,None))]) 451 sage: L2 = len([s for s in dir(MS2) if inspect.ismethod(getattr(MS2,s,None))]) 452 sage: L1 < L2 453 True 454 454 455 455 This is because the class of ``MS2`` also inherits from the parent 456 456 class for algebras:: The doctest in question is supposed to demonstrate that there are more methods for square matrices than nonsquare 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?
comment:26 Changed 3 years ago by
Branch:  u/jhpalmieri/provide_diagonal_matrix_as_a_method_of_matrix_spaces → u/ghSagnikDey92/provide_diagonal_matrix_as_a_method_of_matrix_spaces 

comment:27 Changed 3 years ago by
Commit:  a214fd2c452bb5e168afb78eb6babf78b1052417 → cd627626b7deefaa2e547bb79f1930613cbfaa69 

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.

comment:28 followup: 31 Changed 3 years ago by
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.
comment:29 Changed 3 years ago by
Branch:  u/ghSagnikDey92/provide_diagonal_matrix_as_a_method_of_matrix_spaces → u/jhpalmieri/provide_diagonal_matrix_as_a_method_of_matrix_spaces 

comment:30 Changed 3 years ago by
Commit:  cd627626b7deefaa2e547bb79f1930613cbfaa69 → f2d8ea9acfe88a25856e2f844f66f84d1ebfcc41 

Status:  needs_work → positive_review 
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

comment:31 Changed 3 years ago by
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!
comment:32 Changed 3 years ago by
Branch:  u/jhpalmieri/provide_diagonal_matrix_as_a_method_of_matrix_spaces → f2d8ea9acfe88a25856e2f844f66f84d1ebfcc41 

Resolution:  → fixed 
Status:  positive_review → closed 
Ticket retargeted after milestone closed