-
-
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
Re-Implementation of Hypergeometric PFQ gradient function #2961
Conversation
@spinkney would you be able to have a look at this when you get a minute? Once this is in I'll be able to start adding the rest of the hypergeometric functions from Boost |
This is great news! I may have some time tomorrow to look at this. Can you add some documentation about the simplified way the derivatives are calculated? |
Thanks! Doc added. It's essentially taking advantage of the fact that we'll always have the calculated value of the hypergeometric_pfq function when we need the gradients, so we can use this definition: https://functions.wolfram.com/HypergeometricFunctions/HypergeometricPFQ/20/01/01/0001/ Then because of the digamma recurrence relationship, we can cancel out the |
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.
@andrjohns, can you annotate the code to make the review easier?
Some general things that would help immensely:
- replace
auto
with proper types when it's straightforward. It's being overused and making it absolutely hard to read. - explain some of the bigger blocks that changed.
- explain some of the name changes. I'm seeing minor differences and I'm not sure if there are other non-C++ idioms bleeding into this PR. For example,
calc_z
->CalcZ
? I'm not saying it's better or worse, just want to know what the motivation is and whether it's consistent.
stan/math/prim/fun/grad_pFq.hpp
Outdated
|
||
namespace stan { | ||
namespace math { | ||
namespace internal { | ||
template <typename T> | ||
inline auto binarysign(const T& x) { |
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.
- Don't we have a
sign
function? - This function shouldn't live here.
Jenkins Console Log Machine informationNo LSB modules are available. Distributor ID: Ubuntu Description: Ubuntu 20.04.3 LTS Release: 20.04 Codename: focalCPU: G++: Clang: |
Returning to this PR since there's been some demand for hypergeometric functions on the forums
|
Jenkins Console Log Machine informationNo LSB modules are available. Distributor ID: Ubuntu Description: Ubuntu 20.04.3 LTS Release: 20.04 Codename: focalCPU: G++: Clang: |
Thanks for the updates!
Here's where it stands:
|
Great, thanks! Just to note about the math, the function was already used for gradient calculations in the |
Jenkins Console Log Machine informationNo LSB modules are available. Distributor ID: Ubuntu Description: Ubuntu 20.04.3 LTS Release: 20.04 Codename: focalCPU: G++: Clang: |
@syclik small ping |
@syclik can we look at merging this soon? I've got a few functions that depend on it |
@andrjohns, yes. Will take a look tomorrow (eastern time). Thanks for keeping on top of it. |
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.
@andrjohns, really minor comments, mostly about the latex.
Once that's done, let's merge! I think the code looks fine. No suggestions for change there (other than the one comment about return type).
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.
LGTM!
@andrjohns, looks great! Please merge when tests are done. (If I see it first, I'll go ahead and merge.) |
Jenkins Console Log Machine informationNo LSB modules are available. Distributor ID: Ubuntu Description: Ubuntu 20.04.3 LTS Release: 20.04 Codename: focalCPU: G++: Clang: |
@andrjohns is the idea to expose the Hypergeometric PFQ functions in stanc3? I saw someone request it on the forums. I can get that up and running if so |
Summary
This PR rewrites the
grad_pFq
function to use a significantly simpler form of the gradient definition (now requiring only one infinite sum instead of two nested ones). Thegrad_pFq
function now performs on par with the existinggrad_2F1
andgrad_3F2
functions in terms of both speed and accuracy.The gradient calculations are also now amenable to the autodiff testing framework, and so new
mix
tests have been added.Tests
Existing tests for equality with
grad_2F1
andgrad_3F2
should still pass. Additionalmix
tests using the autodiff testing framework have been added.Side Effects
N/A
Release notes
Gradients for the
hypergeometric_pFq
function rewritten for increased speed and stabilityChecklist
Math issue #(issue number)
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