-
-
Notifications
You must be signed in to change notification settings - Fork 188
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
Framework for generic fvar<T> support through finite-differences #2929
Conversation
Putting this on my todo to review. |
@syclik any chance you've had a minute to take a look at this PR? |
Ah. I should have updated the thread last week. @SteveBronder said he'd review it. (I started looking and it'd take me a little longer to really review it properly.) |
Sorry for confused, happy to review! |
No worries all, thanks! |
@SteveBronder I realised that this would fail to compile in rstan since we don't package 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.
I need to look at the tests a little more but on a first pass this looks good! Just a few minor things
|
||
FvarInnerT rtn_grad = 0; | ||
// Use a fold-expression to aggregate tangents for input arguments | ||
(void)std::initializer_list<int>{(rtn_grad += internal::aggregate_tangent( |
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.
Will the compiler complain if we don't use static_cast<void>(...)
here?
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.
It hasn't so far, but happy to change if you prefer
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.
Yeah I'm pretty sure at some point compilers will complain about a C style cast here so a static cast would be better
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.
Ah yeah that's true, updated
promote_scalar_t<T, U> y; | ||
y.reserve(x.size()); |
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.
You didn't do this, but should these reserves be y.reserve(y.size() + x.size())
?
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.
Not in this case, since the aim is to return a container of the same size as x
@SteveBronder all passed, ready for another look when you get a minute |
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.
Few small docs changes but imo the code looks good
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!
Summary
This PR introduces a framework (
fvar_finite_diff
, open to name changes!) which can addfvar<T>
support for an arbitrary function through finite-differences.This is NOT intended as a replacement for analytic gradients or autodiff. Rather, for the functions in the math library which
fvar<T>
support is not feasible (or at least not trivial).This PR adds higher-order autodiff support to
integrate_1d
(andmix
tests using the testing framework) as an example.Currently, any use-cases which require higher-order derivatives (e.g., hessians) generally resort to finite-differencing the entire implementation. We can instead use finite-diffs only for those functions which aren't autodiff compatible, and use the existing autodiff for the rest. This should provide a decent speed-up for any implementations currently using finite-diff with many input arguments.
Tests
Accuracy/utility of implementation is tested through the added
integrate_1d
mix
testsSide Effects
N/A
Release notes
Added framework for generic higher-order autodiff support through finite-differences.
Checklist
Math issue Use finite-differences where forward-mode not implementable #2842
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