Change `==` to ignore measure-zero branches

[mcabbott]

This changes the meaning of == for dual numbers, to demand that both real and ~~imaginary~~ dual parts match.

Fixes #197, fixes #407. Edit -- also closes #490, closes #506, closes #536, closes #542, closes #551.

See #480 for too-long discussion of why I think this makes sense. And comments on functions like >, which this PR does not alter.

[mcabbott]

~~Test failure on master is that "3-element ForwardDiff.Partials{3, Int32}" now has a space in it, see #476.~~

~~Test failure on 1.0 might be my fault?~~

Edit:

Locally tests pass on 1.5 and on 1.0.

Initially 1.5 passed on CI.

Now 1.0 and 1.5 fail on CI, at the same line, seemingly relevant. The tests here are pretty messy to work with; I'm happy to have another go but won't bother if there's no interest.

[KristofferC]

I also think this makes sense. I am a little bit worried about the implications it will have though, perhaps there isn't a better way than to go ahead and try. Would it be considered a bugfix or a breaking change?

@jrevels, any thoughts here?

[codecov-commenter]

Codecov Report

Merging #481 (5a5dbb6) into master (61e4dd4) will decrease coverage by 0.37%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #481      +/-   ##
==========================================
- Coverage   86.71%   86.33%   -0.38%     
==========================================
  Files           9        9              
  Lines         903      900       -3     
==========================================
- Hits          783      777       -6     
- Misses        120      123       +3     

Impacted Files	Coverage Δ
src/prelude.jl	36.36% <ø> (ø)
src/dual.jl	78.99% <100.00%> (-0.78%)	:arrow_down:
src/partials.jl	83.47% <0.00%> (-0.87%)	:arrow_down:

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

[mcabbott]

Here's something this PR breaks:

julia> ForwardDiff.gradient(x -> sum(abs2, unique(x)), [1,2,1,2,3]) |> println
[2, 4, 0, 0, 6]  # before
[2, 4, 2, 4, 6]  # after

julia> hash(ForwardDiff.Dual(1,0)) === hash(ForwardDiff.Dual(1,2,3))
true  # unchanged

Maybe there is some way to avoid this? Or maybe that's not the wrong answer, if all parameters are infinitesimally perturbed?

julia> using FiniteDifferences

julia> grad(central_fdm(5, 1), x -> sum(abs2, unique(x)), Float32[1,2,1,2,3])
(Float32[1.9998622, 4.0002246, 1.9998622, 4.0002246, 6.000003],)

Xref https://github.com/FluxML/Zygote.jl/issues/1053

[stelmo]

Chiming in here, this patch solved this issue for me that involved sparse arrays. I would offer to help, but unfortunately I just don't know enough about auto diff :/

[mcabbott]

Latest commit proposes to make this a bugfix, since apparently it's what the authors of all the above issues expected, and making a breaking release sounds like it'll take another year.

~~However, tests have already rotted, probably due to something in https://github.com/JuliaDiff/ForwardDiff.jl/pull/577/files.~~ Now fixed.

[stelmo]

Hi there, will this make it into master before 1.0?

[mcabbott]

The integer complaint is this, which is unchanged by this PR:

julia> using ForwardDiff: Dual

julia> isinteger(Dual(1.0, 0.2))  # is this wrong?
true

julia> convert(Int, Dual(1.0, 0.2))
ERROR: InexactError: Int(Int64, Dual{Nothing}(1.0,0.2))

julia> convert(Int, Dual(3.0, 0.0))
3

I'm not too sure what the right policy is here, has this ever come up in the wild?

I don't think we should make quick arguments from apparent consistency. And I don't want to add anything more to this PR.

[devmotion]

I'm sorry, I think my comment was a bit unclear. The main reason for why I think one should consider changing isinteger (and eg. iseven and isodd) is not to fix that isinteger/Integer inconsistency. But rather that

• these functions create also measure-zero branches that this PR tries to fix for other functions such as ==,
• this PR introduces an inconsistency: Currently isinteger(x::Dual) is treated in the same way as the (slightly hypothetical) x == typemin(T) || x == (typemin(T) + 1) || ... || x == typemax(T) where T = typeof(Integer(zero(value(x)))), but with this PR they would be different. The first approach would neglect the partials whereas the second one would demand that they are zero.

Initially I was worried that these additional changes would be too far-fetched or possibly breaking. Only then I noticed the existing inconsistency with isinteger/Integer - which was somewhat relieving since I think it indicates that it is fine to change isinteger (and other functions) as the current implementation seems to have problems that would be fixed by such a change.

[mcabbott]

This PR is narrower than that, it fixes only == (and isequal) not all possible measure-zero branches. To start somewhere, and because equality is what seem to cause problems in the wild.

It's possible that other changes should be considered, in follow-ups. _isinteger(x) = x == trunc(Int, x) is a plausible definition, which at present disagrees with isinteger. #480 also mentions >, which this PR does not touch, allowing this:

julia> x = ForwardDiff.Dual(1.0, 0.2)

julia> x > 1
false

julia> x < 1
false

julia> x == 1  # this PR
false

[mcabbott]

Shall we merge this, before it goes stale again?

[mcabbott]

Thanks!

Test failures on master appear to be small changes in random numbers? This (copied from CI logs) would pass with slightly larger tolerance:

julia> @test [1.7010290555795968e-6, 1.063143159737248e-6, 8.505145277897984e-7] ≈ [1.7010237080913283e-6, 1.0631398175570802e-6, 8.505118540456641e-7] rtol=1.0e-5
Test Passed

julia> @test [1.7010290555795968e-6, 1.063143159737248e-6, 8.505145277897984e-7] ≈ [1.7010237080913283e-6, 1.0631398175570802e-6, 8.505118540456641e-7] rtol=1.0e-6
Test Failed 

Not clear why. CI ran this PR on 1.8 this morning, and it passed.