Conversation
Member
Author
|
I'll be away for two weeks, but did create this PR to avoid overlap in work. Similar PR should be made for incomplete beta gradient roots (mentioned in PR text) |
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.
Use Eigen's
igamma_der_afor the incomplete gamma gradient roots (this PR was Claude assisted)Summary
grad_reg_inc_gammaandgrad_reg_lower_inc_gammacompute the derivative of the regularized incomplete gamma function with respect to its shape parameter. They are the gradient roots under 27 tail functions in 10 distributions. Both have defects that make the gradient wrong by tens of percent, or NaN, in regions that ordinary models reach. This PR replaces both implementations withEigen::numext::igamma_der_a, which is already vendored inlib/eigen_5.0.1. The result is more accurate everywhere measured, about four times faster, and about 120 lines shorter.The values of the distributions do not change. Only the gradients change.
Comparison result, one table
a≈z≈10⁴a≈z≈10⁴z ≳ 700doublerootsgamma_lcdfper obs, α=50varroot (unused in-repo)The defects in the current code on
developEverything in this section and the next describes the code on
developat5252d51d47, before this PR. All numbers were measured against an mpmath reference at 80 digits, built through two independent routes that agree to at least 13 digits, and confirmed by compiling and running the realdevelopheaders rather than a transcription. The section "Accuracy after the change" below gives the same measurements for this PR.1. On
develop,grad_reg_inc_gammatakes an invalid branch whenzis neara. The testz >= a && z >= 8selects a fixed 10-term asymptotic expansion from DLMF 8.11.2. That expansion needsz >> a. The whole banda <= z < 2atakes it, and there the terms do not decay. The relative error atz = ais 4.4e-02 ata = 20, 5.8e-01 ata = 100and 7.2e-01 ata = 171. The expansion also ignores theprecisionargument and always takes exactly 10 terms.2. On
develop,grad_reg_inc_gammareturns NaN foraabove about 171.6. Every call site computesg = tgamma(a)and passes it in. Above 171.62 that isinf, so the correction termexp(-z + (a-1) log z) * S / gisinf / inf. There is no guard at any call site. The series branch fails the same way throughexp(a * log z), which overflows whena log z > 709, for example at(a, z) = (150, 150).3. On
develop,grad_reg_lower_inc_gammaloses all accuracy forzwell abovea. Its delegation test is(a < 0.8 && z > 15) || (a < 12 && z > 30) || a < sqrt(-756 - z*z + 60*z). The third term needs-z^2 + 60z - 756 > 0, which holds only forzin (18, 42). So forz > 42anda >= 12the Gautschi branch always runs, and thereemz * (log_z * sum_a - sum_b)is a difference of two large nearly equal sums. It returns about 1e-14 of rounding noise, with arbitrary sign, where the true value can be 1e-148. Examples: at(50, 150)the reference is -8.29e-22 and develop returns -4.70e-15; at(50, 500)the reference is -5.34e-148 and develop returns +1.47e-14.4. On
develop, the 1e-6 defaultprecisionis real but small. It sets the error of the plain series branch, 9.2e-08 ata = 0.5and 9.0e-05 ata = 8. It is the smallest of the four problems.On
develop, the defects reach the userMeasured at the distribution level, with
var, against the same reference. The CDF value is correct in every case; only the gradient is wrong. This PR brings every row below to the reference within 1e-13; see "Accuracy after the change".developchi_square_lccdfchi_square_lccdfchi_square_lcdfgamma_cdfinv_gamma_lccdfNaN gradients, confirmed at the distribution level:
chi_square_lccdfandchi_square_lcdfat nu = 360,gamma_cdfat alpha = 180,inv_gamma_lccdfat alpha = 180,scaled_inv_chi_square_lcdfat nu = 360.gamma_lcdfandgamma_lccdfare not affected by defects 1 and 2. Theirlog_gamma_q_dgammadispatch avoids that path, and they measure 1e-14 to 5e-13 in the same region.gamma_lcdfdoes inherit defect 3.What this PR does
Eigen::internal::igamma_generic_impldifferentiates the Cephes power series and the Cephes continued fraction term by term, stops at machine epsilon, and branches onx > 1 && x > a, which is the well-conditioned split. It is already in the tree, it is tested upstream, and it is generic in the scalar type.Only three of its helpers are restricted to
floatanddouble:lgamma_impl,digamma_implandcephes_helper. This PR supplies those three for Stan's autodiff scalars in two new headers,stan/math/fwd/fun/Eigen_SpecialFunctions.hppandstan/math/rev/fun/Eigen_SpecialFunctions.hpp.igamma_num_iterationsalready falls back correctly and needs nothing.Both roots then become thin wrappers, and the hand-written series, the asymptotic branch and the Gautschi branch are deleted. One algorithm now serves every scalar type and every autodiff order, so reverse mode, forward mode,
fvar<fvar<double>>,fvar<var>andhessian()all agree.Doing only the
doublepath was tried first and rejected. It lefthessian()on the old code, sogradient()andhessian()returned first derivatives that differed by 5.8e-01 atalpha = 100and by NaN atalpha = 180. Making every order use one algorithm is the point of the design.The signatures do not change, so no call site changes.
g,dig,precisionandmax_stepsare accepted and ignored. Removing them touches 27 call sites and is proposed separately, to keep this PR reviewable.Accuracy after the change, this PR
Sweep of 1800 points in nine regions chosen by the branch structure, scored against the mpmath reference. Worst relative error over every region, with the count of non-finite results out of 200 per region.
grad_reg_inc_gammagrad_reg_lower_inc_gammaWorst over all 1800 points after the change: 4.3e-12. No non-finite result anywhere, including the 200 points above the
tgammaoverflow limit.A second, larger sweep compared develop and this branch head to head: 49 990 points with
aandzboth in [1e-3, 1e4], with dense sampling onz ≈ a, thez = 8seam,a = 171.62anda·log z = 709. Develop returns NaN or inf at 6 379 of those points forgrad_reg_inc_gamma. This branch is finite at every one. Median relative error on this branch is between 1e-16 and 1e-14 in every region; develop's medians reach 5e-3.One known limit of the new implementation. Near
z = athe error grows witha: worst 3e-14 fora < 10, 3e-11 fora < 1 000, 2.9e-7 fora < 10 000, and 5.2e-6 at thea = z = 10⁴corner, where Eigen's Cephes recurrence loses digits near the mode. At that point three independent references agree to 17 digits, so this is the algorithm, not the measurement. It is finite where develop is NaN, and below the 1e-4 thatexpect_adtolerates.A reference caveat. In the far upper tail at
a ≳ 10⁴the mpmath reference is certified to 11 digits only, so errors reported there are bounded at 1e-11 by the reference, not by the code.Autodiff orders agree on this branch to 2.2e-14 across
double,var,fvar<double>andfvar<fvar<double>>. On develop, one order returns NaN where another returns a number in 7 of 10 regions.Speed
Develop and this branch compiled from one source and run back to back on the same exclusive node: Xeon E5-2680 v3 at 2.50 GHz,
performancegovernor, 1024-point input block, with an accumulator-dependent term added to every input so the inner loop is not loop-invariant, a baseline subtracted, and every result checked against a plausible cycle count. Two rounds each; run-to-run spread under 3 %. The input grid stays where develop terminates and is finite,ain [0.5, 60] andzin [0.1, 40], which favours develop.Function level, net ns per call:
grad_reg_inc_gamma, double, incl.tgamma+digammagrad_reg_inc_gamma, fvargrad_reg_inc_gamma, fvar<fvar>grad_reg_lower_inc_gamma, doublegrad_reg_lower_inc_gamma, fvargrad_reg_lower_inc_gamma, fvar<fvar>grad_reg_inc_gamma, var, value + gradgrad_reg_lower_inc_gamma, var, value + gradThe one slower row is the
varinstantiation of the upper root. Eigen's series and continued fraction take more operations than develop's 10-term asymptotic branch on the part of this grid where that branch runs, and withvarevery operation is a tape node. No function in the library calls the roots withvar; every distribution passesdoublepartials. The row is included for completeness.Distribution level, which is what a model pays:
N = 1000observations, one sharedvarshape, value plus reverse sweep timed together, ns per observation:gamma_cdfgamma_lcdfgamma_lccdfchi_square_lccdfinv_gamma_lcdfgamma_cdfgamma_lcdfgamma_lccdfchi_square_lccdfinv_gamma_lcdfForward mode did not terminate on develop
stan/math/fwd/fun/gamma_q.hppon develop held its own inlined copy of the series withwhile (fabs(delta) > 1e-6)and no iteration cap. Forzabove about 700 the alternating terms(-z)^k / k!overflow toinfbefore they decay,deltastaysinf, and the loop never exits. The root's own asymptotic branch callsgamma_q(a, z)withfvararguments, sograd_reg_inc_gammareached it too. In the comparison runs a single call ata = 0.168, z = 4 748did not return in 15 minutes. The existing mix test forgamma_qcarries a commented-outexpect_value(f, 8.01006, 2.47579e+215)with the note "this still fails forward mode", which is the same defect.This PR removes that code, so the loop is gone. Because a test that hangs is not usable in CI, the regression test
mathMixScalFun.gammaQ_fwd_shape_derivative_large_zuses points where the old series terminates but is wrong: at(a, z) = (1.5, 300)develop returns1.2e+112for a derivative whose value is5.7e-129. On develop the test fails; on this PR it passes.Testing
New file
test/unit/math/prim/fun/grad_reg_inc_gamma_accuracy_test.cpp, with fixed references from mpmath at 80 digits.expect_adcompares against finite differences and cannot see errors of this size, so these are absolute references rather than autodiff comparisons.New case
ProbInternalMath.gradRegIncGamma_gradient_matches_hessianasserts that the first derivative ofgamma_cdfis the same whether it comes from reverse mode or fromhessian(). It needs no external reference.Two new cases in
test/unit/math/mix/fun/gamma_q_test.cpp:gammaQ_fwd_shape_derivative_large_z(the indirect non-termination test described above) andgammaQ_fwd_z_derivative_large_a, which checks the second-argument derivative at(a, z) = (200, 150)against the closed form-z^(a-1) e^(-z) / Γ(a); on develop that returned NaN because bothpowandtgammaoverflow. Installing the develop versions of the three changed files makes both cases fail and the rest of the file pass; restoring this PR makes all four cases pass, with the file checksums back to their starting values.Evidence that the tests find the defects, by installing the develop headers, keeping the new tests, and restoring afterwards:
The header checksums in the third stage equal the first, so the swap is clean.
Three constants in
test/unit/math/mix/fun/grad_reg_inc_gamma_test.cpphad to be corrected. They expected 0.38984156 ford/da Q(0.5, 1.0), whose true value is 0.38983726432851057. The old constant encoded two errors together: the rounded support values that the test passes in, sinceg = 1.77245is 2.2e-06 away fromtgamma(0.5), and the 1e-6 series truncation.Two cases in that file were not touched and still pass:
gradRegIncGamma_fv_1stderivandgradRegIncGamma_fv_2ndderivassert the second and third derivatives at(0.5, 1.0)to 1e-6 against constants that predate this work. That is independent confirmation of the higher-order derivatives.The existing
grad_reg_lower_inc_gamma_testchecks 9600 fixed values from Mathematica with a 1e-10 absolute tolerance. Eigen satisfies all 9600. That grid coversain [0.0001, 19.75] andzin [0.0001, 29.75], and contains no point witha >= 12andz >= 3a, which is exactly the region where defect 3 lives. The grid stopped one step short of it.On
develop, a duplicated copy of the same series infwd/fun/gamma_q.hppOn
develop,stan/math/fwd/fun/gamma_q.hppcarried its own inlined copy of the series ingrad_reg_inc_gamma: the samewhile (fabs(delta) > 1e-6), the same(1.0 - u) * (dig - l) + exp(x1 * l) * S / g, the same hard-coded tolerance, and without the second branch. This PR replaces both copies with a call tograd_reg_inc_gamma, which is the function they were copied from.stan/math/fwd/fun/gamma_p.hppwas already written this way, and callsgrad_reg_lower_inc_gamma.The duplicate had been hiding the defect. In
test/prob/chi_square, the generated third-order case compares the distribution's analytic partials against autodiff through the reference formulalog(gamma_q(nu * 0.5, y * 0.5)). Both routes used the same inaccurate series, so their errors cancelled and the comparison passed. With an accurate root and a stale copy, the same comparison fails by 2.4e-03. After the copy is removed, it passes with the new root and with the old one, because the two routes are then consistent either way.One item in that file is left for a follow-up:
der2is-exp(-x2) * pow(x2, x1 - 1) / tgamma(x1), which overflows forx1above 171.6 and silently returns -0, in the same way as defect 2 above.gamma_palready uses the log-space form.Dependency on Eigen
The two new headers specialize
lgamma_impl,digamma_implandcephes_helperinsideEigen::internal. Stan Math already specializes inside that namespace, instan/math/rev/core/Eigen_NumTraits.hpp. The cost is that an Eigen upgrade which renames those three helpers breaks the build. That failure is loud and immediate, not silent, and the alternative was to copy about 300 lines of the Cephes algorithm into Stan Math and maintain it here.The headers belong to the
funlayer, notcore, because they needdigammaandlgamma. Putting them besideEigen_NumTraits.hppincorebreaks the include order of every mix target.Not included
OpenCL is unaffected: no OpenCL code calls these roots, and the five affected families have OpenCL implementations only for their
_lpdf, which does not use the incomplete gamma.The incomplete beta gradient roots,
inc_beta_dda,inc_beta_ddbandgrad_reg_inc_beta, have their own defects: they stop on an absolute threshold of 1e-10 and reach 12 % relative error at(a, b, z) = (2500, 2, 0.999), a case their own docstring names as tested. Boost supplies onlyibeta_derivative, which is the derivative with respect tox, so there is no vendored replacement. The published algorithm is Boik and Robinson-Cox (1998), Journal of Statistical Software 3(1). That work is separate from this PR.Removing the now-unused
g,dig,precisionandmax_stepsarguments is also a separate PR.Release notes
Use Eigen's
igamma_der_afor the incomplete gamma gradient rootsChecklist
Copyright holder: Aalto University
The copyright holder is typically you or your assignee, such as a university or company. By submitting this pull request, the copyright holder is agreeing to the license the submitted work under the following licenses:
- Code: BSD 3-clause (https://opensource.org/licenses/BSD-3-Clause)
- Documentation: CC-BY 4.0 (https://creativecommons.org/licenses/by/4.0/)
the basic tests are passing
./runTests.py test/unit)make test-headers)make test-math-dependencies)make doxygen)make cpplint)the code is written in idiomatic C++ and changes are documented in the doxygen
the new changes are tested