Skip to content

Conversation

@pbrehmer
Copy link
Collaborator

In this PR we will add a rrule for tsvd! which supports Lorentzian broadening. To that end, I copy over TensorKit's svd_pullback! and modify the save_inv function to support a broadened inverse.

Additionally, I will add a keyword argument to control the gauge-dependency warnings in that reverse-rule (which is currently not possible) since in some optimizations, the gauge-dependency warnings are really out of control even though they are non-problematic.

@pbrehmer
Copy link
Collaborator Author

I'll add a test for the Lorentzian broadening tomorrow and then it should be relatively ready to be merged.

But there's one thing where I'm not sure yet: I want a user verbosity kwarg for the gauge warnings in svd_pullback!. However, since we dispatch that rrule on Nothing, there's is no way for the user to determine anything in svd_pullback! except the broadening, since that is a separate field in SVDAdjoint.

My idea to circumvent this would be the following: Add a custom SVD rrule alg for the modified svd_pullback! (in the same way that GMRES, BiCGStab, etc. have their own algorithm struct) which contains a verbosity and a broadening field. I would then get rid of the broadening in SVDAdjoint since most algorithms don't support this anyway. I feel that it would make more sense for the rrule_alg to contain the broadening field since that is a setting that only has effect during the reverse-pass. Also, I would find it very reasonable if the SVDAdjoint only contained a fwd_alg and a rrule_alg since it is a priori not clear if all SVD adjoints should even support some kind of broadening.

@lkdvos, what do you think about this?

@codecov
Copy link

codecov bot commented Apr 17, 2025

Codecov Report

Attention: Patch coverage is 84.44444% with 14 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/utility/svd.jl 84.26% 14 Missing ⚠️
Files with missing lines Coverage Δ
src/Defaults.jl 85.71% <ø> (ø)
src/PEPSKit.jl 100.00% <ø> (ø)
src/algorithms/ctmrg/projectors.jl 82.69% <100.00%> (ø)
...rithms/optimization/fixed_point_differentiation.jl 93.07% <ø> (ø)
src/utility/svd.jl 86.93% <84.26%> (-2.50%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@pbrehmer
Copy link
Collaborator Author

Add a custom SVD rrule alg for the modified svd_pullback! (in the same way that GMRES, BiCGStab, etc. have their own algorithm struct) which contains a verbosity and a broadening field.

I gave this a go and I do like it since it does improve usability quite a bit. I was previously running into cases where the gauge warning output was so verbose that it basically made the optimization run information ineligible. I think I also prefer having SVDAdjoint{F,R} without the broadening as an extra field and instead put it in the rrule_alg. Let me know what you think :-)

@pbrehmer pbrehmer requested a review from lkdvos April 23, 2025 15:40
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.

Overall looks good to me!
For the default value of the broadening, it would probably be better to have that as a relative thing (and this depends on the datatype), so maybe this could be somewhat improved?

Otherwise I can only comment on the name, FullReverse is very non-descriptive, I can't think of anything much better but I would maybe add SVD somewhere in there?

@pbrehmer
Copy link
Collaborator Author

Yes I agree, the FullReverse name was not very good, so I renamed it to FullSVDReverseRule.

I adjusted the default broadening value but inside the Defaults I left it at svd_rrule_broadening=1e-13 since there is no way (I think) to determine the relevant floating-point accuracy.

For me this would be good to go now!

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.

Okay for me once tests pass

@pbrehmer pbrehmer enabled auto-merge (squash) April 23, 2025 18:53
@pbrehmer pbrehmer merged commit 994a94f into master Apr 23, 2025
39 checks passed
@pbrehmer pbrehmer deleted the pb-lorentzian-broadening branch April 23, 2025 20:03
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