Add rules for all Symmetric/Hermitian constructors #182
Conversation
|
I left out the Symmetric -> Matrix and Matrix -> Symmetric conversions at https://github.com/FluxML/Zygote.jl/blob/0b3e32d5c0f8e5ef3b061a6f84ed5505f5a202a0/src/lib/array.jl#L493-L496 but could add those as well. Has there been discussion on a generic |
|
Blocked by #198 |
|
Okay, this is ready for review. I also unified the constructors and also added Since between this and #193 there's a disproportionately large amount of |
|
I'll be reviewing tomorrow morning, please don't merge before then. |
| function rrule(T::Type{<:LinearAlgebra.HermOrSym}, A::AbstractMatrix, uplo) | ||
| Ω = T(A, uplo) | ||
| function HermOrSym_pullback(ΔΩ) | ||
| return (NO_FIELDS, @thunk(_symherm_back(T, ΔΩ, Ω.uplo)), DoesNotExist()) |
There was a problem hiding this comment.
Do we need a thunk here? The cotangent w.r.t. A is the only meaningful, so I would imagine that it would always get used somewhere, but my intuition for this isn't great, perhaps yours is better.
There was a problem hiding this comment.
Probably not? I had thought that the convention was to always thunk in reverse unless 1) there's only one cotangent (not counting the NO_FIELDS) or 2) sometimes for scalar functions (e.g. those made with @scalar_rule). I personally think modifying (1) to be "there's only one cotangent that a user could reasonably want" makes perfect sense.
There was a problem hiding this comment.
@oxinabox do you agree? If so I'll open a PR to the docs to update guidance.
Co-authored-by: willtebbutt <[email protected]>
|
@sethaxen please feel free to merge this whenever. It's good to go IMO. |
sethaxen
changed the title
uplo argument for Symmetric/ add rrule for Hermitian
Fixes #178 by porting the rules from Zygote, along with the
Hermitianrules (see https://github.com/FluxML/Zygote.jl/blob/0b3e32d5c0f8e5ef3b061a6f84ed5505f5a202a0/src/lib/array.jl#L454-L491).Tests currently do not pass I think because ChainRulesTestUtils seems to interpret the
:Uvariable as a variable of nameU, which does not exist. We may also need FiniteDifference v0.10.0 for the complex tests.