Canonicalize Composite{SVD}

[mzgubic]

At the moment:

julia> x = Composite{SVD{Float64,Float64,Array{Float64,2}}}(V = [0.6662838804680036 -0.5008780874375076; 0.729969913633919 0.228331721577141],)

julia> ChainRules.canonicalize(x)
ERROR: ArgumentError: Composite fields do not match primal fields.
Composite fields: (:V,). Primal (SVD{Float64,Float64,Array{Float64,2}}) fields: (:U, :S, :Vt)

This seems to make sense because V is not a field of SVD. However, the rrule for svd expects the Ȳ::Composite to contain U, S, and V fields:

ChainRules.jl/src/rulesets/LinearAlgebra/factorization.jl

Lines 206 to 214 in 76ef95c

	function rrule(::typeof(svd), X::AbstractMatrix{<:Real})
	F = svd(X)
	function svd_pullback(Ȳ::Composite)
	# `getproperty` on `Composite`s ensures we have no thunks.
	∂X = svd_rev(F, Ȳ.U, Ȳ.S, Ȳ.V)
	return (NO_FIELDS, ∂X)
	end
	return F, svd_pullback
	end

I don't know much about svd factorisation so can't really tell why this is the case.

It kind of feels wrong to overload canonicalize in this way, but I can't quite tell why. Any thoughts? Also, which file to put this in, it's not really a rule so I don't think it belongs in the rulesets folder.

Came across this in FluxML/Zygote.jl#922 (comment)

[sethaxen]

It seems like it would be more straightforward to modify the rrule for getproperty(::SVD, ::Symbol) to store the cotangent of Vt instead of V. After all SVD actually has Vt as a field, and V is just its Adjoint.

[mzgubic]

It seems like it would be more straightforward to modify the rrule for getproperty(::SVD, ::Symbol) to store the cotangent of Vt instead of V. After all SVD actually has Vt as a field, and V is just its Adjoint.

Thanks for taking a look, that makes sense to me. For some reason I assumed that there is a complicated reason behind using V, should have checked that.

For getproperty pullback:

ChainRules.jl/src/rulesets/LinearAlgebra/factorization.jl

Lines 217 to 230 in 76ef95c

	function getproperty_svd_pullback(Ȳ)
	C = Composite{T}
	∂F = if x === :U
	C(U=Ȳ,)
	elseif x === :S
	C(S=Ȳ,)
	elseif x === :V
	C(V=Ȳ,)
	elseif x === :Vt
	# TODO: https://github.com/JuliaDiff/ChainRules.jl/issues/106
	throw(ArgumentError("Vt is unsupported; use V and transpose the result"))
	end
	return NO_FIELDS, ∂F, DoesNotExist()
	end

can we just do something like:

....
        elseif x === :V
            C(Vt=Ȳ',)
        elseif x === :Vt
            C(Vt=Ȳ,)
...

Or is there more to it?

[sethaxen]

can we just do something like:

....
        elseif x === :V
            C(Vt=Ȳ',)
        elseif x === :Vt
            C(Vt=Ȳ,)
...

Or is there more to it?

I think that's all there is to it. You could add copy(Ȳ') to make Vt not an Adjoint, but this is unnecessary, so long as the rrules for svd still work when the cotangent for Vt is an Adjoint.