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 ChainRules definitions for gamma(a, x), loggamma(a, x), and gamma_inc #305

Merged
merged 6 commits into from
May 18, 2021

Conversation

devmotion
Copy link
Member

@devmotion devmotion commented Apr 7, 2021

I added ChainRules definitions for gamma(a, x), loggamma(a, x), and gamma_inc according to https://functions.wolfram.com/GammaBetaErf/GammaRegularized/introductions/Gammas/ShowAll.html. Similar to the Bessel functions, derivatives with respect to the first argument a are not implemented since they are given in terms of a (regularized) hypergeometric function.

Unfortunately, it seems due to the error thrown for derivatives of the first argument these rules can't be tested with test_frule and test_rrule since these functions always unthunk all derivatives. It would be good if one could somehow exclude some derivatives from the tests completely and not only from the finite differencing checks. I'll open an issue in ChainRulesTestUtils regarding this question.

(BTW I assume that due to this issue the derivatives of the Bessel functions are not tested currently)

@codecov
Copy link

codecov bot commented Apr 7, 2021

Codecov Report

Merging #305 (bdd6243) into master (a48ba24) will decrease coverage by 0.96%.
The diff coverage is n/a.

❗ Current head bdd6243 differs from pull request most recent head 98bc7d0. Consider uploading reports for the commit 98bc7d0 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #305      +/-   ##
==========================================
- Coverage   89.31%   88.35%   -0.97%     
==========================================
  Files          12       12              
  Lines        2640     2653      +13     
==========================================
- Hits         2358     2344      -14     
- Misses        282      309      +27     
Flag Coverage Δ
unittests 88.35% <ø> (-0.97%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/chainrules.jl 100.00% <ø> (ø)
src/gamma_inc.jl 88.20% <0.00%> (-4.24%) ⬇️
src/expint.jl 95.63% <0.00%> (ø)
src/SpecialFunctions.jl 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a48ba24...98bc7d0. Read the comment docs.

@stevengj
Copy link
Member

stevengj commented Apr 7, 2021

Great, thanks! It would be good to resolve the testing question.

@devmotion
Copy link
Member Author

Soon it will be possible to test (existing and new) partially implemented differentials properly: JuliaDiff/ChainRulesTestUtils.jl#140

@andreasnoack
Copy link
Member

Should this one be update now that JuliaDiff/ChainRulesTestUtils.jl#140 is merged?

@andreasnoack
Copy link
Member

The drone runs seems to have connectivity issues. Not sure why.

@andreasnoack andreasnoack merged commit e59e092 into JuliaMath:master May 18, 2021
@devmotion devmotion deleted the dw/chainrules_gamma branch May 18, 2021 07:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants