Repository navigation
Fix incomplete gamma convergence for GAMMA.DIST / CHISQ.DIST - #4945
Conversation
|
Please add some tests for |
ce7decd to
8441e89
Compare
|
Added tests/PhpSpreadsheetTests/Calculation/Statistical/Distributions/GammaBaseTest.php, testing |
|
Well, this is a fine kettle of fish. I merged your GAMMAINV PR, and then I tried to merge this one. Which was almost successful, but ... it broke one of your new tests from the other PR, |
The incomplete gamma primitive used a fixed 32-term power series with no convergence test, so GAMMA.DIST, GAMMADIST, CHISQ.DIST(.RT), GAMMAINV and CHISQ.INV were grossly wrong once the series argument reached ~32 (e.g. CHISQ.DIST.RT(80, 4) returned 0.806 instead of 1.74e-16). Replace it with the standard convergence-tested regularized incomplete gamma: series P(a,x) for x < a+1, continued fraction Q(a,x) for x >= a+1. CHISQ.DIST.RT now uses Q directly so the right tail stays free of 1 - P cancellation. Consolidates the duplicate copy that already existed privately in ChiSquared onto the shared primitive.
It has no remaining callers (kept only for BC) so nothing else exercises it. Values checked against mpmath's gammainc(a, 0, x); the deep-tail case (a=4, x=80) reproduces the convergence bug this PR fixes.
Merging PHPOffice#4946 (bracket expansion) with PHPOffice#4945 (accurate incomplete gamma) means p=0.9999999 no longer hits the alpha*beta*5 safety ceiling -- the corrected regularizedGammaP/Q resolve the true root. Fold it into the extreme-tail data provider against the closed-form -ln(1-p) check instead of asserting the stale clamped bound.
7e798f5 to
a0e67af
Compare
|
Found it — the two PRs interact. |
|
I am glad you found the problem. You now no longer have an "unreachable tail" test (existing test there is moved to a reachable one). I am willing to proceed without one, but not before asking if you might be able to think up a new test for that case. Let me know either way and I will proceed accordingly. |
|
Went hunting for a genuinely-unreachable case, and it's not probability depth — checked down to That's pre-existing and orthogonal to this PR's series/CF fix, so I'd rather open it as its own follow-up than scope-creep this one. Let me know if you'd want a bounded-behavior test added here instead in the meantime. |
|
Thank you for doing the research. There is no need to add a new test to this PR. |
|
Thank you for your contribution. |
GammaBase::incompleteGammasummed a fixed 32-term power series with no convergence test, so every function built on it went badly wrong once the series argument reached ~32:That primitive backs GAMMA.DIST/GAMMADIST, CHISQ.DIST(.RT), GAMMAINV and CHISQ.INV, so all of them were wrong on realistic inputs (any chi-square with x/2 ≳ 32).
Replaced it with the convergence-tested regularized incomplete gamma: series P(a,x) for x < a+1, continued fraction Q(a,x) for x ≥ a+1. CHISQ.DIST.RT now uses Q directly, so the right tail avoids
1 - Pcancellation. A correct copy of this algorithm already existed privately in ChiSquared (used by CHISQ.INV left tail) and is now folded onto the shared method.Expected values come from mpmath (
gammaincregularized, dps 30 ≡ 35); small-x results are unchanged. Three fixtures had encoded the old wrong output (including an impossible CHISQ.INV.RT of 100.0) and were corrected. Full Statistical test suite is green.