-
-
Notifications
You must be signed in to change notification settings - Fork 198
fix gamma lccdf for alpha > 30 and overflow #3264
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
|
@WardBrian I think the test failure is a GitHub actions failure. Can this be restarted? |
Jenkins Console Log Machine informationNo LSB modules are available. Distributor ID: Ubuntu Description: Ubuntu 20.04.3 LTS Release: 20.04 Codename: focalCPU: G++: Clang: |
|
@spinkney that did go away, but there are some jenkins failures that look legitimate: https://jenkins.flatironinstitute.org/blue/organizations/jenkins/Stan%2FMath/detail/PR-3264/3/pipeline/591 Locally run I do get some failures, but seemingly fewer than CI, when I run this. |
Those are legit. Is this all around having a 0 as input? |
|
I don't think we're passing zero in this test: https://github.com/stan-dev/math/blob/develop/test/prob/gamma/gamma_ccdf_log_test.hpp It sounds like it's an issue with vector/matrix inputs |
|
@WardBrian I'll look at this tomorrow. I suspect it's the addition of |
…as treated like a scalar, so lgamma(alpha) got cached from alpha[0] and reused for all elements (breaks gradients for vector/matrix inputs). 2) y == 0 now sums correctly over vectors.
|
@WardBrian the recent push passes locally. Fixed two vectorization bugs. The first, when alpha is a constant vector it was treated like a scalar, so The second happened with I was able to simplify the expression with |
Jenkins Console Log Machine informationNo LSB modules are available. Distributor ID: Ubuntu Description: Ubuntu 20.04.3 LTS Release: 20.04 Codename: focalCPU: G++: Clang: |
|
This needs #3265 before merging. |
Summary
This pr closes #3257. The
gamma_lccdfexperiences numerical instability for alpha approximately greater than 30. It’s due to gamma_q or grad_reg_inc_gamma. This pr rewrites the code to use gamma_p and grad_reg_lower_inc_gamma, which avoids potential instability in a tgamma(alpha) and digamma(alpha) call. It seems to sample a bit faster than just wrappinggamma_lcdfwithlog1m_exp.Tests
I've implemented new tests in the problematic regime.
Release notes
The
gamma_lccdfis made more robust for alpha values. The issue was overflow in the derivatives when alpha was greater than 30.Checklist
Copyright holder: Publicis Groupe
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