Fix NaN in gammpapprox lower branch for large a - #51
Open
gaoflow wants to merge 1 commit into
Open
Conversation
For a >= ASWITCH (100) the Gauss-Legendre quadrature path took the log of the integration width (xu - x), which is negative in the x <= a-1 branch, so (xu - x).ln() evaluated to NaN. That poisoned gammp/gammq (chi-square / gamma / Poisson CDF returned NaN over the whole 0.2a <= x <= a-1 region) and made invgammp panic for lower-tail p < 0.5 when the NaN reached gammp's assert. The log-space rewrite that introduced this kept only the x > a1 case of the Numerical Recipes sign convention and dropped the lower-branch fold. Take the log of |xu - x| and restore the per-branch sign on return: in the lower branch the signed integral is negative, so P = ans and Q = 1 - ans; the upper branch is unchanged (P = 1 - ans, Q = ans). With gammpapprox correct, invgammp converges for large a on both tails without any extra guard. Values verified against mpmath; the small-a gser/gcf path is untouched.
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.
Problem
For
a >= ASWITCH(100),gammp/gammqtake the Gauss-Legendre quadraturepath in
gammpapprox. Over the whole region0.2a <= x <= a-1they returnNaN, andinvgammppanics for lower-tailp < 0.5.These are the regularized incomplete gamma functions, i.e. the chi-square /
gamma / Poisson CDF, so for example a chi-square CDF with >= 200 degrees of
freedom (
a = df/2 >= 100) isNaNover its entire lower half.Root cause
gammpapproxcomputes the answer in log space:In the
x <= a1branchxuis set belowx, soxu - x < 0and(xu - x).ln()isNaN, which propagates to bothPandQ.invgammpthen feeds that
NaNintogammp'sassert!(x >= 0.0)(NaN >= 0.0isfalse) and panics.
The Numerical Recipes
gammpapproxkeepsans = sum*(xu-x)*exp(...)as asigned quantity (negative in this branch) and folds the sign into the return:
P = ans > 0 ? 1-ans : -ans,Q = ans >= 0 ? ans : 1+ans. The log-spacerewrite in
948acdekept only thex > a1case and dropped the lower-branchfold, while also taking
lnof a negative width.Fix
Take the log of the magnitude
|xu - x|and restore the per-branch sign onreturn:
x <= a1):P = ans,Q = 1 - ansx > a1):P = 1 - ans,Q = ans(unchanged)With
gammpapproxcorrect,invgammpconverges on both tails for largeawith no extra guard.
Verification
Reference values are mpmath (
gammainc(a, 0, x, regularized=True)), stable atdps 30 and 35. The new tests in
tests/gammpapprox_large_a_test.rscover:gammp/gammqfora in {100, 150, 200, 500}withxspanning the formerNaN region,
x = a-1,x = a, andx > a; mid-range values (0.158, 0.00456,0.472, 0.972) asserted at full precision, plus
P + Q == 1and finiteness.P(a, .)monotonic increasing across0.2a .. a + 6*sqrt(a).invgammpround-trip both tails (gammp(a, invgammp(p, a)) == p) and adirect match against mpmath
findrootvalues.agser/gcfpath unchanged.The old large-a quadrature test only exercised
gammp(150, 150)(x = a, theupper branch), so the lower branch was untested. Full suite passes;
cargo fmtclean; the change adds no new
clippywarnings.Fixes the large-a case of #4.