-
Notifications
You must be signed in to change notification settings - Fork 154
Fix implementations of eigen
and eigvals
#757
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
base: master
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -238,22 +238,74 @@ end | |
end | ||
|
||
@testset "eigen" begin | ||
@test ForwardDiff.jacobian(x -> eigvals(SymTridiagonal(x, x[1:end-1])), [1.,2.]) ≈ [(1 - 3/sqrt(5))/2 (1 - 1/sqrt(5))/2 ; (1 + 3/sqrt(5))/2 (1 + 1/sqrt(5))/2] | ||
@test ForwardDiff.jacobian(x -> eigvals(Symmetric(x*x')), [1.,2.]) ≈ [0 0; 2 4] | ||
|
||
x0 = [1.0, 2.0]; | ||
ev1(x) = eigen(Symmetric(x*x')).vectors[:,1] | ||
@test ForwardDiff.jacobian(ev1, x0) ≈ Calculus.finite_difference_jacobian(ev1, x0) | ||
ev2(x) = eigen(SymTridiagonal(x, x[1:end-1])).vectors[:,1] | ||
@test ForwardDiff.jacobian(ev2, x0) ≈ Calculus.finite_difference_jacobian(ev2, x0) | ||
|
||
x0_svector = SVector{2}(x0) | ||
@test ForwardDiff.jacobian(ev1, x0_svector) isa SMatrix{2, 2} | ||
@test ForwardDiff.jacobian(ev1, x0_svector) ≈ Calculus.finite_difference_jacobian(ev1, x0) | ||
|
||
x0_mvector = MVector{2}(x0) | ||
@test ForwardDiff.jacobian(ev1, x0_mvector) isa MMatrix{2, 2} | ||
@test ForwardDiff.jacobian(ev1, x0_mvector) ≈ Calculus.finite_difference_jacobian(ev1, x0) | ||
eigvals_symreal(x) = eigvals(Symmetric(x*x')) | ||
eigvals_hermreal(x) = eigvals(Hermitian(x*x')) | ||
eigvals_hermcomplex(x) = map(abs2, eigvals(Hermitian(complex.(x*x', x'*x)))) | ||
eigvals_symtridiag(x) = eigvals(SymTridiagonal(x, x[begin:(end - 1)])) | ||
|
||
eigen_vals_symreal(x) = eigen(Symmetric(x*x')).values | ||
eigen_vals_hermreal(x) = eigen(Hermitian(x*x')).values | ||
eigen_vals_hermcomplex(x) = map(abs2, eigen(Hermitian(complex.(x*x', x'*x))).values) | ||
eigen_vals_symtridiag(x) = eigen(SymTridiagonal(x, x[begin:(end - 1)])).values | ||
|
||
eigen_vec1_symreal(x) = eigen(Symmetric(x*x')).vectors[:,1] | ||
eigen_vec1_hermreal(x) = eigen(Hermitian(x*x')).vectors[:,1] | ||
eigen_vec1_hermcomplex(x) = map(abs2, eigen(Hermitian(complex.(x*x', x'*x))).vectors[:,1]) | ||
eigen_vec1_symtridiag(x) = eigen(SymTridiagonal(x, x[begin:(end - 1)])).vectors[:,1] | ||
|
||
# Note: SymTridiagonal is not supported for StaticArrays | ||
for T in (Int, Float32, Float64), A in (Array, SArray, MArray) | ||
if A <: StaticArrays.StaticArray | ||
x = A{Tuple{2},T}(x0) | ||
JT = A{Tuple{2,2},float(T)} | ||
else | ||
x = A{T,1}(x0) | ||
JT = A{float(T),2} | ||
end | ||
|
||
# analytic solutions | ||
@test ForwardDiff.jacobian(eigvals_symreal, x) ≈ [0 0; 2 4] | ||
@test ForwardDiff.jacobian(eigvals_hermreal, x) ≈ [0 0; 2 4] | ||
if !(x isa StaticArrays.StaticArray) | ||
@test ForwardDiff.jacobian(eigvals_symtridiag, x) ≈ [(1 - 3/sqrt(5))/2 (1 - 1/sqrt(5))/2 ; (1 + 3/sqrt(5))/2 (1 + 1/sqrt(5))/2] | ||
end | ||
Comment on lines
+272
to
+277
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I haven't looked very closely, but are there more tests somewhere that check the values? In what's changed in this PR, I see one more line with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, there are more in the lines below. Much more exhaustive than on master. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry that wasn't clear. I see the many tests of different paths against each other, which is good. But what I meant is tests of any ForwardDiff path against something completely independent -- a known answer, or finite differences. To catch things like making the same logic error in writing both There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the tests below, Jacobians of every test function with ForwardDiff are checked against finite differencing based Jacobians. The latter doesn't involve any ForwardDiff paths. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry to be dense but where are you looking? Besides line 293, as mentioned:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is line 293. Calculus doesn't support all vector types, hence the ForwardDiff results with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok. So the matrices being tested are these. Does it seem OK to test e.g. no negative eigenvalue for Symmetric{Real}, no zero eigenvalues for Hermitian{Complex}? Haven't thought hard but it just seemed a small sample. julia> x
2-element Vector{Float64}:
1.0
2.0
julia> Symmetric(x*x') # also wrapped in Hermitian
2×2 Symmetric{Float64, Matrix{Float64}}:
1.0 2.0
2.0 4.0
julia> eigvals(ans)
2-element Vector{Float64}:
0.0
5.0
julia> Hermitian(complex.(x*x', x'*x))
2×2 Hermitian{ComplexF64, Matrix{ComplexF64}}:
1.0+0.0im 2.0+5.0im
2.0-5.0im 4.0+0.0im
julia> eigvals(ans)
2-element Vector{Float64}:
-3.0901699437494754
8.090169943749475 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (That said, when I tried various tests locally, I did not manage to break this.) |
||
|
||
# eigen + eigvals | ||
for ev in ( | ||
eigvals_symreal, eigvals_hermreal, eigvals_hermcomplex, eigvals_symtridiag, | ||
eigen_vals_symreal, eigen_vals_hermreal, eigen_vals_hermcomplex, eigen_vals_symtridiag, | ||
eigen_vec1_symreal, eigen_vec1_hermreal, eigen_vec1_hermcomplex, eigen_vec1_symtridiag, | ||
) | ||
if x isa StaticArrays.StaticArray && | ||
(ev === eigvals_symtridiag || ev === eigen_vals_symtridiag || ev === eigen_vec1_symtridiag) | ||
continue | ||
end | ||
|
||
# Chunk size can only be inferred for static arrays | ||
if x isa StaticArrays.StaticArray | ||
@test @inferred(ForwardDiff.jacobian(ev, x)) isa JT | ||
else | ||
@test ForwardDiff.jacobian(ev, x) isa JT | ||
end | ||
cfg = ForwardDiff.JacobianConfig(ev, x) | ||
@test @inferred(ForwardDiff.jacobian(ev, x, cfg)) isa JT | ||
|
||
@test ForwardDiff.jacobian(ev, x) ≈ Calculus.finite_difference_jacobian(ev, float.(x0)) | ||
end | ||
|
||
# consistency of eigen and eigvals | ||
for (eigvals, eigen_vals) in ( | ||
(eigvals_symreal, eigen_vals_symreal), | ||
(eigvals_hermreal, eigen_vals_hermreal), | ||
(eigvals_hermcomplex, eigen_vals_hermcomplex), | ||
(eigvals_symtridiag, eigen_vals_symtridiag), | ||
) | ||
if x isa StaticArrays.StaticArray && eigvals === eigvals_symtridiag | ||
continue | ||
end | ||
@test ForwardDiff.jacobian(eigvals, x) ≈ ForwardDiff.jacobian(eigen_vals, x) | ||
end | ||
end | ||
end | ||
|
||
@testset "type stability" begin | ||
|
Uh oh!
There was an error while loading. Please reload this page.