ChainRules.jl icon indicating copy to clipboard operation
ChainRules.jl copied to clipboard

Add Integration Test for ReversePropagation.jl

Open oxinabox opened this issue 4 years ago • 4 comments

@dpsanders has just released ReversePropagation.jl which is a tape-to-source scalar reverse mode AD using ChainRules. We have reverse depenency testing setup for Zygote, to make certain no changes to ChainRules break that. We should do the same thing for ReservePropagation.jl

This is blocked until ReversePropagation.jl is been registered.

oxinabox avatar Dec 03 '20 14:12 oxinabox

It's quite possible that ReversePropagation.jl will be integrated into ModelingToolkit.jl at some point.

dpsanders avatar Dec 03 '20 16:12 dpsanders

Maybe this can be closed since this test is running?

However, it seems to fail on every run, e.g. recently here: https://github.com/JuliaDiff/ChainRules.jl/runs/4903197151?check_suite_focus=true

518
  Got exception outside of a @test
519
  MethodError: rev(::typeof(+), ::Num, ::Num, ::Num) is ambiguous. Candidates:
520
    rev(arg1, arg2::Num, arg3::Num, arg4::Num) in ReversePropagation at /home/runner/.julia/packages/Symbolics/HDE84/src/register.jl:51
521
...
526
    rev(arg1, arg2::Num, arg3::Real, arg4::Real) in ReversePropagation at /home/runner/.julia/packages/Symbolics/HDE84/src/register.jl:51
527
    rev(::typeof(+), z::Real, x::Real, y::Real) in ReversePropagation at /home/runner/work/ChainRules.jl/ChainRules.jl/downstream/src/reverse_icp.jl:62
528
  Possible fix, define
529
    rev(::typeof(+), ::Num, ::Num, ::Num)
530
  Stacktrace:
531
    [1] rev(eq::SymbolicUtils.Code.Assignment, params::Vector{Any})
532
      @ ReversePropagation ~/work/ChainRules.jl/ChainRules.jl/downstream/src/reverse_icp.jl:33
533
    [2] _broadcast_getindex_evalf
534
      @ ./broadcast.jl:670 [inlined]
...
551
   [11] forward_backward_code(ex::Num, vars::Vector{Num}, params::Vector{Any})
552
      @ ReversePropagation ~/work/ChainRules.jl/ChainRules.jl/downstream/src/reverse_icp.jl:97
      ```

mcabbott avatar Jan 23 '22 16:01 mcabbott

Something in Symbolics.jl broke this, I believe in the transition from version 3 to 4 of that package. Ref: https://github.com/dpsanders/ReversePropagation.jl/issues/64

That issue contains a fix (?) but requires some work which I haven't had a chance to do.

dpsanders avatar Jan 24 '22 03:01 dpsanders

I think I will disable this integration test and reopen this issue.

oxinabox avatar Jan 24 '22 11:01 oxinabox