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. Also closes #676, closes #677, closes #633, closes #653.

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

[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

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.72%. Comparing base (61e4dd4) to head (5a5dbb6).
⚠️ Report is 70 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #481      +/-   ##
==========================================
+ Coverage   86.71%   86.72%   +0.01%     
==========================================
  Files           9        9              
  Lines         903      904       +1     
==========================================
+ Hits          783      784       +1     
  Misses        120      120              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[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 FluxML/Zygote.jl#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]

Maybe I fixed the tests at last.

[mcabbott]

What version should this be?

[cossio]

If in doubt, would it be safer to just bump the version (as if breaking)? Are there any downsides to doing this?

[mcabbott]

The downside is that 290 packages will all have to update their bounds:

https://juliahub.com/ui/Packages/ForwardDiff/k0ETY/0.10.28?page=2

[KristofferC]

If this is done, it should be 1.0

[mcabbott]

If it is 1.0, then ideally any other changes which should be done at the same time. In #467 the only outstanding one is #463 (distinct types). But perhaps #572 (tags), #570 (simd) are worth thinking about.

[devmotion]

Maybe it would also be good to enable nan safe mode by default.

[mcabbott]

Previously it tested with a random global seed.

What was confusing is that sometimes this gives PRIMAL == PRIMAL2 (for integers), and sometimes not. It would be clearer to deliberately test both cases. But, for now, I just made it print the seed used, so that you can lock it to debug locally.

Fixing the seed completely would test only one case per version of Julia.

[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?

[devmotion]

I think this change is reasonable.

Should some of the unary definitions be changed as well? In particular I am thinking about isinteger which is contained in UNARY_PREDICATES and hence leads to the definiton

isinteger(d::Dual) = isinteger(value(d))

However, that means that not every dual d with isinteger(d) can be converted to an Integer since we error if not all(iszero, partials(d)):

ForwardDiff.jl/src/dual.jl

Lines 365 to 368 in 78c73af

	@inline function Base.Integer(d::Dual)
	all(iszero, partials(d)) || throw(InexactError(:Integer, Integer, d))
	Integer(value(d))
	end

I think the error is consistent with the modified definition of == in this PR since I guess we want d == Integer(d) whenever Integer(d) is defined/allowed. Hence probably isinteger should be changed?

[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.