Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
🚀 New features to boost your workflow:
|
| inv_S_minus = _broadened_inv_S(Sp, tol, broadening) # possibly divergent/broadened contribution | ||
| UdΔAV = @. (aUΔU + aVΔV) * inv_S_minus + (aUΔU - aVΔV) * _safe_inv(Sp' .+ Sp, tol) |
There was a problem hiding this comment.
Note that this has a slightly different implementation than before: this is actually instantiating an intermediate array, whereas previously everything was fused into a single computation.
I'm not entirely sure if this really matters though, just wanted to mention that.
There was a problem hiding this comment.
I found it easier to instantiate the inv_S_minus intermediate array since the _broadened_inv_S function explicitly iterates over the axes of Sp. I thought it would probably not matter that much.
src/utility/svd.jl
Outdated
| (UrΔU .- VrΔV) .* _safe_inv.(Sp' .+ Sr, tol) | ||
| ) | ||
| X = @. (1//2) * ( | ||
| (UrΔU + VrΔV) * _safe_inv(Sp' .- Sr, tol) + |
There was a problem hiding this comment.
You seem to have removed a broadening here, was this on purpose?
There was a problem hiding this comment.
I was unsure if broadening would make sense on the truncated bits. I suppose also in that contribution there might zero entries from degenerate singular values which are now just suppressed through the cutoff. Do you think it would be more consistent if we also use broadening on these terms?
src/utility/svd.jl
Outdated
| (UrΔU - VrΔV) * _safe_inv(Sp' .+ Sr, tol) | ||
| ) | ||
| Y = @. (1//2) * ( | ||
| (UrΔU + VrΔV) * _safe_inv(Sp' .- Sr, tol) - |
There was a problem hiding this comment.
Same comment about removed broadening here.
Co-authored-by: Lukas Devos <ldevos98@gmail.com>
Co-authored-by: Lukas Devos <ldevos98@gmail.com>
Co-authored-by: Lukas Devos <ldevos98@gmail.com>
Co-authored-by: Lukas Devos <ldevos98@gmail.com>
lkdvos
left a comment
There was a problem hiding this comment.
I'm okay with merging this, but keep in mind that I have very little experience or tests to support a good intuition for whether or not these changes are better, worse, or don't really change anything at all. Perhaps a second opinion could be nice, but I also guess it can't hurt too much to merge this either.
|
I would think that these changes overall have only minor consequences. But at least we now test a case with degenerate singular values where we do see that the current changes do what they are supposed to do. The thing that is kind of unclear is whether the broadening is an improvement/deterioration over just having a cutoff (which sets the divergent contributions to zero). I talked with Anna & Bram and they do something very similar but for the truncated contribution in the SVD rrule they solve a linear problem (kind of KrylovKit-style) instead of using the truncated bits from the full SVD - so there it is kind of unclear what the broadening should do. In any case, overall I'm also okay with merging this but it's good to keep in mind the places where we're still unsure. |
Initiated from #192, this PR updates/fixes the Lorentzian broadening in the SVD reverse-rule. I also updated the corresponding test where we can see that degenerate singular values lead to wrong gradients if there is no broadening and no cutoff.
In the tests on random matrices it seems that using a cutoff for the degenerate inverse singular value differences - thereby just setting them to zero - kind of has the same effect as the broadening.