-
Notifications
You must be signed in to change notification settings - Fork 104
Exact chainrules derivatives for beta_inc
and beta_inc_inv
#506
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
base: master
Are you sure you want to change the base?
Exact chainrules derivatives for beta_inc
and beta_inc_inv
#506
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #506 +/- ##
==========================================
+ Coverage 94.03% 94.83% +0.80%
==========================================
Files 14 14
Lines 2902 3178 +276
==========================================
+ Hits 2729 3014 +285
+ Misses 173 164 -9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@asinghvi17, @devmotion: I have switched implementations from the porting of the original code (which was probably not licensed correctly as we discussed yesterday) to a port of this repository, which is an independent implementation from @arzwa, unlicensed yet. I have better hopes that @arzwa will allow us to use his code, and have emailed him. By looking at the diff you'll see that this is clearly an independent implementation, and thus, if @arzwa agrees, we are good license-wise. I also have contacted the original authors, so if they come around faster we can still revert my last commit. |
I agree, you can use the code as you wish, I added an MIT license to the repo. |
I knew it'll be faster 🤣 |
Now that I'm reading mentioned discussion, I would like to add that I definitely implemented this using the approach described in the mentioned paper (Boik & Robinson-Cox 1998). As far as I remember, I did not 'translate' the code but implemented the described numerical method 'independently', based on their mathematical description (in fact I'm pretty sure I have never seen S-plus code in my life). However, I don't know what that would mean license-wise and whether it's OK to use an MIT license (I have never had to think about those kind of issues before). Any insights on this? |
I would test a much larger range of a,b values. Look at the code for calculation of the incomplete beta in this repo. They use different algorithms depending on the domain. |
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.
All the nested functions, multiple assignments on a single line and generally lack of comments make it quite challenging to follow the code. Maybe the implementation should be cleaned up a bit before it should be considered for inclusion in SpecialFunctions.
So, I have :
A slightly potentially-problematic outcome : the tests of the new chainrule alone last for ~40s on my machine, which is a large part of the total test time of the package :/ Ready for review round 2. |
Summary
This PR adds ChainRules coverage for the regularized incomplete beta function and its inverse:
frule
/rrule
for:beta_inc(a, b, x) -> (p, q)
beta_inc(a, b, x, y)
withy = 1 − x
semanticsbeta_inc_inv(a, b, p) -> (x, 1 − x)
test/chainrules.jl
.I did that by translating the S+ algorithm of Boik & Robinson-Cox (1998) for the partial derivatives w.r.t.
a
,b
,x
.Motivation
beta_inc
andbeta_inc_inv
.beta_inv
andbeta_inv_inc
gradients. #505, there a lot of places where these derivatives are missing:Distributions.MvTDist()
, which was never fittable due to the lack of these derivatives.Copulas.jl
, when usingDistributions.Beta()
marginalsI definitely think these derivatives belong to
SpecialFunctions.jl
'sChainRule
's extension.Final note
The reference of the algo is :
Maybe we should have it as a proper reference in some documentation somewhere ? Dont know.
The algorithm is quite stable, and has good precision (see tests
rtol
). It works forFloat64
andFloat32
. I do not think I broke anything so this should be patch-level change.Waiting for review :)