-
-
Notifications
You must be signed in to change notification settings - Fork 191
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
More stable implementation of neg_binomial_2_log_lpmf #1830
More stable implementation of neg_binomial_2_log_lpmf #1830
Conversation
Ready for review. Maybe @bbbales2 could take care of it? Hopefully we can get this in for the 2.23 release... Thanks! |
I am sure we can get it in. Note that this is a bugfix so the deadline is actually 20th of April. But if we can get it for the release candidate that would be even better. |
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
@serban-nicusor-toptal Looks like this test is popping up in downstream performance:
Does that mean a computer crashed? Anything we can do to un-crash that haha? @martinmodrak this looks good to me. I'll merge after we get the tests figured out. |
Hey, reduced the parallel variable from 6 to 4 so we rule out that the load may crash the machine. Gonna restart the build. |
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
@serban-nicusor-toptal looks like it failed again. Ideas? |
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
Thanks @martinmodrak, it's in! And thanks for the help @serban-nicusor-toptal! |
Summary
These changes mirror those done for
neg_binomial_2_lpmf
in #1497,binomial_coefficient_log
is used to avoid the necessity to delegate to Poisson.Tests
neg_binomial_2_lpmf(n, exp(eta), phi)
n = 0
andn = 1
Side Effects
The way the density is computed changed to improve stability, at the cost of tangling
eta
andphi
together earlier, so less computation can be avoided whenpropto=true
andphi
is fixed.Release notes
More stable implementation of neg_binomial_2_log_lpmf
Checklist
Math issue neg_binomial_2_log_lpmf is not stable for large phi #1495
Copyright holder: Institute of Microbiology of the Czech Academy of Sciences
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