[RFC/WIP] don't propagate NaN/Inf if Partials are zero #179

jrevels · 2016-12-15T17:32:04Z

This aims to fix #178, and any related issues with other functions.

I am not totally sure this does the right thing for nested Dual numbers; perhaps the perturbation check should be recursive?

There is a slight performance penalty here which is more or less significant depending on the function. I'm also interested to see what it does to SIMD performance.

EDIT:Removed a snippet of an alternate idea that I've now decided against.

jrevels · 2016-12-22T16:19:52Z

The approach I ended up going with here is to make Partials "NaN/Inf-safe".

Unfortunately, I'm seeing about performance regressions of ~10% on this branch vs. master when benchmarking some DiffBase functions, and that's probably going to be near the lower bound on such regressions for this change - I'm only benchmarking cases where it only needs to do the isnan/isinf/zero checks, i.e. not even checking the whole Partials.

Maybe it would be better to create a module-level flag for enabling this change so that users can opt-in only if they need it, preserving performance for users who don't need it. cc @mlubin @KristofferC for your opinions.

mlubin · 2016-12-22T17:50:52Z

src/partials.jl

+#----------------------#
+
+@inline function @compat(Base.:*)(partials::Partials, x::Real)
+    x = ifelse((isnan(x) || isinf(x)) && iszero(partials), one(x), x)


!isfinite could be better than isnan || isinf

KristofferC · 2016-12-22T18:04:36Z

10% seems quite a hefty penalty in my opinion.

One way could be to define most operations for an AbstractDual and then have two concrete types where the NaN behaviour is dispatched on. I don't really like this way though. Feels a bit too intrusive.

… checking isnan or isinf

jrevels · 2016-12-23T16:38:14Z

I don't really like this way though.

"This way" referring to this PR (and the proposed "users switch this on/off via a module-level constant") or "this way" referring to your NaN-safe type idea?

I do think the performance hit is too great here to make this the default behavior. The most convenient API for this, IMO, would be adding something like a nansafe flag to the config types so that users could easily enable this behavior when they need it. Having this be a runtime switchable thing would probably mean implementing something close to what you mentioned (though I might just go with adding a Bool type parameter to Partials to flip it on/off). That could be done in a separate PR, though, and we could just provide the module-level option for now.

jrevels · 2016-12-23T16:57:57Z

Any performance benefit?

From switching to !isfinite(x)? Yeah, thanks! It seems to improve performance by around ~3%-5% on the DiffBase functions I'm looking at versus isnan(x) || isinf(x).

mlubin · 2016-12-23T17:45:42Z

The module-level option is problematic for packages that use ForwardDiff under the hood. What is JuMP supposed to do?

KristofferC · 2016-12-23T17:57:56Z

I thought that introducing a second concrete Dual number type is perhaps a quite large change to solve this problem (at least compared to the solution you have now). A 5% performance penalty might be acceptable to skip the headache of two concrete types? I haven't really run into this problem myself so I feel I can't really weigh options against eachother in an informed way.

jrevels · 2016-12-23T23:49:37Z

The module-level option is problematic for packages that use ForwardDiff under the hood. What is JuMP supposed to do?

Really good point. It's definitely bothersome that whatever module-level option the user sets would apply to all downstream module usage, when they might instead want to only apply it selectively (e.g. have NaN-safe mode enabled for their JuMP model but disabled for usage in Optim). For now, however, it'd still an improvement over not having this option at all.

In a future PR, we could add AbstractConfig constructors like GradientConfig(x, Chunk{N}, NaNSafe{true}) (OT: I really wish something like JuliaLang/julia#16580 would land in v0.6, but that's really unlikely...fingers crossed for v1.0). The generated dual numbers would then contain partials of type Partials{N,T,true}, which would dispatch to the NaN-safe versions of methods. Downstream modules could then support passing that option through their APIs (or simply setting it to a reasonable default for themselves internally).

tkelman · 2017-01-25T04:19:16Z

.travis.yml

    - if (julia -e 'VERSION < v"0.5" && exit(1)'); then
        julia -e 'include(joinpath(JULIA_HOME, Base.DATAROOTDIR, "julia", "build_sysimg.jl")); build_sysimg(force=true)';
-        julia -e 'Pkg.clone(pwd()); Pkg.build("ForwardDiff"); Pkg.test("ForwardDiff"; coverage=true)';
+        julia -e 'Pkg.clone(pwd()); Pkg.build("ForwardDiff"); Pkg.checkout("DiffBase"); Pkg.test("ForwardDiff"; coverage=true)';


Do you still need to do this? These kind of things should be very temporary, otherwise you're not testing against the same versions of things your users will have installed.

DiffBase serves as a repository of test functions. If I didn't do this, I'd have to re-tag DiffBase every time I wanted to add a new shared test function, which would get really annoying.

I share your concern though. I've been meaning to factor the shared test suite out of DiffBase and into an unregistered package (DiffTestSuite or something). Then DiffTestSuite could be a test-only dependency that I Pkg.checkout every time, and I could go back to CI-testing DiffBase's functionality with the proper versions. That's what I probably should've done this in the first place...I'll set it up tomorrow.

Pkg.add("ForwardDiff"); Pkg.test("ForwardDiff") should pass. If that requires tagging more often, tag more often. Pkg.test shouldn't depend on unregistered packages.

Same dependency versioning issue applies as everywhere else - if it's needed to pass tests and doesn't exist in every version of DiffBase, then at least test/REQUIRE needs to have a corresponding minimum version requirement.

Fair enough - I'll fix this to use tagged DiffBase versions and start tagging DiffBase more often, then.

I don't mind if you want to run additional tests on CI-only, or have things temporarily checked out to branches on CI if that's necessary while things are being developed -- as long as you keep in mind how that configuration will differ from what users will have installed, and that Pkg.test should work for them on releases too.

Yeah, no worries! My future workflow will be to keep master branch's CI running against tagged dependency versions for the reasons you mentioned. I can always toggle a PR's CI config to use whatever DiffBase branch I want during development, and then before merging, I can tag the new DiffBase version (if necessary) and switch the config back to using only tagged versions.

andreasnoack · 2019-02-11T22:31:01Z

This is what I expect to see after flipping the NANSAFE_MODE_ENABLED flag

julia> Inf*ForwardDiff.Dual{Float64}(1.0,0.0)
Dual{Float64}(Inf,0.0)

but should I expect to see

julia> Inf*ForwardDiff.Dual{Float64}(1.0,0.0,1.0)
Dual{Float64}(Inf,NaN,Inf)

?

jrevels · 2019-02-12T19:54:18Z

Hmm. Yeah, it would probably be consistent for us to check/propagate all partials individually when NANSAFE_MODE_ENABLED is set to true. Might cause a bigger performance hit for this NANSAFE mode but maybe not, and it'd probably be worthwhile either way.

devmotion · 2026-02-11T20:27:17Z

This is what I expect to see after flipping the NANSAFE_MODE_ENABLED flag
julia> Inf*ForwardDiff.Dual{Float64}(1.0,0.0)
Dual{Float64}(Inf,0.0)
but should I expect to see
julia> Inf*ForwardDiff.Dual{Float64}(1.0,0.0,1.0)
Dual{Float64}(Inf,NaN,Inf)
?

For anyone coming across this PR - this was fixed by #777 in ForwardDiff >= 1.2.2. With the latest release of ForwardDiff (and NaN-safe mode enabled):

julia> Inf*ForwardDiff.Dual{Float64}(1.0,0.0)
Dual{Float64}(Inf,0.0)

julia> Inf*ForwardDiff.Dual{Float64}(1.0,0.0,1.0)
Dual{Float64}(Inf,0.0,Inf)

Might cause a bigger performance hit for this NANSAFE mode but maybe not,

It actually improved performance of NaN-safe mode 🙂

don't propagate NaN/Inf if Partials are zero

77e3736

jrevels force-pushed the jr/logfix branch from 0a6d242 to 77e3736 Compare December 21, 2016 23:16

attempt to fix Calculus.jl test failure on v0.4

5c51df7

jrevels force-pushed the jr/logfix branch from 5ad4154 to 5c51df7 Compare December 22, 2016 15:31

mlubin reviewed Dec 22, 2016

View reviewed changes

improve efficiency of NaN/Inf pruning by negating isfinite instead of…

b44a180

… checking isnan or isinf

add NaN-safe mode switch

0156387

jrevels mentioned this pull request Dec 26, 2016

dynamic NaN-safe mode switching #181

Open

add documentation for NaN-safe mode

b6cd27a

jrevels merged commit 253d305 into master Dec 26, 2016

jrevels deleted the jr/logfix branch December 26, 2016 17:23

tkelman reviewed Jan 25, 2017

View reviewed changes

jrevels mentioned this pull request Jan 25, 2017

test against tagged DiffBase versions instead of DiffBase master #185

Merged

fredrikekre mentioned this pull request Jan 5, 2018

ForwardDiff.gradient(x->log(1/max(1f-20,x[1])), zeros(Float32,1))) returns [NaN32] #287

Closed

andreasnoack mentioned this pull request Feb 11, 2019

Use NaNMath JuliaNLSolvers/LineSearches.jl#71

Merged

ajwheeler mentioned this pull request Nov 13, 2024

Tag v1.0 #719

Closed

[RFC/WIP] don't propagate NaN/Inf if Partials are zero #179

[RFC/WIP] don't propagate NaN/Inf if Partials are zero #179

Uh oh!

Conversation

jrevels commented Dec 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jrevels commented Dec 22, 2016

Uh oh!

mlubin Dec 22, 2016

Choose a reason for hiding this comment

Uh oh!

KristofferC commented Dec 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jrevels commented Dec 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jrevels commented Dec 23, 2016

Uh oh!

mlubin commented Dec 23, 2016

Uh oh!

KristofferC commented Dec 23, 2016

Uh oh!

jrevels commented Dec 23, 2016

Uh oh!

tkelman Jan 25, 2017

Choose a reason for hiding this comment

Uh oh!

jrevels Jan 25, 2017

Choose a reason for hiding this comment

Uh oh!

tkelman Jan 25, 2017

Choose a reason for hiding this comment

Uh oh!

tkelman Jan 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrevels Jan 25, 2017

Choose a reason for hiding this comment

Uh oh!

tkelman Jan 25, 2017

Choose a reason for hiding this comment

Uh oh!

jrevels Jan 25, 2017

Choose a reason for hiding this comment

Uh oh!

andreasnoack commented Feb 11, 2019

Uh oh!

jrevels commented Feb 12, 2019

Uh oh!

devmotion commented Feb 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jrevels commented Dec 15, 2016 •

edited

Loading

KristofferC commented Dec 22, 2016 •

edited

Loading

jrevels commented Dec 23, 2016 •

edited

Loading

tkelman Jan 25, 2017 •

edited

Loading