JuliaDiff / ReverseDiff.jl

Reverse Mode Automatic Differentiation for Julia
Other
348 stars 56 forks source link

DiffResults objects are not re-aliased properly #251

Open gdalle opened 6 months ago

gdalle commented 6 months ago

The DiffResults documentation clearly states that whenever a DiffResult object is used, it must be realiased at the end of the function call:

result = do_stuff!(result, args...)

See for instance the docstrings in https://juliadiff.org/DiffResults.jl/stable/#Mutating-a-DiffResult, or the issue DiffResults#17

This is not done by ReverseDiff, for instance here:

https://github.com/JuliaDiff/ReverseDiff.jl/blob/c982cde5494fc166965a9d04691f390d9e3073fd/src/api/gradients.jl#L21-L27

Am I right in deducing that it can lead to incorrectness?

gdalle commented 6 months ago

See also https://github.com/JuliaDiff/ForwardDiff.jl/issues/696

gdalle commented 6 months ago

Here's an MWE demonstrating the bug:

julia> using StaticArrays, ReverseDiff, DiffResults

julia> x = MVector{2}(3.0, 4.0);

julia> result_before = DiffResults.GradientResult(x)
ImmutableDiffResult(3.0, ([3.0, 4.0],))

julia> result_after = ReverseDiff.gradient!(result_before, sum, x)
ImmutableDiffResult(3.0, ([1.0, 1.0],))  # the first value should be 7 = sum(x)

julia> x = [3.0, 4.0];

julia> result_before = DiffResults.GradientResult(x)
MutableDiffResult(3.0, ([6.365987374e-314, 1.0e-323],))

julia> result_after = ReverseDiff.gradient!(result_before, sum, x)
MutableDiffResult(7.0, ([1.0, 1.0],))  # the first value is 7 = sum(x)