Fix and test promote_rule definitions
#207
Conversation
| @@ -24,7 +24,7 @@ using ChainRulesCore | |||
| # Not all operations will be valid over all of these types, but that's okay; such cases | |||
| # will simply error when they hit the original operation in the overloaded definition. | |||
| const ARRAY_TYPES = (:AbstractArray, :AbstractVector, :AbstractMatrix, :Array, :Vector, :Matrix) | |||
| const REAL_TYPES = (:Bool, :Integer, :(Irrational{:e}), :(Irrational{:π}), :Rational, :BigFloat, :BigInt, :AbstractFloat, :Real, :Dual) | |||
| const REAL_TYPES = (:Bool, :Integer, :(Irrational{:ℯ}), :(Irrational{:π}), :Rational, :BigFloat, :BigInt, :AbstractFloat, :Real, :Dual) | |||
There was a problem hiding this comment.
This was a bug it seems:
julia> typeof(ℯ)
Irrational{:ℯ}
julia> typeof(MathConstants.e)
Irrational{:ℯ}
There was a problem hiding this comment.
Was it never tested? Would be nice to add a test.
There was a problem hiding this comment.
I already added a test (I only noticed the bug since it failed without the fix).
| @@ -270,10 +270,22 @@ Base.convert(::Type{T}, t::T) where {T<:TrackedReal} = t | |||
| Base.convert(::Type{T}, t::T) where {T<:TrackedArray} = t | |||
|
|
|||
| for R in REAL_TYPES | |||
| @eval Base.promote_rule(::Type{$R}, ::Type{TrackedReal{V,D,O}}) where {V,D,O} = TrackedReal{promote_type($R,V),D,O} | |||
| R === :Dual && continue # ForwardDiff.Dual is handled below | |||
There was a problem hiding this comment.
Only defining promote_rule for Type{<:Dual} leads to method ambiguity issues; hence it is skipped here and defined below for Type{Dual{T,V,N}}.
| end | ||
|
|
||
| Base.promote_rule(::Type{R}, ::Type{TrackedReal{V,D,O}}) where {R<:Real,V,D,O} = TrackedReal{promote_type(R,V),D,O} |
There was a problem hiding this comment.
This is covered by the loop above (:Real is part of REAL_TYPES).
|
It seems DiffTests 0.1.2 (recently released) broke the ReverseDiff tests 😢 Promotion tests passed. |
| R === :Dual && continue # ForwardDiff.Dual is handled below | ||
| @eval begin | ||
| if isconcretetype($R) # issue ForwardDiff#322 | ||
| Base.promote_rule(::Type{TrackedReal{V,D,O}}, ::Type{$R}) where {V,D,O} = TrackedReal{promote_type(V,$R),D,O} |
There was a problem hiding this comment.
Since we are implementing both methods, do we know if they are all covered by tests? Might be worth adding a test checking that switching arguments gives the same result.
There was a problem hiding this comment.
Both are tested since promote_type calls both promote_rule definitions.
There was a problem hiding this comment.
I should add that actually tests fail when one of the definitions is removed due to eg stackoverflow errors when calling promote_type as mentioned in the OP.
|
I left some comments. It would also be nice to run some downstream tests to make sure this doesn't break anything. DiffEqSensitivity, DistributionsAD and GalacticOptim would be good to test. |
I ran tests of SciMLSensitivity, DistributionsAD (with `ENV["AD"] = "ReverseDiff"), and Optimization locally. Optimization and DistributionsAD succeeded, SciMLSensitivity tests did not finish before my laptop ran out of battery. I can retry tomorrow. |
|
Took forever but tests passed with this PR in the end: 10702.904078 seconds (22.35 G allocations: 1.929 TiB, 8.78% gc time, 87.66% compilation time: 0% of which was recompilation)
Testing SciMLSensitivity tests passed@mohamed82008 is this good to go? All test failures seem unrelated (also exist on the master branch, due to the new DiffTests release and some changes in Julia base that also broke Tracker tests a while ago) but the |
|
Looks good. Let's wait for CI to pass before we merge this PR though. |
|
There's no response yet in DiffTests to my issue and PR, and I'm not sure when I will have time to dig more into other unrelated test errors on the master branch. So it might take a while until CI is fixed on the master branch. |
|
Can we rebase this one now that DiffTests is upper bounded? |
Codecov ReportBase: 82.19% // Head: 81.98% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #207 +/- ##
==========================================
- Coverage 82.19% 81.98% -0.22%
==========================================
Files 18 18
Lines 1528 1532 +4
==========================================
Hits 1256 1256
- Misses 272 276 +4
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
CI passed. |
This PR fixes the
promote_ruledefinitions by using basically the same implementation as ForwardDiff (https://github.com/JuliaDiff/ForwardDiff.jl/blob/6a6443b754b0fcfb4d671c9a3d01776df801f498/src/dual.jl#L440-L452).Currently, abstract types (such as
Real,AbstractFloatetc.) are not handled properly aspromote_ruleshould be defined forType{<:T}for these types instead ofType{T}. Moreover, currently only one order of arguments is defined which can lead to issues (usually stackoverflow errors when callingpromote_type: JuliaLang/julia#13193) if some other package (such as ForwardDiff) defines apromote_rulewith reversed order of arguments and different result.Fixes #206. Closes #203.