Adds auxilary functions needed for reduce_sum #1800
Conversation
|
Ooops, missed this. Thanks for the ping! I'll get on it tonight! |
|
Oh wait I just wrote this code and the tests lol. @wds15 can you do the review? |
|
Ok...I should be able to review. |
|
@wds15 I should probably be the one to finish up the reduce_sum tests, Steve can manage the pull, and you can review? I'll try to get that done before the Stan meeting tomorrow. I didn't see any sign of a Math meeting. |
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
| storage.setZero(); | ||
| double* ptr = stan::math::accumulate_adjoints(storage.data(), arg); | ||
|
|
||
| size_t num_vars = stan::math::count_vars(arg); |
There was a problem hiding this comment.
In the sense of a unit test, I would prefer it to not use count_vars here.
There was a problem hiding this comment.
same comment to the other instances.
| size_t num_vars = stan::math::count_vars(arg); | ||
|
|
||
| for (int i = 0; i < num_vars; ++i) | ||
| EXPECT_FLOAT_EQ(storage(i), i + 1.0); |
There was a problem hiding this comment.
The expectation should just be 1 here, no?
There was a problem hiding this comment.
Yep! in this loop i only ever equals zero so I fixed that up here and in the other tests where that happens
| EXPECT_FLOAT_EQ(storage(i), 0.0); | ||
|
|
||
| EXPECT_EQ(ptr, storage.data() + num_vars); | ||
| } |
There was a problem hiding this comment.
I think it would be good to finish every test with recover_memory() in order to wipe the AD tape. This will be important if we later do some big testing binaries with all tests in one file.
| EXPECT_FLOAT_EQ(storage(i), 0.0); | ||
|
|
||
| EXPECT_EQ(ptr, storage.data() + num_vars); | ||
| } |
There was a problem hiding this comment.
Can u add a test for the accumulate_adjoints function which terminates the recursion we use for the parameter packs? Thanks.
There was a problem hiding this comment.
Do you just mean checking the end of the recursion method like
TEST(AgradRev_accumulate_adjoints, zero_args) {
Eigen::VectorXd storage = Eigen::VectorXd::Zero(1000);
double* ptr = stan::math::accumulate_adjoints(storage.data());
for (int i = 0; i < storage.size(); ++i) {
EXPECT_FLOAT_EQ(storage(i), 0.0);
}
EXPECT_EQ(ptr, storage.data());
}
There was a problem hiding this comment.
That looks good to me. We want to make sure it returns the pointer it got, and at least hope it didn't do anything to the data there.
| decltype(stan::math::deep_copy_vars(arg)) out | ||
| = stan::math::deep_copy_vars(arg); | ||
|
|
||
| for (int i = 0; i < arg.size(); ++i) |
There was a problem hiding this comment.
These tests are actually not sufficient if we get the deep copies as we need them. The point about deep copies is that the vars are not linked to one another. To test this, I would suggest to make a test which follows how we use this. This is, we have some vars, then start a nested AD region, there we do a deep copy, call grad, check grads are fine (adjoints are non-zero), then tear it down, and then check that the adjoints of the outer vars are still zero. That is why we do this function and this is what we should test it for, I think. Does what I say make sense to you?
| namespace stan { | ||
| namespace math { | ||
|
|
||
| template <typename... Pargs> |
There was a problem hiding this comment.
Can we add somewhere a hint as to how the "..." processing works? I am not sure if we need to repeat this for all utilities, but I would appreciate a short explanation of the "peel off" logic for the ... argument pack. I don't know where to put this, as this is something which is not specific to a given function... maybe to the function which ends the recursion?
There was a problem hiding this comment.
We could include a comment on the last one with a link to an article explaining variadic parameter packs, how do you feel about that? We could also have a module for reduce_sum so on the site there's a section that explains the more complex parts of the reduce sum implimentation
There was a problem hiding this comment.
By module I mean a section like we have for the requires stuff
There was a problem hiding this comment.
Sounds great to me to have such a module.
| Eigen::RowVectorXd arg = Eigen::RowVectorXd::Ones(5); | ||
|
|
||
| decltype(stan::math::deep_copy_vars(arg)) out | ||
| = stan::math::deep_copy_vars(arg); |
There was a problem hiding this comment.
These tests don't look right for save_varis.... maybe some meshup happened?
| 1 + 5 + 3 + 4 + 15 + 2 * 5 + 2 * 3 + 2 * 4 + 30, | ||
| count_vars(arg1, arg18, arg17, arg2, arg16, arg3, arg15, arg4, arg14, | ||
| arg5, arg13, arg12, arg6, arg11, arg7, arg10, arg8, arg9)); | ||
| } |
There was a problem hiding this comment.
Here as well, we should also test the case of no arguments returning 0.. so count_vars() == 0, right?
|
@bbbales2 are you handling the tests here or should I go at the above? |
|
@SteveBronder I have a plan for deep copy. The other things have at em'. There should be save_varis test in the reduce_sum WIP pull. |
|
@bbbales2 do you think it makes sense to do |
…xp1~20180509124008.99 (branches/release_50)
…stable/2017-11-14)
I guess I have no opinion really. Maybe it makes docs clearer cause then there's only one entry point? Either way is good. |
|
I made the changes to the deep_copy_vars tests. Hopefully they're good now. |
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
|
@wds15 this is ready for review! |
| namespace stan { | ||
| namespace math { | ||
|
|
||
| template <typename... Pargs> |
There was a problem hiding this comment.
Sounds great to me to have such a module.
| using stan::math::var; | ||
| using stan::math::vari; | ||
|
|
||
| TEST(AgradRev_save_varis, int_arg) { |
There was a problem hiding this comment.
I think the recursion ending function is not tested for save_varis. Can u add that please?
|
|
||
| template <typename VecVar, require_std_vector_vt<is_var, VecVar>* = nullptr, | ||
| typename... Pargs> | ||
| inline size_t count_vars_impl(size_t count, VecVar&& x, Pargs&&... args); |
There was a problem hiding this comment.
Sorry for bringing this up somewhat late.... but shouldn't we place the "*_impl" stuff into an "internal" namespace? This applies to the other tools as well which we are writing with an impl naming.
There was a problem hiding this comment.
Oh... count_vars is the only one with a "impl" thing - for good reasons as here you have to initialise to zero. So, I think moving the "*_impl" functions into an internal namespace, makes sense to me. Right?
|
Gonna make these changes real quick |
Moved count_vars_impl to internal namespace Added empty tuple test for apply (for reduce_sum, design-doc #17)
…xp1~20180509124008.99 (branches/release_50)
|
@wds15 Ready to check again (if tests pass) Also what was "Sounds great to me to have such a module." pointing at? |
|
Steve suggested to add some documentation on the tuples argument processing magic as documentation to the modules doxygen section. |
|
Looks like we have issue with |
|
Same issue here: #1811 Maybe something odd was merged to develop or there are some issues with the Jenkins pipelines... no idea. @rok-cesnovar do you have a hint? Not sure who else to ping. |
|
Yes, this is the same issue that happened in #1804. Try restarting tests and hope the Linux machine does not get picked. See discussion #1804 (comment) Ben is on it, I was hoping #1810 would fix it, but it apparently needs some more work. My opinion is that we give Ben some more time to fix it, otherwise we revert. |
…stable/2017-11-14)
|
Thanks. I wasn't aware of this mess. Looks ugly, but let's see. Is the complex stuff what is causing the hiccup here? |
|
Yes, its from the complex PR. It fails on a specific Linux AWS EC2 machine. If that test runs elsewhere it's fine. Its an issue with overloads and standard libraries I believe. |
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
Summary
Adds the following functions needed for
reduce_sumaccumulate_adjoints: Iterates through a parameter pack and assigns any var object or containers of vars adjoint values into a contiguous block of memory.count_vars: Iterates over a parameter pack counting the number of var objects as well as vars in containersdeep_copy_vars: Iterates over a parameter pack while only copying the var objects and var containers.save_varisIterates over a paramater pack of var objects and containers storing the vari pointers into a contiguous block of memoryTests
Tests can be run with
Side Effects
None
Release notes
Checklist
Math issue [WIP] Parallel Prototype #1616
Copyright holder: Steve Bronder, Ben Bales, Sebastian Weber
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