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: sage-9.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:

Status badges

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 Erik Bray

Milestone: sage-9.0sage-9.1

Ticket retargeted after milestone closed

comment:2 Changed 3 years ago by Mahathi Vempati

Branch: u/gh-Tinkidinki/provide_diagonal_matrix_as_a_method_of_matrix_spaces

comment:3 Changed 3 years ago by Mahathi Vempati

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:

b9d128bDiagonal matrix function done
2913be1Documentation for diagonal matrix done
744d8f2Pointed ms.diagonal_matrix() to the diagonal_matrix() implementation

comment:4 Changed 3 years ago by Mahathi Vempati

Status: newneeds_review

comment:5 Changed 3 years ago by John Palmieri

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.

Last edited 3 years ago by John Palmieri (previous) (diff)

comment:6 Changed 3 years ago by Mahathi Vempati

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.

comment:7 Changed 3 years ago by Mahathi Vempati

Keywords: beginner added

comment:8 Changed 3 years ago by Mahathi Vempati

Status: needs_reviewneeds_work

comment:9 Changed 3 years ago by Sagnik Dey

Branch: u/gh-Tinkidinki/provide_diagonal_matrix_as_a_method_of_matrix_spacesu/gh-SagnikDey92/provide_diagonal_matrix_as_a_method_of_matrix_spaces

comment:10 Changed 3 years ago by Sagnik Dey

Commit: 744d8f2eae5ace821bf674998d74023045bc34f6bccdee385da0a05a708890989c504d180fbbe562
Status: needs_workneeds_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:

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

comment:11 Changed 3 years ago by John Palmieri

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): 
    16001600        """
    16011601        if self.__nrows != self.__ncols:
    16021602            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())
    16041604        A.set_immutable()
    16051605        return A
    16061606

Why make it immutable by default?

Last edited 3 years ago by John Palmieri (previous) (diff)

comment:12 Changed 3 years ago by git

Commit: bccdee385da0a05a708890989c504d180fbbe56212eca57a2fc73f795a053d836559b12216074cbe

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

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

comment:13 in reply to:  11 Changed 3 years ago by Sagnik Dey

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

  • 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): 
    16001600        """
    16011601        if self.__nrows != self.__ncols:
    16021602            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())
    16041604        A.set_immutable()
    16051605        return A
    16061606

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 John Palmieri

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 Sagnik Dey

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

Commit: 12eca57a2fc73f795a053d836559b12216074cbef48d91551e994a81dbe7e6edd639d35b994608d3

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

f48d915Removed immutability of diagonal matrix and edited documentation appropriately

comment:17 in reply to:  16 Changed 3 years ago by Sagnik Dey

Replying to git:

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

f48d915Removed immutability of diagonal matrix and edited documentation appropriately

Please review

comment:18 Changed 3 years ago by John Palmieri

Branch: u/gh-SagnikDey92/provide_diagonal_matrix_as_a_method_of_matrix_spacesu/jhpalmieri/provide_diagonal_matrix_as_a_method_of_matrix_spaces

comment:19 Changed 3 years ago by John Palmieri

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

86e5d33Diagonal matrix function done
68ce495Documentation for diagonal matrix done
242a5d7Pointed ms.diagonal_matrix() to the diagonal_matrix() implementation
42a4810Added non square matrix error and some documentation. Made diagonal matrix returned immutable
cb656f3Modeled diagonal_matrix on identity_matrix. Changed the documentation accordingly. No longer uses the function in special.py.
29f3805Removed immutability of diagonal matrix and edited documentation appropriately
a214fd2trac 28882: reviewer changes

comment:20 Changed 3 years ago by Sagnik Dey

Authors: Sagnik Dey
Status: needs_reviewpositive_review

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

comment:21 Changed 3 years ago by Volker Braun

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

comment:22 in reply to:  21 Changed 3 years ago by Sagnik Dey

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?

comment:23 Changed 3 years ago by John Palmieri

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:: 
    447447And indeed, ``MS2`` has *more* methods than ``MS1``::
    448448
    449449    sage: import inspect
    450     sage: len([s for s in dir(MS1) if inspect.ismethod(getattr(MS1,s,None))])
    451     81
    452     sage: len([s for s in dir(MS2) if inspect.ismethod(getattr(MS2,s,None))])
    453     121
     450    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
    454454
    455455This is because the class of ``MS2`` also inherits from the parent
    456456class 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.

comment:24 in reply to:  23 Changed 3 years ago by Sagnik Dey

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:: 
    447447And indeed, ``MS2`` has *more* methods than ``MS1``::
    448448
    449449    sage: import inspect
    450     sage: len([s for s in dir(MS1) if inspect.ismethod(getattr(MS1,s,None))])
    451     81
    452     sage: len([s for s in dir(MS2) if inspect.ismethod(getattr(MS2,s,None))])
    453     121
     450    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
    454454
    455455This is because the class of ``MS2`` also inherits from the parent
    456456class 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?

comment:25 Changed 3 years ago by John Palmieri

That was vbraun, not me.

comment:26 Changed 3 years ago by Sagnik Dey

Branch: u/jhpalmieri/provide_diagonal_matrix_as_a_method_of_matrix_spacesu/gh-SagnikDey92/provide_diagonal_matrix_as_a_method_of_matrix_spaces

comment:27 in reply to:  25 Changed 3 years ago by Sagnik Dey

Commit: a214fd2c452bb5e168afb78eb6babf78b1052417cd627626b7deefaa2e547bb79f1930613cbfaa69

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:

9bc5ef5Diagonal matrix function done
c20ffceDocumentation for diagonal matrix done
5463512Pointed ms.diagonal_matrix() to the diagonal_matrix() implementation
a9308bfAdded non square matrix error and some documentation. Made diagonal matrix returned immutable
4fa5373Modeled diagonal_matrix on identity_matrix. Changed the documentation accordingly. No longer uses the function in special.py.
091cd1dRemoved immutability of diagonal matrix and edited documentation appropriately
cd62762Modified a doctest that replaces checking for exact number of methods for Matrix spaces with a comparison test.

comment:28 Changed 3 years ago by John Palmieri

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 John Palmieri

Branch: u/gh-SagnikDey92/provide_diagonal_matrix_as_a_method_of_matrix_spacesu/jhpalmieri/provide_diagonal_matrix_as_a_method_of_matrix_spaces

comment:30 Changed 3 years ago by John Palmieri

Commit: cd627626b7deefaa2e547bb79f1930613cbfaa69f2d8ea9acfe88a25856e2f844f66f84d1ebfcc41
Status: needs_workpositive_review

Here's a branch including that change.


New commits:

c279781Diagonal matrix function done
b132688Documentation for diagonal matrix done
f56aee3Pointed ms.diagonal_matrix() to the diagonal_matrix() implementation
54620b2Added non square matrix error and some documentation. Made diagonal matrix returned immutable
5c2a5b2Modeled diagonal_matrix on identity_matrix. Changed the documentation accordingly. No longer uses the function in special.py.
14c422aRemoved immutability of diagonal matrix and edited documentation appropriately
758e943trac 28882: reviewer changes
f2d8ea9trac 28882: fix doctest in thematic_tutorials

comment:31 in reply to:  28 Changed 3 years ago by Sagnik Dey

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 Volker Braun

Branch: u/jhpalmieri/provide_diagonal_matrix_as_a_method_of_matrix_spacesf2d8ea9acfe88a25856e2f844f66f84d1ebfcc41
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.