Conversation
- Add magnitude-aware tolerance floor to prevent chasing floating-point granularity - Reduce max_iter to 50 (proper stopping rule should rarely need it) - Add clear warnings for non-convergence with fallback to bisection - Detect 2-cycles explicitly as sign of ill-conditioning
Delegates core root-finding logic in quantile_newton functions to Roots.find_zero using Newton(). Upgrades quantile_bisect to use the ITP() bracketing solver as suggested. Retains precision-based xrtol tolerance scaling.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2070 +/- ##
==========================================
+ Coverage 86.63% 86.74% +0.11%
==========================================
Files 149 149
Lines 8921 8882 -39
==========================================
- Hits 7729 7705 -24
+ Misses 1192 1177 -15 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Building on #2066, simplify the quantile solvers to rely on Roots' default tolerances instead of hand-tuned options: - `quantile_bisect` delegates to `find_zero(g, (lx, rx), ITP())`. ITP's relative `xrtol = eps` converges even for brackets far from zero, where the previous absolute `cbrt(eps)^2` tolerance fell below the floating-point spacing and looped forever (#1611, #1807). - The four `_newton` functions delegate to `find_zero((F, f), x, Newton(), ITP())`. Roots switches to the ITP bracketing method as soon as Newton brackets the root, which resolves the oscillation/stalling seen for extreme quantiles (#1571, #1898, #2061). The `tol`/`max_iters` parameters and the explicit tolerance floors are dropped: Roots' Newton defaults combine an absolute (`xatol = eps`) and a relative (`xrtol = eps`) x-tolerance, so they converge both near `p ≈ 1` and for roots near zero without bespoke floors. `quantile_bisect` keeps only the exact `rx == lx` degenerate guard (#1501); the approximate "collapsed bracket" handling is dropped, as a non-bracketing interval is a separate concern from the solver choice. Tests use `isapprox`'s type-appropriate defaults rather than fixed tolerances, and are split one regression per testset. Adds coverage for #1898 and a `Float32` round-trip. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
devmotion
added a commit
that referenced
this pull request
Jun 19, 2026
Implement the previously-missing (c)cdf for SkewNormal, now that StatsFuns
2.2 provides Owen's T function `owens_t`. The closed forms are
cdf(x) = Φ(z) - 2·T(z, α)
ccdf(x) = Φ̄(z) + 2·T(z, α), z = (x - ξ)/ω
with `logcdf`/`logccdf` using the generic `log(cdf)`/`log(ccdf)` fallbacks.
`pdf`/`logpdf` are refactored for consistency (destructure once, reuse `z`).
`quantile`/`cquantile` are added via Newton iteration from the mode, using
`quantile_newton`/`cquantile_newton`. This branch is stacked on #2070, whose
Roots-based solver handles the near-zero-root convergence that SkewNormal's
support (straddling 0) requires, so no change to `src/quantilealgs.jl` is
needed here. `invlogcdf`/`invlogccdf` are left to the generic
`quantile(d, exp(lp))` fallback: the log-space `invlogcdf_newton` overshoots
into the underflowed tail from the approximate mode `m_0` and returns NaN
there, whereas the generic fallback through `quantile_newton` stays robust.
Require StatsFuns 2.2 and bump the package version.
Testing: add a `sn`-based reference class (test/ref/continuous/skewnormal.R)
to the R reference framework, pin `sn` and its deps in renv.lock, and add the
generated entries to continuous_test.ref.json. The dedicated testset is
reduced to construction, support boundaries and the α=0 reduction to Normal.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
andreasnoack
approved these changes
Jun 25, 2026
Member
Author
|
Thank you @Cedriq1astaken! |
This was referenced Jun 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
First, thanks to @Cedriq1astaken for the work in #2066 — diagnosing the
quantile_newtoninfinite loop, introducingRoots.find_zero, and assembling the regression cases. This PR builds directly on that branch (its commits are preserved here) and pushes the idea a little further.I started looking into this more closely after hitting the same class of bug from a different direction in #2069 (adding
cdf/ccdf/quantiletoSkewNormal). There, the hand-rolled Newton iteration hangs whenever the root is near zero:SkewNormal's support straddles 0, and the relative-only stopping ruleabs(x - x0) > max(abs(x), abs(x0)) * tolshrinks below thecdf/pdfevaluation noise as the root approaches 0, so the iterates oscillate in their last bits and never converge. That's the same underlying issue as the extreme-tail oscillation in #2061, just triggered by the root's location rather than byp. Seeing two distinct symptoms of one root cause is what motivated converging on a single, options-free formulation.1) What changes compared to
masterThis adds a dependency on Roots v3 (compat
Roots = "3").mastersolves quantiles with hand-rolled Newton/bisection loops and hand-tuned tolerances. This replaces them withRoots.find_zero, relying on Roots' default tolerances plus a bracketing fallback — notol/max_iters/floor parameters:quantile_bisect→find_zero(x -> cdf(d, x) - p, (lx, rx), ITP())quantile_newton/cquantile_newton/invlogcdf_newton/invlogccdf_newton→find_zero((F, f), x, Newton(), ITP()). Passing a bracketing method afterNewton()makes Roots switch to ITP the moment the iteration brackets the root (a sign change ofF).Chernoffquantiles now route throughquantile_newton(with the sameNormal-based warm start) instead of three bespokenewton(...)branches. Those branches called anewtonfunction that was never defined anywhere in the package, soquantile(Chernoff(), τ)threwUndefVarErrorfor any non-tabulatedτ(e.g.truncated(Chernoff(); lower=0.1)) — present since the original Chernoff PR (Chernoff distribution #840). This removes the dead code and fixes Error with Truncated Chernoff quantiles; missing function (newton) #1999.The reason defaults suffice is that Roots' tolerances supply three distinct ingredients, each covering one of the failure modes:
quantile_newtonstalls/oscillates for a root near zeroNewton's absolutexatol = eps(replaces #2069's hand-addedeps^(3/4)floor)quantile_newtonoscillates in the extreme tail (p ≈ 1)quantile_bisectcan't terminate for a bracket far from zeroxrtol = eps(the old absolutecbrt(eps)^2fell below the float spacing)Roots' full defaults for reference:
Newton()ITP()/ bracketingxatoleps(T)eps(T)^3xrtoleps(T)eps(T)atol/rtol4·eps(S)0/0maxiters/maxevalsstrictfalsetrueI confirmed the mapping empirically: for
SkewNormal's near-zero roots,Newton()converges without the ITP fallback (the absolutexatoldoes the work); the fallback is what rescues the extreme-tail cases. Because Roots' defaults already cover the near-zero regime, #2069 no longer needs its ownquantile_newtonchange.Bracketing algorithm: standardized on
ITP— NonlinearSolve's documented recommendation, explicitly suited to smooth, well-behaved functions. CDF roots are simple (d(cdf)/dx = pdf > 0), so it's a natural fit. (For reference: Roots' robust default isBisection, its performance picks areA42/AlefeldPotraShi; the newerModABwas recently added to both packages but isn't yet a documented recommendation, and its strength — roots of higher multiplicity — doesn't apply here.)Fixes: #1571, #1611, #1807, #1898, #1999, #2061 (and removes the need for #2069's separate
quantile_newtonfix).2) What changes compared to #2066
#2066 also moves to
Roots.find_zero, but keeps hand-tuned machinery on top. Relative to that branch, this PR:tolandmax_itersparameters and themax(tol, cbrt(eps)^2)floors from all four_newtonfunctions — pure Roots defaults instead.xatol = toloverride inquantile_bisect— pure Roots defaults there too.isapprox-stylerx ≈ lx → middle), keeping only the exactrx == lxdegenerate guard (Sampling from truncated mixture model results in an error #1501). A non-bracketing interval (Bug in quantile() for some special cases of a MixtureModel #1869) is a separate problem from the solver choice and is intentionally left out of scope.@testset(nocasesloops), andisapproxdefaults instead of fixedatol. Adds coverage for [InverseGaussian?]quantile()hangs for larger number ofσ's (quantile_newtonnot converging) #1898 and aFloat32round-trip.🤖 Generated with Claude Code