Update integrate_1d to use variadic autodiff stuff internally in preparation for closures #2397
Conversation
…4.1 (tags/RELEASE_600/final)
|
@nhuurre this doesn't change the integrate_1d external interface but it should make it easy to hook up in the closure pull. |
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
|
Is this waiting for a review? |
|
@SteveBronder yeah have at it. It's some changes to make it easy to do closures with integrate_1d. |
|
Binds removed! |
There was a problem hiding this comment.
Couple small comments overall code looks good!
One thing, I think we can use a double nested reverse pass to avoid copying the args every iteration. Can you take a look at the code here to see if it makes sense? It's passing all the tests but I'm not super familiar with nested autodiff so I'm not sure if this would break in some edge case we are not testing right now
| @@ -57,6 +58,8 @@ inline double integrate(const F& f, double a, double b, | |||
| bool used_two_integrals = false; | |||
| size_t levels; | |||
| double Q = 0.0; | |||
| // if a or b is infinite, set xc argument to NaN (see docs above for user | |||
| // function for xc info) | |||
There was a problem hiding this comment.
[optional] We can do this some other time but it would be nice to make the errors in the if (used_two_integrals) into a function that's just called in the places we use two integrals
There was a problem hiding this comment.
I split these into lambda functions. That look better or is used_two_integrals clearer?
| apply( | ||
| [&](auto &&... args) { | ||
| accumulate_adjoints(adjoints.data(), | ||
| std::forward<decltype(args)>(args)...); | ||
| }, | ||
| std::move(args_tuple_local_copy)); | ||
|
|
||
| gradient = adjoints.coeff(n); |
There was a problem hiding this comment.
I forgot to make this comment in my review, but optionally we can definitely figure out which arg has the adjoint value we need here in a clever way that doesn't require copying all of them. We could have some function to get the Nth var in a tuple but I'm also fine with not doing that in this PR
There was a problem hiding this comment.
We could have some internal function like
template <typename... Args>
double get_nth_adjoint(size_t n, const std::tuple<Args...>& tuple_arg) {
size_t accum_vars = 0;
bool stop_checking = false;
// for_each goes off from left to right
std::array<double, sizeof...(Args)> possible_adjs =
for_each([&accum_vars, &stop_checking](auto&& arg){
if (stop_checking) return 0.0;
size_t num_vars = count_vars(arg);
// Need to keep moving along
if ((accum_vars + num_vars) < nth || stop_checking) {
accum_vars += num_vars;
return 0.0;
} else { // We reached the first arg that passes
stop_checking = true;
// I'm tired but do the logic here to get the nth value from that particular arg
return get_the_adj(arg, some_index_calculation);
}
}, tuple_arg);
// Loop over possible_adjs until we hit a nonzero value (or they are all zero and return a zero)
return get_nonzero_value(possible_adjs);
};There was a problem hiding this comment.
Yeah it's awkward. I also want to leave it for now.
The way to speed this up is writing our own quadratures or talking to the Boost people about an interface where we integrate multiple functions on the same domain together. Now we compute all three of these integrals totally separately:
\int f(x, a, b) dx
\int df(x, a, b)/da
\int df(x, a, b)/db
But anytime we compute df(x, a, b)/da we also get df(x, a, b)/db, and so the efficiency gains would be taking advantage of that (and what we're doing here is throwing away a ton of gradient info).
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
Co-authored-by: Steve Bronder <[email protected]>
Co-authored-by: Steve Bronder <[email protected]>
Co-authored-by: Steve Bronder <[email protected]>
…ath into feature/variadic-integrate-1d
…to feature/variadic-integrate-1d
I think this is right. I didn't like doing a weird thing with nested autodiff across functions like that, so I just put the contents of |
…4.1 (tags/RELEASE_600/final)
…p instead of copying all the adjoints
There was a problem hiding this comment.
I think it looks good! I had a few small comments I just put into a PR that you can merge if you like. I also figured out how to do the thing where we only grab the one adjoint we need instead of making a vector and copying all the adjoints into it each iteration.
feature/variadic-integrate-1d...review/integrate-1d-variadic-2
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
…into feature/variadic-integrate-1d
|
@SteveBronder I merged the stuff. There was another way to do the nth adjoint thing that was less code so I went with that so we should be fast |
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
Summary
This should make it easy to hook up integrate_1d with closures (#2384)
Release notes
Checklist
Math issue Implement closures #2197
Copyright holder: Columbia University
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