Skip to content

Collapse zeros for Tuple & Ref tangents#565

Merged
mcabbott merged 3 commits intoJuliaDiff:mainfrom
mcabbott:collapse_zero
Sep 5, 2022
Merged

Collapse zeros for Tuple & Ref tangents#565
mcabbott merged 3 commits intoJuliaDiff:mainfrom
mcabbott:collapse_zero

Conversation

@mcabbott
Copy link
Member

@mcabbott mcabbott commented Jul 16, 2022

This is one way not to make a Tangent with only zero types in it:

julia> ProjectTo(Ref(1))(Ref(1))  # ok
Tangent{Base.RefValue{Int64}}(x = 1.0,)

julia> ProjectTo(Ref(1))(Ref(NoTangent()))  # could collapse to NoTangent()
Tangent{Base.RefValue{Int64}}(x = NoTangent(),)

You could go further, and make Tangent{Foo}() and Tangent{Foo}(x = NoTangent()) all return NoTangent. But perhaps it's odd to make a a type constructor not return that type... this PR does not do that.

@codecov-commenter
Copy link

codecov-commenter commented Jul 16, 2022

Codecov Report

Merging #565 (971eba9) into main (f2e3ac5) will decrease coverage by 0.09%.
The diff coverage is 88.88%.

@@            Coverage Diff             @@
##             main     #565      +/-   ##
==========================================
- Coverage   93.14%   93.04%   -0.10%     
==========================================
  Files          15       14       -1     
  Lines         875      877       +2     
==========================================
+ Hits          815      816       +1     
- Misses         60       61       +1     
Impacted Files Coverage Δ
src/projection.jl 96.98% <88.88%> (-0.33%) ⬇️
src/rule_definition_tools.jl 96.81% <0.00%> (-0.05%) ⬇️
src/ChainRulesCore.jl

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f2e3ac5...971eba9. Read the comment docs.

@mcabbott mcabbott added the Structural Tangent Related to the `Tangent` type for structured (composite) values label Aug 25, 2022
@mcabbott
Copy link
Member Author

mcabbott commented Sep 1, 2022

Xref discussion at #581, I guess that it would be slightly better for this to return a ZeroTangent than a NoTangent.

Copy link
Member

@mzgubic mzgubic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, just needs a version bump. I wonder whether we can abstract away the collapsing of the tangent somehow. Something like collapse(dy, whatever_it_is_if_not_NoTangent) which would internally check if it is an AbstractZero or a collection of AbstractZeros. Might be an overkill

@mcabbott
Copy link
Member Author

mcabbott commented Sep 5, 2022

The stronger way to abstract this would just be to make the Tangent constructor return a Zero. I don't quite see why an all-Zero Tangent is ever better.

@mcabbott mcabbott merged commit 07dfe60 into JuliaDiff:main Sep 5, 2022
@mcabbott mcabbott deleted the collapse_zero branch September 5, 2022 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Structural Tangent Related to the `Tangent` type for structured (composite) values

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants