JuliaDiff / ForwardDiff.jl

Forward Mode Automatic Differentiation for Julia
Other
902 stars 146 forks source link

Modification of equality checking between two Duals not synchronized between master and last release #726

Closed clguillot closed 7 hours ago

clguillot commented 8 hours ago

Hi everyone,

It seems that the modifications required by issue #481, which should have been in the code starting from release 0.10.33 (according to the release notes) are indeed included in the branch master, but are not present in the last release 0.10.38. In particular, the following https://github.com/JuliaDiff/ForwardDiff.jl/blob/0a35a807e9a39a37d99d9928ea7d61247d8e2882/src/dual.jl#L392-L401 should have been replaced for the operator == by https://github.com/JuliaDiff/ForwardDiff.jl/blob/3acc8a64088564deec85103ed2dcc7cf6c0c2406/src/dual.jl#L391-L407 As a consequence of this modification, equality between 2 Dual with the same tags should check both the equality of the value and the partials, hence avoiding some branching that would have been taken otherwise. In particular, without this fix, we obtain

julia> f(x) = exp(complex(0, x))
f (generic function with 1 method)

julia> df(x) = ForwardDiff.derivative(f, x)
df (generic function with 1 method)

julia> d2f(x) = ForwardDiff.derivative(df, x)
d2f (generic function with 1 method)

julia> d2f(0.0)
0.0 + 0.0im

which is obviously wrong, but

julia> d2f(1e-15)
-1.0 - 1.0e-15im

which is right. This can be explained by looking at the code of exp in complex.jl

function exp(z::Complex)
    zr, zi = reim(z)
    if isnan(zr)
        Complex(zr, zi==0 ? zi : zr)
    elseif !isfinite(zi)
        if zr == Inf
            Complex(-zr, oftype(zr,NaN))
        elseif zr == -Inf
            Complex(-zero(zr), copysign(zero(zi), zi))
        else
            Complex(oftype(zr,NaN), oftype(zi,NaN))
        end
    else
        er = exp(zr)
        if iszero(zi)
            Complex(er, zi)
        else
            s, c = sincos(zi)
            Complex(er * c, er * s)
        end
    end
end

Here, the branching if iszero(zi) is taken because iszero will only check that value(zi) is zero when it should also check that partials(zi) is zero (which won't be the case in this example).

Is there a reason why the modification that should have been included from release 0.10.33 onward can be found in the master branch but not in the last release ?

mcabbott commented 8 hours ago

should have been in the code starting from release 0.10.33 (according to the release notes)

I think that by release note you mean this:

https://github.com/JuliaDiff/ForwardDiff.jl/releases/tag/v0.10.33

Note the explanation that this release was yanked, after too many complaints that #481 should perhaps not be viewed as a bugfix.

After that, master was marked 0.11-DEV, and a new branch for further 0.10 releases was made. Github's release notes don't know about branches -- they just list issues closed & PRs merged chronologically between one tag and the next.

There have been no 0.11 releases. https://github.com/JuliaDiff/ForwardDiff.jl/issues/719 is the current discussion about releasing master as 1.0 instead.