-
-
Notifications
You must be signed in to change notification settings - Fork 189
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
Incomplete Beta Function Inverse #2637
Conversation
Wow, so fast, thanks @andrjohns! Do you mind if I put in the latex formulas for the partials in the rev file documentation? |
Ah good point forgot about those! Yeah feel free to add them, otherwise I can do it tonight/tomorrow |
@serban-nicusor-toptal it looks like the downstream tests for this are failing due to some odd git errors, would you mind having a look when you get a minute? |
Hey, checking! Edit: Should be fine now. |
Thanks for sorting this so quickly! |
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
I don't feel comfortable reviewing the cpp but the derivatives and tests look good to me. Maybe @SteveBronder can take a look? |
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
will it be possible to get this in to the 2.29 release? We can then do |
@serban-nicusor-toptal looks like the jenkins for this one is having trouble with
|
Can you try merging in the latest develop, there were quite a few changes in the Jenkinsfile and test machine since then. Hopefully that will help. |
Ah thought I had but it hadn't triggered a new CI run for some reason |
@rok-cesnovar or @SteveBronder would either of you have a minute to review this one? @spinkney has signed off on the math, so it's just the c++ to be checked |
I can do it today. We wont be able to merge after we do the patch release (stan-dev/cmdstan#1086) which should be in a day or two. |
Thanks! There's no rush at all on this, so just whenever you get the time |
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.
Looks great. I only have one question in two places.
@rok-cesnovar This should be all ready for another look |
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.
Good to go! Thanks Andrew!
Thanks! |
Summary
This PR adds the inverse of the incomplete beta function, will be needed for some of the quantile/icdf functions to come.
Tests
Individual
rev/fwd/mix
tests have been added, as the gradients use theF32
hypergeometric series and the autodiff finite-differencing tests are not stable.Side Effects
The
F32
templating has been relaxed to allow each of the inputs to be different typesRelease notes
Add the incomplete beta function inverse
Checklist
Math issue Incomplete Beta Inverse #2513
Copyright holder: Andrew Johnson
The copyright holder is typically you or your assignee, such as a university or company. By submitting this pull request, the copyright holder is agreeing to the license the submitted work under the following licenses:
- Code: BSD 3-clause (https://opensource.org/licenses/BSD-3-Clause)
- Documentation: CC-BY 4.0 (https://creativecommons.org/licenses/by/4.0/)
the basic tests are passing
./runTests.py test/unit
)make test-headers
)make test-math-dependencies
)make doxygen
)make cpplint
)the code is written in idiomatic C++ and changes are documented in the doxygen
the new changes are tested