fix division return type #233
Conversation
They are defined in ChainRulesTestUtils.jl. (This is a separate package to keep dependencies separate)
No, I think they only test via comparing to Finite Differencing |
|
Yeah, and @CarloLucibello in terms of testing this rule, the correctness tests should be fine as is, and it would be sufficient for you to add some basic regression tests on the type of the differential. Could you also please open an issue on |
test/rulesets/Base/fastmath_able.jl
Outdated
| x, Δx, x̄ = 10rand(T, 3) | ||
| y, Δy, ȳ = rand(T, 3) | ||
| Δz = randn(typeof(f(x, y))) | ||
|
|
||
| frule_test(f, (x, Δx), (y, Δy)) | ||
| rrule_test(f, Δz, (x, x̄), (y, ȳ)) | ||
| if T != Float32 |
There was a problem hiding this comment.
had to exclude this case because test tolerance is too strict for Float32
There was a problem hiding this comment.
Could you decrease the tolerance using the keyword arguments of the testers?
e.g. something like frule_test(f, (x, Δx), (y, Δy); atol=0, rtol = sqrt(eps(T)))
|
I can't find a rule for |
Here it is: https://github.com/JuliaDiff/ChainRules.jl/blob/master/src/rulesets/Base/base.jl#L141 |
src/rulesets/Base/fastmath_able.jl
Outdated
| ) where {T<:Union{Real,Complex}} | ||
| x::T1, | ||
| y::T2, | ||
| ) where {T1<:Union{Real,Complex}, T2<:Union{Real,Complex}} |
There was a problem hiding this comment.
Is this signature just changed so that the tests in the loop pass? If so, maybe it's better to just take that test out of the loop. The signature we had here is the one that is FastMath compatible. There is a hypot that takes mixed arguments, but it promotes before calling this one, so currently we would just let the AD handle that. Same with the rrule.
test/rulesets/Base/fastmath_able.jl
Outdated
| x, Δx, x̄ = 10rand(T, 3) | ||
| y, Δy, ȳ = rand(T, 3) | ||
| Δz = randn(typeof(f(x, y))) | ||
|
|
||
| frule_test(f, (x, Δx), (y, Δy)) | ||
| rrule_test(f, Δz, (x, x̄), (y, ȳ)) | ||
| if T != Float32 |
There was a problem hiding this comment.
Could you decrease the tolerance using the keyword arguments of the testers?
e.g. something like frule_test(f, (x, Δx), (y, Δy); atol=0, rtol = sqrt(eps(T)))
|
zygote failure is unrelated |
|
again Zygote failure unrelated. This should be ready to go |
|
merge and tag? |
|
Please. Thanks for the contribution |
Fix FluxML/Zygote.jl#727.
Still have to add tests. Where are
rrule_test,frule_testandscalar_testdefined? Do they check types?