Repository navigation
Conversation
Fixes #857. The previous behaviour remains available by setting the `nansafe_mode` preference to `false`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
In NaN-safe mode every partial was masked with `iszero(partial) ? zero(y) : y`. The mask only changes the result when the scalar factor is non-finite (for multiplication) or zero/NaN (for division), so check the factor once per tuple operation and only take the masked path in that case. `_isfinite_all` recurses into nested `Dual`s so Hessians also take the fast path. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Check your benchmarks with #859 perhaps |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #858 +/- ##
==========================================
+ Coverage 91.23% 91.42% +0.19%
==========================================
Files 11 11
Lines 1072 1096 +24
==========================================
+ Hits 978 1002 +24
Misses 94 94 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
I don't think we should merge this until something like #859 is in. I didn't fully get the objection to that PR, it was a very long response but feel free to modify as you see fit. |
|
I benchmarked #859 on more examples and studied a few other alternatives, but none of them was able to consistently outperform this PR here. Regarding #859 specifically, I think master/this PR is better since
|
|
As I mentioned in #857, my strong opinion is that the correct mode should be the default, regardless of its performance impact. Unsafe performance optimization must be an opt-in, similar to Any user can still opt into the faster but unsafe version by setting the preference. |
This is obviously false. You can test by putting a |
Who cares about "conceptually", though? The point is to get the result and to get it quickly.
This is from benchmarking a completely useless |
|
I disagree with both points. There's one thing we want to achieve: zero partials out for zero partials in. The approach on master clearly enforces this, and it's easy to check and validate for any reader. The approach in #859 is indirect and does not work in general, hence Moreover, on my computer I just couldn't see a consistent advantage of #859 in the benchmarks. Neither in microbenchmarks nor in full derivative/gradient calls. Based on my experiments, I just can't reproduce that it is clearly superior. On the other hand, what my experiments and study of some alternatives revealed was that the compiler much more consistently and reliably vectorizes the approach on master whereas the approach on #859 is compiled in a much less predictable way, even in the alternatives I tried. This seemed to explain also some of the performance regressions of #859. All in all, in my experiments #859 doesn't seem to be a clear winner but come with a few caveats and disadvantages. |
|
I put together a larger set of hopefully somewhat realistic comparisons of NaN-safe mode on master (which #858 enables by default) and in #859. I deliberately included cases where I expected #859 to be slower. All workloads are hand-written without dependencies besides ForwardDiff and use the public API with preallocated configs and default chunk sizes: Rosenbrock; logistic regression, Weibull survival regression, sigmoid Emax PK-PD model, MLP, polynomial and Michaelis-Menten least squares; ODE right-hand side Jacobians (ROBER, 2D Brusselator, 1D Fisher-KPP); gradients through RK4 solves of PK models; Newton's method for Kepler's equation; and the benchmarks from #858 and #859. ResultsTimes are relative to master with NaN-safe mode disabled; in the last column, < 1 means #859 is faster than master with NaN-safe mode enabled (with bootstrap 95% CI).
With NaN-safe mode disabled, #859 is within 0.98×–1.02× of master everywhere. Summary
Overall, #859 is not uniformly faster. It is clearly worthwhile for Hessians, but for first-order code the gains are modest, some realistic workloads regress by up to 1.3×, and whether #859 is faster or slower can flip just based on how the same model is written ( Setup and caveats
Benchmark script (
|
|
That being said, I want to repeat once more what I already wrote repeatedly: In my opinion this PR is a long overdue bug fix and hence the performance implications are a side effect but not relevant for whether the bug should be fixed or not and this PR should be merged or not. I can't count how often I ran into this correctness issue, in e.g. Turing, Distributions and Pumas. At least I know that I could try turning on the correct mode, in contrast to many users who don't. We're only having this discussion since for whatever reason what should have been a simple bugfix resolving a correctness issue ended up as a secondary non-default opt-in mode. |
Rename the masking helpers to `_strong_mul`/`_strong_div`, following ChainRulesCore's `strong_mul`. Plain `*`/`/` are now used only if both the partials' element type and the factor are known Base number types (or dual numbers thereof), the factor is finite for multiplication, and non-zero and non-NaN for division (`±Inf` no longer forces the masked path). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
NaN-safe mode is now the default, so the remaining preference is an opt-out. Rename it to `unsafe_mode` (set to `true` to disable NaN-safe mode) and rename `NANSAFE_MODE_ENABLED` to `UNSAFE_MODE_ENABLED`. The `nansafe_mode` preference is deprecated: it is still honoured if `unsafe_mode` is not set, but loading ForwardDiff with it set always emits a deprecation warning, even with `--depwarn=no`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Julia 1.10 hashes only the values of compile-time preferences, so `nansafe_mode = false` in the subprocess matched the cache compiled with `unsafe_mode = false` in CI. Setting `default_chunk_threshold` changes the hash without changing behaviour. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- Reframe the docs on disabling NaN-safe mode as a trade of correctness for performance, similar to `@fastmath`, and update the benchmark numbers - Merge `div_tuple_by_scalar` into `scale_tuple` - Simplify the default of the `unsafe_mode` preference - Treat `AbstractIrrational`s as strong zero types - Test division by dual numbers with non-finite partials, `_mul_partials` with non-finite dual factors, irrational factors and the deprecated `nansafe_mode = true` preference Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`_div_partials` multiplied by `inv(bval)` and divided by `bval * bval`, which overflow or underflow for tiny and huge `bval`. Then e.g. the derivative of `x -> 2x / x` was `-Inf` at `1e-200` and `2.0e-200` at `1e200`, and dividing zero by a dual number with tiny value yielded `NaN` partials. Following the quotient rule, `_div_partials` now divides by `bval`. Dividing each partial is slower than multiplying by `inv(bval)` (up to ~19% in division-heavy gradients and Hessians on Apple M2), but correct. Plain division of partials by a dual number now treats zero partials as strong zeros if its value is non-zero and not `NaN` and its partials are finite, so NaN-safe mode no longer masks every division by a dual number. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@KristofferC I assume the windows CI failure on 1.14.0-alpha1 is caused by an upstream issue, maybe Julia or LLVM |
|
Upstream issue: JuliaLang/julia#64013 |
Fixes #857.
This PR enables NaN-safe mode by default. With this PR:
(previously
[Inf, NaN]andDual{:tag}(-Inf,NaN)).I bumped the version to 1.5.0 since the default behaviour changes. Happy to revert that if you'd prefer a patch release.
unsafe_modepreferenceSince NaN-safe mode is now the default, the preference is renamed to make clear that turning it off trades correctness for speed: NaN-safe mode is disabled by setting the
unsafe_modepreference totrue(instead of settingnansafe_modetofalse). The internal constantNANSAFE_MODE_ENABLEDis renamed toUNSAFE_MODE_ENABLEDaccordingly, and the CI matrix setsunsafe_mode, so both modes remain tested.The
nansafe_modepreference is deprecated. Ifunsafe_modeis not set,nansafe_mode = falsestill disables NaN-safe mode. Ifnansafe_modeis set at all, loading ForwardDiff emits a deprecation warning withforce = true, i.e. even with--depwarn=no:Note that with
--depwarn=errorthis warning becomes an error, so loading ForwardDiff fails until the old preference is removed.The documentation section is rewritten: disabling NaN-safe mode is described as trading correctness for performance, similar to
@fastmath, and the outdated "~5%-10%" claim is removed since the cost depends on the function and the architecture (see the numbers below). The deprecated preference is mentioned in acompatadmonition.Reducing the cost of NaN-safe mode
This PR includes #859 (Kristoffer's commit is kept as is), which checks the scalar factor once instead of masking every partial, plus follow-ups that make its fast path stricter:
strong_mul(Avoid NaN-propagation in scalar rules ChainRulesCore.jl#551), the masking helpers are called_strong_mul/_strong_div._mul_is_strong(V, x)/_div_is_strong(V, x)decide whether plain*//already treat zero partials of typeVas strong zeros. With NaN-safe mode disabled they are constanttrue, so nothing changes there.*//is only used if both the partials' element type and the factor are known Base number types (IEEEFloat,BitInteger,Bool,BigInt,BigFloat,Rationals of these,AbstractIrrational) or dual numbers of those. Any otherRealtype always takes the masked path.*, the factor must be finite (in all components)./, the divisor must be non-zero and notNaN; for dual numbers, its value must satisfy this (recursively) and its partials must be finite.±Infdoes not force the masked path._mul_partialskeeps Reduce the cost of NaN-safe mode by checking the scalar factor once #859's single check for both factors: choosing plain or masked separately for each factor made the firsthessiancall compile 2–3× longer, for at most 4–6% runtime.Division of dual numbers
_div_partials(used byDual / Dual) multiplied byinv(bval)and divided bybval * bval, which overflow or underflow for tiny and hugebval. For example, on master the derivative ofx -> 2x / xis-Infat1e-200and2.0e-200at1e200, and dividing a dual zero by a dual number with value1e-200yieldsNaNpartials (which would also break the fast path for dual divisors above). Following the quotient rule,_div_partialsnow computes(a - (aval / bval) * b) / bval, avoidinginvasDual / RealandReal / Dualalready do. Dividing each partial is slower than multiplying byinv(bval)in division-heavy code (see below), but correct.Timings
Minimum times (Chairmarks
@be,seconds=3), each variant in a fresh process, two alternating runs. "NaN-safe disabled" is this PR withunsafe_mode = true; "master" and "#859" are withnansafe_mode = true:gradient(rosenbrock, rand(1000))hessian(rosenbrock, rand(10))hessian(rosenbrock, rand(20))hessian(logreg_nll, randn(8)), 500 × 8gradient(x -> sum(exp, x), rand(100))jacobian(cumprod, rand(10))*derivative(x -> sin(x) * exp(x) / x, 1.3)* Measured on its own in a fresh process. Within the full script below, this PR consistently takes 733–761 ns for it (#859: 565–587 ns), i.e. it depends on the benchmarks that ran before.
Division change, NaN-safe mode enabled, with
ratios(x) = sum(x[i] / x[i+1] for i in 1:length(x)-1)androsen_div(x) = sum((1 - x[i])^2 / (1 + x[i+1]^2) for i in 1:length(x)-1)onrand(n) .+ 0.5:inv(bval),aval / (bval * bval)(a - (aval / bval) * b) / bvalgradient(rosen_div, 1000)hessian(rosen_div, 20)hessian(ratios, 10)gradient(ratios, 1000)derivative(x -> x / (1 + x^2), 1.3)Benchmark script
The division benchmark uses the same setup with
ratiosandrosen_divas defined above, onrand(n) .+ 0.5.versioninfo()Tests
test/PreferencesTest.jlchecks the deprecated preference in subprocesses (resulting mode and warning text).test/PartialsTest.jlchecks strong zeros for unknown number types, non-finite dual factors and divisors, and tiny nested divisors;test/DerivativeTest.jlchecks division by tiny and huge numbers.Pkg.test()passes locally both with the new default (9784 tests) and withunsafe_mode = true(9664 tests).🤖 Generated with Claude Code