Some basic svd forward rules and tests#247
Conversation
|
Took another look at this. The Enzyme tests seem to be failing because of finite differences, for |
|
Your PR no longer requires formatting changes. Thank you for your contribution! |
Co-authored-by: Jutho <Jutho@users.noreply.github.com>
|
In the gauge fixing parts, we could use more views for the slicing |
|
Anyway, I will leave this aside now for a while and look at some other PRs. In principle, tests can now be expanded to cover the complex case and |
lkdvos
left a comment
There was a problem hiding this comment.
Left some final small comments, otherwise good to go?
| hUᴴΔAV₁ = inv_safe.(transpose(S₁) .- S₁) .* project_hermitian(UᴴΔAV₁) | ||
| aUᴴΔAV₁ = inv_safe.(transpose(S₁) .+ S₁) .* project_antihermitian(UᴴΔAV₁) |
There was a problem hiding this comment.
I think below only the sum and difference are actually used, we could use a kernel like
MatrixAlgebraKit.jl/src/implementations/polar.jl
Lines 209 to 220 in e271b4a
|
The GPU tests will probably fail until a new version is tagged at GPUArrays (JuliaGPU/GPUArrays.jl#738) |
Codecov Report❌ Patch coverage is
... and 4 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
Ran it locally and it worked, let's hope! |
| S, dS = arrayify(USVᴴ[2], dUSVᴴ[2]) | ||
| Vᴴ, dVᴴ = arrayify(USVᴴ[3], dUSVᴴ[3]) | ||
| $f!(A, USVᴴ, Mooncake.primal(alg_dalg)) | ||
| svd_pushforward!(dA, A, (U, S, Vᴴ), (dU, dS, dVᴴ)) |
There was a problem hiding this comment.
This again works because svd_pushforward! doesn't actually need A, since A is destroyed at this point. Not sure if it is worth to add a comment.
| # have to override this as methods are missing in GPUArrays for the various | ||
| # views of Diagonal of ΔA |
There was a problem hiding this comment.
This is a bit of a confusing comment: what exactly is missing?
Might be useful to keep track of this, in case it gets fixed.
There was a problem hiding this comment.
It's again a situation of mul!(::Diagonal{T, CuVector{T}}, [horrific view of adjoint of view], CuArray) which GPUArrays cannot dispatch onto at all.
|
Of course when I ran this locally (5 times!!!) it passed every single time. |
|
Some of these look like tolerance issues to me at least |
| if eltype(U) <: Complex && !iszerotangent(ΔU) && !iszerotangent(ΔVᴴ) # fix gauge for `gaugefix!` compatibility | ||
| _, I = findmax(abs, U₁; dims = 1) | ||
| infinitesimal_phases = imag.(ΔU₁[I] ./ U₁[I]) | ||
| infinitesimal_phases = imag.(ΔU₁[I] .* inv_safe.(U₁[I])) |
There was a problem hiding this comment.
I would be suprised if that is needed or makes a difference. U₁[I] is the maximum element of every column of U₁. As U₁ is an isometric matrix, all of its columns have norm 1, and therefore, the largest element needs to be at least 1/sqrt(m), in magnitude, with m the number of rows. Typically, it will be larger.
There was a problem hiding this comment.
I would be quite surprised too but I'm otherwise very confused where the NaN are emerging from (the ./transpose(S1) line has the same objection). I'll try stepping through the pushfoward to see if I can find the culprit.
|
I am also happy to take a look at the test failures. |
If you like! I'm about to get on a plane and unsure if it will have WiFi with which to push if I can find the problem |
Definitely not optimized...