Skip to content
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

Merged
merged 1 commit into from
Jul 9, 2024

Conversation

ricardoV94
Copy link
Member

@ricardoV94 ricardoV94 commented Jul 9, 2024

Description

Need to use more aggressive constants to match Scipy

Related Issue

Checklist

Type of change

  • New feature / enhancement
  • Bug fix
  • Documentation
  • Maintenance
  • Other (please specify):

@ricardoV94 ricardoV94 requested a review from twiecki July 9, 2024 13:30
@ricardoV94 ricardoV94 changed the title Revert betainc c impl Revert betainc C implementation Jul 9, 2024
@ricardoV94 ricardoV94 removed the request for review from twiecki July 9, 2024 13:35
@ricardoV94 ricardoV94 marked this pull request as draft July 9, 2024 13:35
@ricardoV94
Copy link
Member Author

Actually this is fine, we just need to use the same constants Scipy used to use when betainc relied on Cephes

@ricardoV94 ricardoV94 changed the title Revert betainc C implementation Increase precision of betainc C implementation Jul 9, 2024
@ricardoV94 ricardoV94 marked this pull request as ready for review July 9, 2024 14:11
@ricardoV94 ricardoV94 added bug Something isn't working and removed C-backend labels Jul 9, 2024
Copy link

codecov bot commented Jul 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.23%. Comparing base (56c30e0) to head (78bb748).
Report is 9 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            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     
Files Coverage Δ
pytensor/scalar/math.py 87.26% <100.00%> (ø)

... and 21 files with indirect coverage changes

#define MAXGAM 171.624376956302725
#define EPSILON 2.2204460492503131e-16
#define EPSILON 1.11022302462515654042e-16 // 2**-53
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Obviously!

Copy link
Member

@jessegrabowski jessegrabowski left a 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?

@ricardoV94
Copy link
Member Author

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)

@ricardoV94 ricardoV94 merged commit a6e79f2 into pymc-devs:main Jul 9, 2024
59 checks passed
@ricardoV94 ricardoV94 deleted the revert_betainc_c_impl branch July 9, 2024 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working maintenance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New betainc C implementation less precise than Scipy
3 participants