Resolve test_approx ambiguities
#253
Conversation
test/check_result.jl
Outdated
| test_approx( | ||
| Tangent{Tuple{Float64,Float64}}(ZeroTangent(), NoTangent()), | ||
| NoTangent(), | ||
| ) | ||
| test_approx( | ||
| NoTangent(), | ||
| Tangent{Tuple{Float64,Float64}}(ZeroTangent(), NoTangent()), | ||
| ) |
There was a problem hiding this comment.
Do we want this? I think we do
There was a problem hiding this comment.
I don't really think we do. Why do we?
I guess the argument can be made that something which is partially ZeroTangent and partially NoTangent is effectively net NoTangent
This definately works and makes sense for addition. If perturbing an input is an error along one code-path but fine in the other it is an error.
Then overall perturbing it is an error.
julia> NoTangent() + ZeroTangent()
NoTangent()But I am not sure how to think about it for a tuple
There was a problem hiding this comment.
In practice I think what matters is that NoTangent() is the same as Tangent of just NoTangent()s.
E.g. the choice similar to the one we made in
julia> rand_tangent((1, 2))
NoTangent()I added on the ZeroTangent on purely practical grounds, since they are effectively the same. But I can't justify it either, so we can push that decision down the road when we deal with automating testing a bit better, and restrict to NoTangent for now.
Does that sound good to merge?
test/check_result.jl
Outdated
| test_approx( | ||
| Tangent{Tuple{Float64,Float64}}(ZeroTangent(), NoTangent()), | ||
| NoTangent(), | ||
| ) | ||
| test_approx( | ||
| NoTangent(), | ||
| Tangent{Tuple{Float64,Float64}}(ZeroTangent(), NoTangent()), | ||
| ) |
There was a problem hiding this comment.
I don't really think we do. Why do we?
I guess the argument can be made that something which is partially ZeroTangent and partially NoTangent is effectively net NoTangent
This definately works and makes sense for addition. If perturbing an input is an error along one code-path but fine in the other it is an error.
Then overall perturbing it is an error.
julia> NoTangent() + ZeroTangent()
NoTangent()But I am not sure how to think about it for a tuple
|
Fixes the example from #229 too: |
* getindex for tuples * sum for tuples * repeated indices in getindex, etc * add colon case * tidy, use Tanget * first, tail * simplify * Apply 2 suggestions Co-authored-by: Frames Catherine White <oxinabox@ucc.asn.au> * skip a test until JuliaDiff/ChainRulesTestUtils.jl#253, bump version * comment on Zygote Co-authored-by: Frames Catherine White <oxinabox@ucc.asn.au>
Co-authored-by: Frames Catherine White <lyndon.white@invenialabs.co.uk>
Codecov Report
@@ Coverage Diff @@
## main #253 +/- ##
=======================================
Coverage 93.41% 93.41%
=======================================
Files 12 12
Lines 334 334
=======================================
Hits 312 312
Misses 22 22
Continue to review full report at Codecov.
|
Closes #252