Skip to content

Conversation

@kshyatt
Copy link
Member

@kshyatt kshyatt commented Aug 12, 2025

Attempted to wrap the randomized SVD method which relies on oversampling and the power method. Definitely open to some interface improvements here! Also, the ugly look of the tests suggests perhaps we should extend diagview a little...

@kshyatt kshyatt requested a review from Jutho August 12, 2025 17:10
@codecov
Copy link

codecov bot commented Aug 12, 2025

Codecov Report

❌ Patch coverage is 97.64706% with 4 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/implementations/orthnull.jl 90.47% 2 Missing ⚠️
ext/MatrixAlgebraKitCUDAExt/yacusolver.jl 97.05% 1 Missing ⚠️
src/implementations/svd.jl 98.64% 1 Missing ⚠️
Files with missing lines Coverage Δ
ext/MatrixAlgebraKitAMDGPUExt/yarocsolver.jl 86.66% <100.00%> (ø)
...MatrixAlgebraKitCUDAExt/MatrixAlgebraKitCUDAExt.jl 86.66% <100.00%> (+0.95%) ⬆️
src/MatrixAlgebraKit.jl 100.00% <ø> (ø)
src/common/gauge.jl 100.00% <100.00%> (ø)
src/implementations/eig.jl 100.00% <100.00%> (ø)
src/implementations/eigh.jl 98.41% <100.00%> (ø)
src/implementations/gen_eig.jl 100.00% <100.00%> (ø)
src/implementations/lq.jl 98.62% <100.00%> (ø)
src/implementations/polar.jl 100.00% <100.00%> (ø)
src/implementations/qr.jl 96.31% <100.00%> (ø)
... and 4 more

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Jutho
Copy link
Member

Jutho commented Aug 12, 2025

I haven't looked at this in detail yet. Do you think CUSOLVER_Randomized should be a valid argument for svd_full! given the warning note in the doc string? Wouldn't this make more sense as a svd_trunc! algorithm ?

@kshyatt
Copy link
Member Author

kshyatt commented Aug 12, 2025

Could well do better there since for best performance/accuracy k should be much much smaller than min(m,n). I can switch it in the morning.

@kshyatt
Copy link
Member Author

kshyatt commented Aug 13, 2025

The reason I didn't do this at first I think is that Xgesvdr expects U, S, and V to be full sized, which is rather annoying...

@lkdvos
Copy link
Member

lkdvos commented Aug 13, 2025

I do have somewhere on my todo list to make the check_input function also take the algorithm, would that help here?

I think I agree with Jutho though, that this might make more sense as an svd_trunc implementation, with some restrictions on the possible truncation strategies of course.

@kshyatt
Copy link
Member Author

kshyatt commented Aug 13, 2025

If we can modify check_input then that seems fine to me! I could do that as part of this PR, even?

Katharine Hyatt and others added 2 commits August 18, 2025 16:06
@kshyatt kshyatt enabled auto-merge (squash) August 18, 2025 14:55
Copy link
Member

@lkdvos lkdvos left a comment

Choose a reason for hiding this comment

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

Looks good to me! I left some minor comments, but otherwise I'm happy to merge. (GPU CI runs now right?)

@kshyatt
Copy link
Member Author

kshyatt commented Aug 18, 2025

CI does now run!

@kshyatt kshyatt merged commit 8e1c8e7 into main Aug 18, 2025
10 checks passed
@kshyatt kshyatt deleted the ksh/randsvd branch August 18, 2025 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants