Skip to content

Conversation

@mforets
Copy link
Contributor

@mforets mforets commented Dec 11, 2020

This PR continues #38793, addressing comment JuliaLang/LinearAlgebra.jl#796. The motivation is to fix a small inconsistency between the SymTridiagonal type and admissible calls to eigen / eigen! for that type.

In master, the SymTridiagonal constructor allows the superdiagonal vector (ev) to be of length either n or n-1 (where n is the length of the main diagonal), but some calls to LAPACK eigensolvers which dispatch on SymTridiagonal through eigen / eigen! are stricter, only allowing ev to have dimension n-1.

CC @dkarrasch

Copy link
Member

@dkarrasch dkarrasch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice!

@test_throws DimensionMismatch LAPACK.stebz!('A','B',zero(elty),zero(elty),0,0,-1.,d,rand(elty,10))
@test_throws DimensionMismatch LAPACK.stegr!('N','A',d,rand(elty,11),zero(elty),zero(elty),0,0)
@test_throws DimensionMismatch LAPACK.stein!(d,zeros(elty,10),zeros(elty,10),zeros(BlasInt,10),zeros(BlasInt,10))
@test_throws DimensionMismatch LAPACK.stein!(d,zeros(elty,11),zeros(elty,10),zeros(BlasInt,10),zeros(BlasInt,10))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to also have positive tests, so that this functionality cannot regress in the future. Like have an example where both dv and ev have the same length, and then call eigen, eigvals etc. on them. For simplicity, you could choose ev = zeros(eltype(dv), length(dv)), then you have the eigenvalues on the diagonal, and you can check the numerical values easily.

@dkarrasch dkarrasch added the linear algebra Linear algebra label Dec 11, 2020
@vtjnash vtjnash added the needs tests Unit tests are required for this change label Apr 3, 2021
@vtjnash
Copy link
Member

vtjnash commented Apr 3, 2021

bump. apparently just needs a quick test?

@vtjnash
Copy link
Member

vtjnash commented Apr 16, 2021

bump?

@mforets
Copy link
Contributor Author

mforets commented Apr 20, 2021

Thank you @dkarrasch, and sorry for dropping the ball on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

linear algebra Linear algebra needs tests Unit tests are required for this change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants