Conversation
syclik
left a comment
There was a problem hiding this comment.
A few things to change. I'll try to update the branch accordingly; it's nothing about the math itself.
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: |
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: |
Does this mean that 3264 needs to be merged before this, or just that it should be closed? |
That #3264 should be closed |
| * @return log(Q(a,z)) with same type as T_a and T_z | ||
| */ | ||
| template <typename T_a, typename T_z> | ||
| inline auto log_q_gamma_cf(const T_a& a, const T_z& z, double precision = 1e-16, |
There was a problem hiding this comment.
Quick Q for clarify: Does this function only work for double types or should it be able to accept autodiff types as well? Reading the gamma_lccdf code I'm kind of confused on the branching logic.
There was a problem hiding this comment.
I'm seeing autodiff types being passed through. For example, L100 in gamma_lcdf.hpp:
log_Qn = internal::log_q_gamma_cf(alpha_dbl, beta_y);
where const T_partials_return beta_y = beta_dbl * y_dbl;. So it'll only be non-double when we're using higher order autodiff.
stan/math/prim/prob/gamma_lccdf.hpp
Outdated
| [[maybe_unused]] const double beta_y_dbl = value_of(value_of(beta_y)); | ||
| [[maybe_unused]] const double alpha_dbl_val = value_of(value_of(alpha_dbl)); | ||
|
|
||
| if (use_cf) { |
There was a problem hiding this comment.
I was having a hard time parsing this logic, could we do the branching logic for the autodiff types first and then do the use_cf on the inside? It looks like you need branching logic for both var and fvar types.
|
Notes: add |
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: |
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: |
b0ce647 to
81c67a0
Compare
df0101e to
637b398
Compare
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 this failed on the merge: If you have time to look at it shortly, great, otherwise we can revert and re-open the changes later |
|
@syclik do you think this is related to any changes you made? I don't know when I can look into this. |
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.This supercedes and improves upon pr #3264.
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