Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add getproperty for Tangent{<:Cholesky} #641

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rofinn
Copy link
Contributor

@rofinn rofinn commented Jun 29, 2022

Initial code for closing #640. I'm open to changing what it checks, but this seems like any easy way to minimize breaking changes for now.

@github-actions github-actions bot added the needs version bump Version needs to be incremented or set to -DEV in Project.toml label Jun 29, 2022
@devmotion
Copy link
Member

My intuition is that we would want getproperty(x::Tangent{<:Cholesky}, :U) == getproperty(x::Tangent{<:Cholesky}, :L)'. I think that's not satisfied by the PR?

I guess it might also be useful to define getproperty(x::Tangent{<:Cholesky}, :UL) (at least for completeness and to avoid ZeroTangents in that case as well).

@sethaxen
Copy link
Member

One of the challenges with this approach is that we can't capture the logic present in this rule:

∂F = if x === :U
if F.uplo === 'U'
C(factors=_maybeUpperTri(Ȳ),)
else
C(factors=_maybeLowerTri'),)
end
elseif x === :L
if F.uplo === 'L'
C(factors=_maybeLowerTri(Ȳ),)
else
C(factors=_maybeUpperTri'),)
end
end

Because we don't have access to the Cholesky object's uplo field, we don't know the relationship between factors, U, and L. e.g. if uplo=='L', then a user requesting ΔC.L should be given LowerTriangular(ΔC.factors), and a user requesting ΔC.U should be given LowerTriangular(ΔC.factors)', but the case is completely different if uplo=='U'. Currently this rule will only work when Symbol(C.uplo) == sym. I'm not certain if/how this can be resolved, short of modifying Tangent to include metadata from the primal object.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs version bump Version needs to be incremented or set to -DEV in Project.toml
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants