Converge the default continuous inverse_cdf instead of a fixed 16 iterations - #390
Merged
YeungOnion merged 1 commit intoJul 19, 2026
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #390 +/- ##
==========================================
+ Coverage 94.73% 94.94% +0.21%
==========================================
Files 59 59
Lines 13052 13146 +94
==========================================
+ Hits 12365 12482 +117
+ Misses 687 664 -23 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The ContinuousCDF::inverse_cdf default ran a fixed 16-iteration bisection with no convergence check, narrowing the bracket by only 2^16 regardless of its width. Distributions without a closed-form quantile (Chi, InverseGamma) inherited errors far above the crate's DEFAULT_RELATIVE_ACC: Chi::new(1).inverse_cdf(1e-12) returned 3.05e-5 instead of 1.25e-12, and even the median was only accurate to ~1e-5. Bisect until the bracket meets DEFAULT_RELATIVE_ACC, seed it from the distribution's finite domain bounds, and invert sf in the upper half where cdf saturates to one. Worst-case relative error over p in [1e-12, 1-1e-12] drops from ~1e7 to <=1.3e-14 for both affected distributions.
gaoflow
force-pushed
the
fix-continuous-default-inverse-cdf-convergence
branch
from
July 18, 2026 15:04
3c07a7c to
49983c2
Compare
Contributor
|
Thanks! would you be willing follow this with something similar to the brent-like approach that uses NR steps that you introduced for Gamma in #382? |
YeungOnion
pushed a commit
that referenced
this pull request
Jul 27, 2026
Chi and InverseGamma were the only continuous distributions still using the generic bisection default for inverse_cdf. Give them a custom solver in the same brent-like + Newton-Raphson vein as Gamma (#382): a shared internal::newton_raphson_quantile that brackets the quantile to a factor of two and refines it with safeguarded Newton steps, falling back to bisection whenever a step is non-finite or leaves the bracket. Two refinements over a plain port keep it accurate and fast across the whole range, including the tails #390 cared about: - convergence is tested on the relative step, not prec::convergence's absolute 1e-9 (meaningless for a 1e-12 quantile); and the Newton step is checked for convergence before the bracket is tightened, so a converged step that rounds onto an endpoint is not rejected into a spurious bisection. - the upper half inverts sf rather than cdf, which saturates to one and loses the resolution to place a deep upper-tail quantile. Matches scipy/mpmath (dps=60) to <= 3.4e-15 relative error over p in [1e-12, 1 - 1e-12] for both distributions, and converges in a handful of Newton steps (medians ~5, vs ~48 fixed bisection steps), roughly 60% fewer cdf/pdf evaluations across the grid.
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.
The
ContinuousCDF::inverse_cdfdefault runs a fixed 16-iteration bisection with noconvergence test, so it only ever narrows the initial bracket by 2^16 regardless of how
wide the bracket is or how small the quantile is. Every continuous distribution without a
closed-form quantile inherits this — currently
ChiandInverseGamma— and the resultsits far from the crate's own
DEFAULT_RELATIVE_ACC(1e-14):Chi::new(1).inverse_cdf(1e-12)returns3.05e-5instead of1.25e-12; in fact everyp <= ~1e-6returns the same3.05e-5bracket floor. Even the median is only accurateto ~1e-5.
InverseGammashows the same ~1e-5 baseline error, growing into the tails.This is the continuous mirror of the discrete side, which already converges via
internal::integral_bisection_search.The fix replaces the fixed count with a bisection that runs until the bracket agrees to
DEFAULT_RELATIVE_ACC(capped at 100 iterations and by float resolution). Two smallerchanges make the tails accurate:
min()/max()when finite, only doubling out from±2 when a bound is infinite.
Chi/InverseGammaare supported on[0, ∞), so the old[-2, 2]seed wasted the lower half.sfrather thancdf. Ascdfsaturates to one it can nolonger place the quantile (many
xmap to a singlef64cdf value);sfstays wellconditioned. Distributions that don't override
sffall back to1 - cdf, so this is ano-op for them.
Across
p ∈ [1e-12, 1-1e-12]the worst relative error forChiandInverseGammadropsfrom ~1e7 (and ~1e-5 at the median) to
<= 1.3e-14. Reference quantiles in the new testscome from scipy, cross-checked against mpmath at 60 digits; a round-trip test covers a
wider grid.