Jutho / KrylovKit.jl

Krylov methods for linear problems, eigenvalues, singular values and matrix functions
Other
284 stars 37 forks source link

Add svdsolve AD rule #84

Closed pbrehmer closed 5 months ago

pbrehmer commented 5 months ago

This PR adds a reverse-rule for svdsolve which implements the truncated SVD adjoint recently derived in this pre-print. See also PR #83.

pbrehmer commented 5 months ago

For the first commit I mostly copied the code from quantumghent/PEPSKit.jl#15 and adapted it a bit to KrylovKit. As of now, this is untested but the PEPSKit version runs correctly and produces results that are consistent with the TensorKit SVD reverse-rule. However, this PR does need a bit of help to get it fully running @lkdvos.

lkdvos commented 5 months ago

Great, thanks! ( I had in mind merging it into #83 first, such that I can immediately make it part of the package extension :smile: ) I'll try and have a look at this later this week!

Jutho commented 5 months ago

Am I correct that this only works for the case where A is an explicit matrix?

codecov[bot] commented 5 months ago

Codecov Report

Attention: Patch coverage is 0% with 98 lines in your changes are missing coverage. Please review.

Project coverage is 75.16%. Comparing base (4e8e1fa) to head (d9779c4).

Files Patch % Lines
ext/KrylovKitChainRulesCoreExt/svdsolve.jl 0.00% 49 Missing :warning:
src/adrules/svdsolve.jl 0.00% 49 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## ad-extension #84 +/- ## ================================================ - Coverage 77.40% 75.16% -2.25% ================================================ Files 30 31 +1 Lines 2957 2851 -106 ================================================ - Hits 2289 2143 -146 - Misses 668 708 +40 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

pbrehmer commented 5 months ago

Am I correct that this only works for the case where A is an explicit matrix?

Yes, I forgot to add that. I wasn't sure how KrylovKit internally handles the abstract vector types and what would be the best way to rewrite things like U = hcat(lvecs...) as well as the matrix multiplications that are performed.

lkdvos commented 5 months ago

@pbrehmer , I edited this PR so it now points to the branch on the main repository. Can you still move the implementation to the extension folder? I think I can merge after

pbrehmer commented 5 months ago

I think this should do the job? I hope this is mergeable now.

Jutho commented 5 months ago

Ok, I pushed some updates to the ad-extension branch, but now there is a conflict again. Can I resolve it (this will also push to your master) or do you want to rebase from your side?

pbrehmer commented 5 months ago

Can I resolve it (this will also push to your master) or do you want to rebase from your side?

Yes sure, you can go ahead and resolve it (and push), thanks!