Skip to content

Updated random_diagonal_matrix function so it works for any ring #39374

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

Merged
merged 7 commits into from
Feb 10, 2025

Conversation

Noel-Roemmele
Copy link
Contributor

@Noel-Roemmele Noel-Roemmele commented Jan 24, 2025

Fixes #24801. Added functionality to random_diagonal_matrix that allows a random matrix to be generated using an arbitrary ring instead of only the rational numbers. If the matrix of rational numbers is requested the integer ring is used in place of the rational ring to keep most of the functionality the same. Also updated the function documentation to reflect this change and added and extra test with the example from issue #24801.

📝 Checklist

  • [ X] The title is concise and informative.
  • [ X] The description explains in detail what this PR is about.
  • [ X] I have linked a relevant issue or discussion.
  • [ X] I have created tests covering the changes.
  • [ X] I have updated the documentation and checked the documentation preview.

⌛ Dependencies

Copy link

Documentation preview for this PR (built with commit 13d74a3; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

…ion of random_matrix. Removed redundant check to

see if the base ring provided was teh rationals. Moved doctest from tests to examples. Use the matrix.identity
function for the eigenvector_matrix instead.
@DaveWitteMorris
Copy link
Member

Thanks for doing this. It looks good, but I have a few minor suggestions.

lines 244-248, I think it would be good to keep the same format as the other input docs, i.e., start with "creates ...". Maybe: "creates a diagonalizable matrix. If the base ring is QQ, then the eigenvectors, if computed by hand, will have only integer entries. See the documentation of :meth:~sage.matrix.special.random_diagonalizable_matrix for more information.

line 3069: I think it would be more clear to say: "the desired parent of the square matrix (a matrix space)"

lines 3075-3076 sound repetitive because "If the ring used is QQ" is said twice. I think it would be better to combine these two by saying something like "If the ring used is QQ, then the matrix has integer entries, and the eigenspaces of the matrix, if computed by hand, give basis vectors with only integer entries."

line 3252: The signature of matrix.identity is matrix.identity(ring, n=0, sparse=False), so python will interpret the second appearance of size in matrix.identity(ring, size, size) as a value for the sparse keyword. That's not what you intended, so this should be changed to matrix.identity(ring, size).

…the matrix.identity call also suggested by

Dave Morris.
@DaveWitteMorris
Copy link
Member

LGTM. If there are no other comments in the next couple of days, I will set to positive review on Thursday.

@DaveWitteMorris
Copy link
Member

Oops, make ptestlong failed when trying to build the documentation, because of an error in the indentation. In the documentation of the diagonalizable algorithm (starting at line 244), each line after the first needs to be indented an extra 2 spaces. (Compare with the other paragraphs in that section. Sorry I didn't notice this earlier.) That may make one of the lines too long, so you may need another line break.

Also, I realized that the new doctest is incomplete because it doesn't actually verify that the method produces a diagonalizable matrix (and users would probably like to see sample output), so please replace it with something like the following:

        sage: K = GF(3)
        sage: M = random_matrix(K, 3, 3, algorithm="diagonalizable")
        sage: M.parent()
        Full MatrixSpace of 3 by 3 dense matrices over Finite Field of size 3
        sage: M.is_diagonalizable()
        True
        sage: M  # random
        [0 0 1]
        [2 1 1]
        [1 0 0]

…izable algorihtm of random_matrix. Added

verification of diagonalizability to the doctest for a random diagonal matrix of GF(3).
@Noel-Roemmele
Copy link
Contributor Author

Fixed diagonalizable algorithm space and added to the doctests.

@DaveWitteMorris
Copy link
Member

There haven't been any other comments, and make ptestlong had no unexpected errors, so I'm setting to positive review.

vbraun pushed a commit to vbraun/sage that referenced this pull request Feb 3, 2025
sagemathgh-39374: Updated random_diagonal_matrix function so it works for any ring
    
Fixes sagemath#24801. Added functionality to random_diagonal_matrix that allows
a random matrix to be generated using an arbitrary ring instead of only
the rational numbers. If the matrix of rational numbers is requested the
integer ring is used in place of the rational ring to keep most of the
functionality the same. Also updated the function documentation to
reflect this change and added and extra test with the example from issue
sagemath#24801.

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [ X] The title is concise and informative.
- [ X] The description explains in detail what this PR is about.
- [ X] I have linked a relevant issue or discussion.
- [ X] I have created tests covering the changes.
- [ X] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies
    
URL: sagemath#39374
Reported by: Noel-Roemmele
Reviewer(s):
vbraun pushed a commit to vbraun/sage that referenced this pull request Feb 4, 2025
sagemathgh-39374: Updated random_diagonal_matrix function so it works for any ring
    
Fixes sagemath#24801. Added functionality to random_diagonal_matrix that allows
a random matrix to be generated using an arbitrary ring instead of only
the rational numbers. If the matrix of rational numbers is requested the
integer ring is used in place of the rational ring to keep most of the
functionality the same. Also updated the function documentation to
reflect this change and added and extra test with the example from issue
sagemath#24801.

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [ X] The title is concise and informative.
- [ X] The description explains in detail what this PR is about.
- [ X] I have linked a relevant issue or discussion.
- [ X] I have created tests covering the changes.
- [ X] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies
    
URL: sagemath#39374
Reported by: Noel-Roemmele
Reviewer(s):
vbraun pushed a commit to vbraun/sage that referenced this pull request Feb 9, 2025
sagemathgh-39374: Updated random_diagonal_matrix function so it works for any ring
    
Fixes sagemath#24801. Added functionality to random_diagonal_matrix that allows
a random matrix to be generated using an arbitrary ring instead of only
the rational numbers. If the matrix of rational numbers is requested the
integer ring is used in place of the rational ring to keep most of the
functionality the same. Also updated the function documentation to
reflect this change and added and extra test with the example from issue
sagemath#24801.

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [ X] The title is concise and informative.
- [ X] The description explains in detail what this PR is about.
- [ X] I have linked a relevant issue or discussion.
- [ X] I have created tests covering the changes.
- [ X] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies
    
URL: sagemath#39374
Reported by: Noel-Roemmele
Reviewer(s):
vbraun pushed a commit to vbraun/sage that referenced this pull request Feb 9, 2025
sagemathgh-39374: Updated random_diagonal_matrix function so it works for any ring
    
Fixes sagemath#24801. Added functionality to random_diagonal_matrix that allows
a random matrix to be generated using an arbitrary ring instead of only
the rational numbers. If the matrix of rational numbers is requested the
integer ring is used in place of the rational ring to keep most of the
functionality the same. Also updated the function documentation to
reflect this change and added and extra test with the example from issue
sagemath#24801.

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [ X] The title is concise and informative.
- [ X] The description explains in detail what this PR is about.
- [ X] I have linked a relevant issue or discussion.
- [ X] I have created tests covering the changes.
- [ X] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies
    
URL: sagemath#39374
Reported by: Noel-Roemmele
Reviewer(s):
@vbraun vbraun merged commit 0143a19 into sagemath:develop Feb 10, 2025
3 checks passed
@Noel-Roemmele Noel-Roemmele deleted the 24801RandomDiagonalMatrix branch February 19, 2025 00:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

random_diagonalizable_matrix silently ignores the base ring
3 participants