Special case Inf and -Inf in truncated

[mohamed82008]

This PR fixes a problem that showed up recently but I didn't track down exactly why it shows up now. Currently, the following code produces a NaN in the derivative of the log of the normalisation constant of a Truncated.

using ForwardDiff, Distributions

d = truncated(Normal(ForwardDiff.Dual(0.0, 1.0), ForwardDiff.Dual(1.0, 0.0)), 0, Inf)
d.logtp.partials[1] # NaN

Since the logpdf of a truncated subtracts logtp, the derivative is always going to be NaN at the end. This is clearly wrong and is only an artefact of how ForwardDiff/DiffRules behaviour is undefined for Inf values. For example, x.partials[1] is NaN in:

x = ForwardDiff.Dual(Inf, 0.0) / ForwardDiff.Dual(0.1, 0.0)

causing the zval, (Inf - d.μ) / d.σ, to give NaN in the derivative. This however seems to be the case even in previous versions of ForwardDiff and I haven't tracked down which change in Distributions.truncated made this ForwardDiff behaviour significant in the above example.

However, this PR feels like a half fix because:

1. It's not clear which change caused the above issue,
2. There are other similar functions that may need a special case for Inf and -Inf but I don't need them so I didn't fix their Dual support, and
3. It feels like ForwardDiff or DiffRules should be the place to fix this or special case Inf and -Inf but that will require a more significant effort.

That said, this PR gets the job done so I am opening it for now to get feedback.

[devmotion]

Hmm I think it is a bit suboptimal to have to work around such ForwardDiff specific issues since it makes the code more complex without any advantage (?) in other use cases. Does the problem occur also with the NaN-safe setting of ForwardDiff?

[mohamed82008]

Does the problem occur also with the NaN-safe setting of ForwardDiff?

Could you please elaborate on this?

[devmotion]

There's a NaN-safe mode: https://juliadiff.org/ForwardDiff.jl/dev/user/advanced/#Fixing-NaN/Inf-Issues It became a bit easier to use with Preferences (before one had to modify ForwardDiff; Chris even created a nansafe branch to make it a bit easier).

[mohamed82008]

Same behaviour with the NaN-safe mode.

[codecov-commenter]

Codecov Report

Merging #1467 (e878bb1) into master (93a542d) will decrease coverage by 0.00%.
The diff coverage is 80.00%.

@@            Coverage Diff             @@
##           master    #1467      +/-   ##
==========================================
- Coverage   84.44%   84.43%   -0.01%     
==========================================
  Files         124      124              
  Lines        7475     7480       +5     
==========================================
+ Hits         6312     6316       +4     
- Misses       1163     1164       +1     

Impacted Files	Coverage Δ
src/truncate.jl	85.18% <80.00%> (-0.35%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 93a542d...e878bb1. Read the comment docs.

[mohamed82008]

I generalised the fix here to work with any Truncated.

[devmotion]

Can you explain why this fixes the problem? It shouldn't change the value of loglcdf?

[mohamed82008]

It doesn't change its value but it changes the partial to 0 from NaN which gives the correct behaviour.

julia> using ForwardDiff

julia> d = ForwardDiff.Dual(1.0, 1.0)
Dual{Nothing}(1.0,1.0)

julia> convert(typeof(d), Inf)
Dual{Nothing}(Inf,0.0)

[mohamed82008]

bump

[devmotion]

I'm not happy with this PR as it is a quite strange workaround that only makes sense for ForwardDiff. At least it would be good to add some comments for readers about these checks, add tests for the bug that it fixes, and bump the version.

I'd like to check and see if there's a cleaner approach but it might take a few more days since I'm moving and don't have access to my computer right now.

[devmotion]

Same behaviour with the NaN-safe mode.

I can't reproduce this, NaN-safe mode fixes the issue for me (latest releases of both packages):

julia> using ForwardDiff, Distributions

julia> d = truncated(Normal(ForwardDiff.Dual(0.0, 1.0), ForwardDiff.Dual(1.0, 0.0)), 0, Inf)
Truncated(Normal{ForwardDiff.Dual{Nothing, Float64, 1}}(μ=Dual{Nothing}(0.0,1.0), σ=Dual{Nothing}(1.0,0.0)), range=(Dual{Nothing}(0.0,0.0), Dual{Nothing}(Inf,0.0)))

julia> d.logtp.partials[1]
0.7978845608028654

Maybe you forgot to restart Julia after setting the NaN-safe preference? You can check if it is enabled by running the example from the documentation:

julia> log(ForwardDiff.Dual{:tag}(0.0, 0.0))
Dual{:tag}(-Inf,0.0)

The partial is NaN if the NaN-safe mode is not enabled.

I'll close this PR since it is not needed and the problem is fixed by using NaN-safe mode.

[mohamed82008]

I think the NaN-safe mode is still a sub-optimal solution to the problem. You are basically taking a performance hit everywhere in the code base to avoid a problem that only happens with truncated distributions or in a few cases.

[mohamed82008]

I think the fix in this PR though sub-optimal in a different way still gives a better overall performance.

[devmotion]

I think the NaN-safe mode is still a sub-optimal solution to the problem. You are basically taking a performance hit everywhere in the code base to avoid a problem that only happens with truncated distributions or in a few cases.

It's unclear whether and how large a performance hit it actually causes. Hence it might even become the default at some point, see e.g. JuliaDiff/ForwardDiff.jl#451 (comment). It's really a ForwardDiff issue, and I agree with Andreas that the default setting is just wrong: JuliaDiff/ForwardDiff.jl#451 (comment)