This PR fixes the promote_rule definitions 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, AbstractFloat etc.) are not handled properly as promote_rule should be defined for Type{<:T} for these types instead of Type{T}. Moreover, currently only one order of arguments is defined which can lead to issues (usually stackoverflow errors when calling promote_type: https://github.com/JuliaLang/julia/issues/13193) if some other package (such as ForwardDiff) defines a promote_rule with reversed order of arguments and different result.

Fixes #206. Closes https://github.com/JuliaDiff/ReverseDiff.jl/pull/203.

Sep 24 '22 21:09 devmotion

It seems DiffTests 0.1.2 (recently released) broke the ReverseDiff tests :cry: Promotion tests passed.

Sep 24 '22 21:09 devmotion

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.

Sep 25 '22 00:09 mohdibntarek

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.

Sep 25 '22 21:09 devmotion

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 promote_type tests succeed and downstream tests pass.

Sep 26 '22 18:09 devmotion

Looks good. Let's wait for CI to pass before we merge this PR though.

Sep 27 '22 00:09 mohdibntarek

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.

Oct 01 '22 00:10 devmotion

Can we rebase this one now that DiffTests is upper bounded?

Oct 03 '22 09:10 mohdibntarek

Codecov Report

Base: 82.19% // Head: 81.98% // Decreases project coverage by -0.21% :warning:

Coverage data is based on head (287de26) compared to base (ac44511). Patch coverage: 0.00% of modified lines in pull request are covered.

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

Impacted Files	Coverage Δ
src/ReverseDiff.jl	`100.00% <ø> (ø)`
src/tracked.jl	`86.81% <0.00%> (-1.61%)`	:arrow_down:

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

Oct 03 '22 10:10 codecov-commenter

CI passed.

Oct 03 '22 10:10 devmotion

Fix and test `promote_rule` definitions

Codecov Report