Conversation
|
thanks! I'll take a look tomorrow and then we'll probably want @SteveBronder to look it over too |
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 have you had a change to look yet? |
|
@syclik I made a good number of cleanup changes in the branch below. Feel free to take a look and merge what you would like |
SteveBronder
left a comment
There was a problem hiding this comment.
@syclik or @spinkney can one of yinz have a look at the changes I made below? imo I think it would be nice to clean this a bit before merge.
https://github.com/stan-dev/math/tree/fix/gamma-lccdf-v3-review
(diff)
https://github.com/stan-dev/math/compare/fix-gamma-lccdf-v3...fix/gamma-lccdf-v3-review?expand=1
That looks good for what I understand. I still don't have a good grasp of On the |
I absolutely prefer the more explicit types. While there's a lot of benefit to Otherwise, nice changes! Should we try to open a PR against this branch and we can merge most of it? |
|
Just opened a PR here! Feel free to add or remove what you like.
The godbolt below has an example that may be easier to understand than my description above.
In the godbolt below, the top left program has https://godbolt.org/z/ocThfc8fK Also happy to jump on a call to chat! |
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: |
|
Ping me when tests pass and I'll approve |
|
@SteveBronder thank you for those godbolts! That's super helpful! Also, do we need to restart the tests? |
|
I might have messed up some of the types. Let me check into it first, @spinkney |
|
@SteveBronder, the proposed changes that you made didn't actually work. I went back to your last commit: 3fb810f. The updates I made to the branch didn't solve the problem. Here's the reproducible way to get the error: At commit 3fb810f, the compiler error looks like: and it keeps going. I didn't delete the proposed branch, so what I'll do is reset this branch back to the last working commit. |
e4240a7 to
86ac561
Compare
|
@SteveBronder, this branch was reset back to 86ac561 (the last commit before the Do you want to reset the other branch back to 3fb810f and work through the test? |
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 fixes the reverted PR: #3272.
cc: @spinkney
Tests
New probability 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, Daniel Lee
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