Skip to content

random_diagonalizable_matrix silently ignores the base ring #24801

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.

Sign up for GitHub

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

Jump to bottom
Closed
nthiery opened this issue Feb 21, 2018 · 4 comments · Fixed by #39374
Closed

random_diagonalizable_matrix silently ignores the base ring #24801

nthiery opened this issue Feb 21, 2018 · 4 comments · Fixed by #39374
Assignees
@Noel-Roemmele
Labels
c: linear algebra p: major / 3 t: bug

Comments

@nthiery
Copy link
Contributor

nthiery commented Feb 21, 2018

A student of mine got surprised this morning :-)

    sage: K = GF(3)
    sage: random_matrix(K, 3,3,algorithm="diagonalizable").parent()
    Full MatrixSpace of 3 by 3 dense matrices over Rational Field

Recommendations:

  • Make it explicit in the documentation of random_matrix and random_diagonalizable_matrix that algorithm="diagonalizable" is only implemented for the rational field
  • Add a check for the base ring in random_diagonalizable_matrix, and fail gracefully with a NotImplementedError if it's not QQ.

Alternatively: implement random_diagonalizable_matrix for any base ring/field. It sounds like the current algorithm would produce a reasonable answer; so this could boil down to just creating the matrix in the appropriate parent; and updating the documentation to say something like "if the parent is QQ, then the eigenvectors are guaranteed to ...".

CC: @rbeezer

Component: linear algebra

Issue created by migration from https://trac.sagemath.org/ticket/24801
@nthiery nthiery added this to the sage-8.2 milestone Feb 21, 2018
@nthiery

This comment has been minimized.

Sign in to view
@nthiery nthiery changed the title random_diagonalizable_matrix silently ignores the base ring random_diagonalizable_matrix silently ignores the base ring Feb 21, 2018
@mkoeppe mkoeppe removed this from the sage-8.2 milestone Dec 29, 2022
@Noel-Roemmele Noel-Roemmele mentioned this issue Jan 24, 2025
Merged
@Noel-Roemmele
Copy link
Contributor

Current Pull request is a draft. Needs some small changes.

@Noel-Roemmele
Copy link
Contributor

Made required changes to pull request. Can now be updated to s: needs review.

@Noel-Roemmele
Copy link
Contributor

Made changes suggested by @DaveWitteMorris.

vbraun pushed a commit to vbraun/sage that referenced this issue Feb 3, 2025
4425c4d 
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 issue Feb 4, 2025
63a0906 
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 issue Feb 9, 2025
15cbef1 
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 issue Feb 9, 2025
aa33874 
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 closed this as completed in 0143a19 Feb 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Noel-Roemmele Noel-Roemmele

Labels
c: linear algebra p: major / 3 t: bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Updated random_diagonal_matrix function so it works for any ring Noel-Roemmele/sage
4 participants
@nthiery @mkoeppe @DaveWitteMorris @Noel-Roemmele