-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
Added SubArray tests for linalg/eigen.jl and linalg/schur.jl #15364
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
Conversation
8e7e473 to
d98ef63
Compare
test/linalg/schur.jl
Outdated
| sum(select) != 0 && @test_approx_eq S[:values][find(select)] O[:values][1:sum(select)] | ||
| @test_approx_eq O[:vectors]*O[:Schur]*O[:vectors]' ordschura | ||
| @test_throws KeyError f[:A] | ||
| Snew = Base.LinAlg.Schur(copy(S.T), copy(S.Z), copy(S.values)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if you remove the copys here and instead adds a copy(Snew) in the next line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like so?
Snew = Base.LinAlg.Schur(S.T, S.Z, S.values)
SchurNew = ordschur!(copy(Snew), select)It gives me
ERROR: MethodError: no method matching copy(::Base.LinAlg.Schur{Float32,Array{Float32,2}})There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a suspicion that this would be the result. Would you mind adding the two copy methods for Schur and GeneralizedSchur to this PR? They should be almost trivial.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. Keep the suggestions above, and add copys?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. copy inside the ordschur! functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By "add copys" I meant method definitions, but yes, we agree :) I'll probably do it tomorrow.
|
Test passed, so it should be 👍 . The reason why it is running again, is because I rebased it. |
Added SubArray tests for linalg/eigen.jl and linalg/schur.jl
These tests are added to make sure that the linear algebra functions work with
SubArrays, as proposed in JuliaLang/LinearAlgebra.jl#299.ArrayandSubArrayineigen.jl, and appropriately definea,asym,apd,asym_sg,a_sg,a1_nsg, anda2_nsg(andaagain at the bottom`).ArrayandSubArrayinschur.jland appropriately definea,asym,apd,a1_sf, anda2_sf.Everything seems to work fine with
SubArrays.Edit: I've seen the whitespace errors, will fix when at a computer.