Skip to content
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

Add svdsolve AD rule #84

Merged
merged 6 commits into from
May 13, 2024
Merged

Add svdsolve AD rule #84

merged 6 commits into from
May 13, 2024

Conversation

pbrehmer
Copy link
Contributor

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
Copy link
Contributor Author

For the first commit I mostly copied the code from QuantumKitHub/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
Copy link
Collaborator

lkdvos commented Apr 24, 2024

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

@Jutho
Copy link
Owner

Jutho commented Apr 25, 2024

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

Copy link

codecov bot commented Apr 25, 2024

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 ⚠️
src/adrules/svdsolve.jl 0.00% 49 Missing ⚠️
Additional details and impacted files
@@               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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pbrehmer
Copy link
Contributor Author

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 lkdvos changed the base branch from master to ad-extension May 13, 2024 13:26
@lkdvos
Copy link
Collaborator

lkdvos commented May 13, 2024

@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
Copy link
Contributor Author

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

@Jutho
Copy link
Owner

Jutho commented May 13, 2024

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
Copy link
Contributor Author

pbrehmer commented May 13, 2024

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!

@Jutho Jutho merged commit c4f6a48 into Jutho:ad-extension May 13, 2024
0 of 2 checks passed
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.

3 participants