-
Notifications
You must be signed in to change notification settings - Fork 100
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
Increase precision of betainc C implementation #908
Conversation
Actually this is fine, we just need to use the same constants Scipy used to use when betainc relied on Cephes |
92a2ecf
to
e9ddb20
Compare
e9ddb20
to
78bb748
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #908 +/- ##
==========================================
- Coverage 81.24% 81.23% -0.01%
==========================================
Files 170 170
Lines 46928 46826 -102
Branches 11484 11465 -19
==========================================
- Hits 38126 38041 -85
+ Misses 6603 6592 -11
+ Partials 2199 2193 -6
|
#define MAXGAM 171.624376956302725 | ||
#define EPSILON 2.2204460492503131e-16 | ||
#define EPSILON 1.11022302462515654042e-16 // 2**-53 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Obviously!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't there a beta function that doesn't have a C implementation at all? I recall seeing warnings when you do pm.find_constrained_prior
on a Beta. It's not related to this is it?
We added C code because of those warnings, but I also disabled that sort of warnings because they were counter-productive: #794 (People were sampling with JAX and worrying about the warning) |
Description
Need to use more aggressive constants to match Scipy
Related Issue
Checklist
Type of change