Skip to content

Add betainc C implementation #798

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

Merged
merged 9 commits into from
Jul 5, 2024
Merged

Conversation

arthus701
Copy link
Contributor

@arthus701 arthus701 commented Jun 3, 2024

Description

As discussed over at #673, this adds a C-implementation to BetaInc.

Related Issue

Type of change

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

@ricardoV94
Copy link
Member

Seems like you need to run pre-commit for auto-formatting

@maresb
Copy link
Contributor

maresb commented Jun 4, 2024

Could you remove the comma after self and reformat? The comma causes the formatter to make a bunch of unnecessary new lines.

@arthus701
Copy link
Contributor Author

arthus701 commented Jun 4, 2024

Thanks, that also led me to realize I didn't set a code cache version. I set 0.0.1 now, I hope that's fine.

Copy link

codecov bot commented Jun 4, 2024

Codecov Report

Attention: Patch coverage is 83.33333% with 2 lines in your changes missing coverage. Please review.

Project coverage is 80.83%. Comparing base (b7b309d) to head (40e6877).
Report is 197 commits behind head on main.

Files with missing lines Patch % Lines
pytensor/scalar/math.py 83.33% 1 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #798      +/-   ##
==========================================
+ Coverage   80.76%   80.83%   +0.07%     
==========================================
  Files         163      163              
  Lines       46941    46907      -34     
  Branches    11499    11470      -29     
==========================================
+ Hits        37912    37918       +6     
+ Misses       6786     6772      -14     
+ Partials     2243     2217      -26     
Files with missing lines Coverage Δ
pytensor/scalar/math.py 87.26% <83.33%> (-0.08%) ⬇️

... and 10 files with indirect coverage changes

raise NotImplementedError("type not supported", type)

def c_code_cache_version(self):
return (0, 0, 1)
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: return (1,)?

Copy link
Member

@ricardoV94 ricardoV94 left a comment

Choose a reason for hiding this comment

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

Looks great, just one nit

Comment on lines 1506 to 1507
if node.inputs[0].type in float_types:
return f"""{z} = BetaInc({a}, {b}, {x});"""
Copy link
Member

@ricardoV94 ricardoV94 Jun 5, 2024

Choose a reason for hiding this comment

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

Don't all the inputs need to be floats?

@arthus701
Copy link
Contributor Author

Sorry for the delay, I addressed your comments. Thanks for catching the input check.

@ricardoV94 ricardoV94 added enhancement New feature or request C-backend labels Jul 5, 2024
@ricardoV94 ricardoV94 merged commit 31bf682 into pymc-devs:main Jul 5, 2024
55 of 56 checks passed
@ricardoV94
Copy link
Member

Sorry for the delay @arthus701 and thanks a lot

ricardoV94 added a commit to ricardoV94/pytensor that referenced this pull request Jul 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-backend enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants