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

Add gammainc(c) derivatives #513

Merged
merged 3 commits into from
Sep 19, 2021
Merged

Conversation

ricardoV94
Copy link
Contributor

This PR adds a Python implementation of derivatives of the gammainc and gamaincc. Closes #74.

The algorithms were adapted from the STAN library.

@codecov
Copy link

codecov bot commented Jul 8, 2021

Codecov Report

Merging #513 (13d3984) into main (0ae63d1) will increase coverage by 0.01%.
The diff coverage is 82.79%.

❗ Current head 13d3984 differs from pull request most recent head 924e8c4. Consider uploading reports for the commit 924e8c4 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #513      +/-   ##
==========================================
+ Coverage   76.89%   76.91%   +0.01%     
==========================================
  Files         149      149              
  Lines       46602    46689      +87     
  Branches    10230    10242      +12     
==========================================
+ Hits        35833    35909      +76     
- Misses       8190     8196       +6     
- Partials     2579     2584       +5     
Impacted Files Coverage Δ
aesara/scalar/math.py 84.52% <82.79%> (+0.43%) ⬆️

@twiecki
Copy link
Contributor

twiecki commented Aug 6, 2021

Is speed a concern here with a pure-Python implementation?

@ricardoV94
Copy link
Contributor Author

ricardoV94 commented Aug 6, 2021

Is speed a concern here with a pure-Python implementation?

Definitely. We have to fix #512 before deciding on how to proceed in #514 (pure Aesara function or this Op with specialized C/Numba dispatch).

If it's the latter we still want to keep the Python code for fast compilation mode

Copy link
Member

@brandonwillard brandonwillard left a comment

Choose a reason for hiding this comment

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

Do you want to create a follow-up issue for the non-Python implementation and merge this in the meantime?

@ricardoV94
Copy link
Contributor Author

Do you want to create a follow-up issue for the non-Python implementation and merge this in the meantime?

There is already a follow up issue here: #514

Yes, I think we can merge this already

twiecki
twiecki previously approved these changes Sep 17, 2021
@brandonwillard
Copy link
Member

The last commit was a merge commit; I've rebased instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement grad for GammaInc
3 participants