Add some tests for randomized SVD #159
Conversation
test/testsuite/svd.jl
Outdated
| minmn = min(m, n) | ||
| S₀ = collect(svd_vals(A)) | ||
| U1, S1, V1ᴴ, ϵ1 = @testinferred svd_trunc(A; alg) | ||
| @test collect(diagview(S1))[1:alg.k] ≈ S₀[1:alg.k] |
There was a problem hiding this comment.
Overall looks good, but this alg.k kind of sticks out a bit, do we always expect this property to be available?
On a separate note, should we think about whether or not the svd_trunc should really just output an S of length k to begin with?
That way, we can simply do:
@test collect(diagview(S1)) ≈ S₀[axes(S1, 1)]There was a problem hiding this comment.
Should I add a comment that we should revisit this? IDK if this has to be solved in this PR
Codecov Report✅ All modified and coverable lines are covered by tests.
... and 2 files with indirect coverage changes 🚀 New features to boost your workflow:
|
kshyatt
force-pushed
the
ksh/randomized_svd
branch
from
adb7cda to
3afd7bc
Compare
lkdvos
left a comment
lkdvos
left a comment
There was a problem hiding this comment.
Looks like a strict improvement, ok to go for me!
As discussed, at some point we might want to revisit the entire randomized SVD (and other factorizations) approach in its entirety, but not today 😉
| (Utr, Str, Vᴴtr), _ = truncate(svd_trunc!, (U, S, Vᴴ), alg.trunc) | ||
|
|
||
| do_gauge_fix = get(alg.alg.kwargs, :fixgauge, default_fixgauge())::Bool | ||
| # the output matrices here are the same size as for svd_full! |
There was a problem hiding this comment.
I am confused about the conclusion of the resolved comments, and just about the validity of this comment in the code, as well as why it is here in between those two code lines. The arguments to gaugefix! on the line below are already truncated, right?
There was a problem hiding this comment.
I agree this comment should not be here, good catch.
I got confused because the output from the CUSOLVER randomized SVD has the same size as the output from svd_full - the matrices are only reduced in size when we directly call truncate which is/was confusing to me. I think what I should do is a) fix this comment b) add more documentation explaining this.
3 participants
Definitely open to feedback on this one but thought we should start testing this. Also simplified the implementation a bit to reduce repeated code.