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.

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

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

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

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 :/

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.

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

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.

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.

Shall we merge this, before it goes stale again?

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.