#28882 closed enhancement (fixed)

Provide diagonal_matrix as a method of matrix spaces

Reported by: slelievre 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 21 months ago by embray

  • Milestone changed from sage-9.0 to sage-9.1

Ticket retargeted after milestone closed

comment:2 Changed 20 months ago by gh-Tinkidinki

  • Branch set to u/gh-Tinkidinki/provide_diagonal_matrix_as_a_method_of_matrix_spaces

comment:3 Changed 20 months ago by gh-Tinkidinki

  • Commit set to 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 20 months ago by gh-Tinkidinki

  • Status changed from new to needs_review

comment:5 Changed 20 months ago by jhpalmieri

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 20 months ago by jhpalmieri (previous) (diff)

comment:6 Changed 20 months ago by gh-Tinkidinki

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 20 months ago by gh-Tinkidinki

  • Keywords beginner added

comment:8 Changed 20 months ago by gh-Tinkidinki

  • Status changed from needs_review to needs_work

comment:9 Changed 19 months ago by gh-SagnikDey92

  • Branch changed 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

comment:10 Changed 19 months ago by gh-SagnikDey92

  • Commit changed from 744d8f2eae5ace821bf674998d74023045bc34f6 to bccdee385da0a05a708890989c504d180fbbe562
  • Status changed from needs_work to 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:

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

comment:11 follow-up: Changed 19 months ago by 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?

Last edited 19 months ago by jhpalmieri (previous) (diff)

comment:12 Changed 19 months ago by git

  • Commit changed from bccdee385da0a05a708890989c504d180fbbe562 to 12eca57a2fc73f795a053d836559b12216074cbe

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 19 months ago by gh-SagnikDey92

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 19 months ago by jhpalmieri

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 19 months ago by gh-SagnikDey92

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 follow-up: Changed 19 months ago by git

  • Commit changed from 12eca57a2fc73f795a053d836559b12216074cbe to f48d91551e994a81dbe7e6edd639d35b994608d3

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 19 months ago by gh-SagnikDey92

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 19 months ago by jhpalmieri

  • Branch changed 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

comment:19 Changed 19 months ago by jhpalmieri

  • Commit changed from f48d91551e994a81dbe7e6edd639d35b994608d3 to a214fd2c452bb5e168afb78eb6babf78b1052417
  • Reviewers set to 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 19 months ago by gh-SagnikDey92

  • Authors set to Sagnik Dey
  • Status changed from needs_review to positive_review

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

comment:21 follow-up: Changed 19 months ago by vbraun

  • Status changed from positive_review to needs_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 19 months ago by gh-SagnikDey92

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 follow-up: Changed 19 months ago by 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.

comment:24 in reply to: ↑ 23 Changed 19 months ago by gh-SagnikDey92

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 follow-up: Changed 19 months ago by jhpalmieri

That was vbraun, not me.

comment:26 Changed 19 months ago by gh-SagnikDey92

  • Branch changed 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

comment:27 in reply to: ↑ 25 Changed 19 months ago by gh-SagnikDey92

  • Commit changed from a214fd2c452bb5e168afb78eb6babf78b1052417 to 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:

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 follow-up: Changed 19 months ago by jhpalmieri

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 19 months ago by jhpalmieri

  • Branch changed 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

comment:30 Changed 19 months ago by jhpalmieri

  • Commit changed from cd627626b7deefaa2e547bb79f1930613cbfaa69 to f2d8ea9acfe88a25856e2f844f66f84d1ebfcc41
  • Status changed from needs_work to positive_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 19 months ago by gh-SagnikDey92

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 19 months ago by vbraun

  • Branch changed from u/jhpalmieri/provide_diagonal_matrix_as_a_method_of_matrix_spaces to f2d8ea9acfe88a25856e2f844f66f84d1ebfcc41
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.