Conversation
Codecov Report
@@ Coverage Diff @@
## main #567 +/- ##
==========================================
+ Coverage 93.14% 93.15% +0.01%
==========================================
Files 15 15
Lines 875 891 +16
==========================================
+ Hits 815 830 +15
- Misses 60 61 +1
Continue to review full report at Codecov.
|
| @test getproperty(Tangent{Tuple{Float64}}(2.0), 1) == 2.0 | ||
| @test getproperty(Tangent{Tuple{Float64}}(@thunk 2.0^2), 1) == 4.0 | ||
|
|
||
| tang3 = Tangent{Tuple{Float64, String, Vector{Float64}}}(1.0, NoTangent(), @thunk [3.0] .+ 4) |
There was a problem hiding this comment.
Should we test this behaviour explicitly?
julia> tang1 = Tangent{Tuple{Float64}}(1.0)
Tangent{Tuple{Float64}}(1.0,)
julia> Base.tail(tang1)
Tangent{Tuple{}}()There was a problem hiding this comment.
Maybe this should just return NoTangent()?
There was a problem hiding this comment.
Maybe, would be consistent with
julia> rand_tangent(())
NoTangent()
src/tangent_types/tangent.jl
Outdated
| Base.first(tangent::Tangent{P,T}) where {P,T<:Union{Tuple,NamedTuple}} = first(backing(canonicalize(tangent))) | ||
| Base.last(tangent::Tangent{P,T}) where {P,T<:Union{Tuple,NamedTuple}} = last(backing(canonicalize(tangent))) | ||
|
|
||
| Base.tail(tangent::Tangent{P}) where {P<:Tuple} = Tangent{_tailtype(P)}(Base.tail(backing(tangent))...) |
There was a problem hiding this comment.
Why is this limited to Tuples only, does NamedTuple not work in some way?
It's also not clear to me why tail is not defined on any primal P (as for first and last). I thought it is because the order is chosen when defining the Tangent, i.e.
julia> Tangent{Foo}(;a=1.0, b=2.0, c=3.0)
Tangent{Foo}(a = 1.0, b = 2.0, c = 3.0)
julia> Tangent{Foo}(;a=1.0,c=3.0, b=2.0)
Tangent{Foo}(a = 1.0, c = 3.0, b = 2.0)which was handled in last by adding canonicalize
There was a problem hiding this comment.
I don't know what this should do for Foo.
Tangent{Foo}(;a=1.0, b=2.0,) is very different to what this does. And very different to indexing; dx[1] does not produce Tangent{Foo}(;a=1.0). Tangents with wrong-order or wrong-length backing are an aberration, and nothing should deliberately produce them.
I didn't add NamedTuples just because they would need a more elaborate _tailtype.
There was a problem hiding this comment.
Now NamedTuples are added.
|
Thanks! Formatting and good to go :) |
3 participants
It would be nice if
Base.tail(::Tangent)worked. Converting to a Tuple isn't safe... I think for example thisfrule:https://github.com/JuliaDiff/ChainRules.jl/blob/main/src/rulesets/Base/indexing.jl#L102-L105
goes wrong if
ẋ isa NoTangent, which might be causing this failure:https://github.com/JuliaDiff/Diffractor.jl/runs/7351650418?check_suite_focus=true#step:6:252