adds accumulators for var and fwd #2535
Conversation
…4.1 (tags/RELEASE_600/final)
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: |
It says in the doxigen. It is faster for reverse mode since executing reverse pass of one sum vari is much faster than many add varis. I don't think it maters for prim, fwd or containers (as your benchmarks show directly summing is faster). It might also be worth checking how much it actually matters for rev scalars. If it does not, I would suggest completely removing the accumulator class. If it does, I suggest doing general implementation that stores just a scalar and adds everything to it and a rev specialization that keeps the same vector it is using now, but sums any containers first.
I would say just copying the code around is not the best solution. We should find the root cause (probably some includes missing or some cyclical includes) and fix that. |
|
@SteveBronder Sorry for nagging you with errors for WIP; let me know if I need to stop 😄 Is the following error related to this PR? It occurs when building stan/math/prim/fun/accumulator.hpp:46:10: error: request for member ‘push_back’ in ‘((stan::math::accumulator<double>*)this)->stan::math::accumulator<double>::buf_’, which is of non-class type
double’
46 | buf_.push_back(x);
| ~~~~~^~~~~~~~~
In file included from /usr/lib/R/library/StanHeaders/include/stan/math/prim/fun.hpp:5,
from /usr/lib/R/library/StanHeaders/include/stan/math/prim.hpp:14,
from /usr/lib/R/library/StanHeaders/include/src/stan/io/dump.hpp:7,
from /usr/lib/R/library/rstan/include/rstan/stan_fit.hpp:43,
from /usr/lib/R/library/rstan/include/rstan/rstaninc.hpp:4,
from stanExports_gamma.h:20,
from stanExports_gamma.cc:5:
/usr/lib/R/library/StanHeaders/include/stan/math/prim/fun/accumulator.hpp: In instantiation of ‘void stan::math::accumulator<T>::add(S) [with S = double; <template-parameter-2-2> = void; T = double]’:
stanExports_exp.h:182:23: required from ‘stan::scalar_type_t<T2> model_exp_namespace::model_exp::log_prob_impl(VecR&, VecI&, std::ostream*) const [with bool propto__ = false; bool jacobian__ = false; VecR = std::vector<double>; Vec
I = std::vector<int>; stan::require_vector_like_t<VecR>* <anonymous> = 0; stan::require_vector_like_vt<std::is_integral, VecI>* <anonymous> = 0; stan::scalar_type_t<T2> = double; std::ostream = std::basic_ostream<char>]’
stanExports_exp.h:378:49: required from ‘T__ model_exp_namespace::model_exp::log_prob(std::vector<T_l>&, std::vector<int>&, std::ostream*) const [with bool propto__ = false; bool jacobian__ = false; T__ = double; std::ostream = std
::basic_ostream<char>]’
/usr/lib/R/library/StanHeaders/include/src/stan/services/optimize/newton.hpp:55:47: required from ‘int stan::services::optimize::newton(Model&, const stan::io::var_context&, unsigned int, unsigned int, double, int, bool, stan::call
backs::interrupt&, stan::callbacks::logger&, stan::callbacks::writer&, stan::callbacks::writer&) [with Model = model_exp_namespace::model_exp]’
/usr/lib/R/library/rstan/include/rstan/stan_fit.hpp:502:41: required from ‘int rstan::{anonymous}::command(rstan::stan_args&, Model&, Rcpp::List&, const std::vector<long unsigned int>&, const std::vector<std::__cxx11::basic_string<
char> >&, RNG_t&) [with Model = model_exp_namespace::model_exp; RNG_t = boost::random::additive_combine_engine<boost::random::linear_congruential_engine<unsigned int, 40014, 0, 2147483563>, boost::random::linear_congruential_engine<u
nsigned int, 40692, 0, 2147483399> >; Rcpp::List = Rcpp::Vector<19>]’
/usr/lib/R/library/rstan/include/rstan/stan_fit.hpp:1215:18: required from ‘SEXPREC* rstan::stan_fit<Model, RNG_t>::call_sampler(SEXP) [with Model = model_exp_namespace::model_exp; RNG_t = boost::random::additive_combine_engine<boo
st::random::linear_congruential_engine<unsigned int, 40014, 0, 2147483563>, boost::random::linear_congruential_engine<unsigned int, 40692, 0, 2147483399> >; SEXP = SEXPREC*]’
stanExports_exp.cc:15:87: required from here
|
…4.1 (tags/RELEASE_600/final)
…4.1 (tags/RELEASE_600/final)
…4.1 (tags/RELEASE_600/final)
… into feature/varmat-accumulator
…4.1 (tags/RELEASE_600/final)
|
@t4c1 I failed miserably to sort out the includes issue. I went back to the specialization, but to sweeten the pot I added an actual var specialization that is a hair more than a duplicate along with a custom sum function. The results seem to be nice at small N, but any optimization here gets dominated by other stuff at large N This PRDevelop |
stan/math/fwd/fun/accumulator.hpp
Outdated
| * @tparam T Type of scalar added | ||
| */ | ||
| template <typename T> | ||
| class accumulator<fvar<T>> { |
There was a problem hiding this comment.
I still think we dont need a fwd specialization for accumulator. At worst we might need a .hpp file for it that just includes /fwd/fun/sum.hpp and prim/fun/accumulator.hpp
There was a problem hiding this comment.
So I tried removing this and it failed to compile. This is definitely an include issue but a pretty big one that I think should be tackled in a separate PR that's going to take some time to sort out. I'm going to make an issue about it today or Sunday
There was a problem hiding this comment.
I would agree with this reasoning if the issue was present in develop before this PR. But this PR introduces the issue of duplicated code, so I dont think we should merge it until the issue is resolved.
I am fine with resolving includes in a separate PR, but I would wait with merging of this PR until the includes are resolved.
There was a problem hiding this comment.
So I'm pretty sure this issue actually did exist before this PR but we didn't notice. The fundamental problem is in the godbolt below
https://godbolt.org/z/je89G573e
Before the class definition of accumulate we need to have the definition of sum() in order to compile with the var specialized sum. It was working before because one of the definitions of sum() in prim is
template <typename T>
inline T sum(const std::vector<T>& m) {
return std::accumulate(m.begin(), m.end(), T{0});
}Which will work for var, double, fvar, etc. We can have the include order anyway we want when we are just including functions, but when we include classes we need to make sure that the definitions that the class uses are available before the class is declared. We don't see this error very often because most things in Stan math are just functions (though this error did just pop up again with apply_scalar_unary where the definition for apply_scalar_unary<var> did not show up before prim's log() function.
I can take a shot at fixing it today, but it's going to require some fancy footwork and a good headache to make sure we get the includes correct. I'm worries this is going to take me a while to fix and this is stopping the new matrix stuff from working in the compiler so if your okay with it I'd prefer merging this PR in and then tacking this stuff in a seperate issue.
There was a problem hiding this comment.
We did have multiple issues with include order before, but we never duplicated classes just because of that. So far these issues could be solved by changing the order of includes.
Duplicating the code is a new problem introduced in this PR and I would like it resolved before this is merged.
Maybe I will be able to take some time to look at it today.
There was a problem hiding this comment.
^ if you can I'd totally appreciate it. I took another stab at the includes today and couldn't find an answer that wasn't funky.
…4.1 (tags/RELEASE_600/final)
|
@t4c1 with your changes stuff looks good! I moved the buffer size up to 128 as in benchmarking that seemed to help a bit though idt we want to be too greedy Develop |
…4.1 (tags/RELEASE_600/final)
|
@t4c1 I'm still seeing the error on Jenkins when it's doing the jumbo tests. I'm going to add back the fvar specialization |
|
@t4c1 I also had to put back the buffers for fwd and prim, I'm not sure why but at -O3 those were giving me really weird errors when we just did the accumulator. |
|
@serban-nicusor-toptal is one of the servers down? I'm seeing |
|
Hey @SteveBronder it's a low chance that we can sometimes lose a machine when being outbid so I think that may have happened there. I've restarted the job and gonna watch it until it's green. |
|
Awesome thanks! |
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
|
@SteveBronder I'm still getting the following error when compiling a model: stan/math/prim/fun/accumulator.hpp:41:10:
error: request for member ‘push_back’ in
‘((stan::math::accumulator<double>*)this)->stan::math::accumulator<double>::buf_’, which is of non-class type ‘double’
41 | buf_.push_back(x);
| ~~~~~^~~~~~~~~ |
|
@hsbadr can you make sure you are on this PR's most recent version? This PR uses |
My bad! Just wanted to comment before merging, after it's been approved. Updating my sources has fixed the error. Thanks! |
|
@t4c1 it looks like this is failing upstream on the stan opencl tests? Seems unrelated to this PR |
|
Yeah, it does not seem connected. I tried restarting the test and it consistently fails. However, I can not reproduce the error locally. |
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
Summary
This adds specializations for
varandfwdfor Stan'saccumulatorclass.For stan-dev/stanc3#898 I needed to add the ability for the accumulator to accept
var<Matrix>type inputs. But then I was looking at the impl and I didn't really understand why we kept a wholestd::vector<T>when this is used in the compiler to just accumulate on the joint log probability.While I was doing this I found that I was getting some really really weird include errors about how it was not using
stan::math::sum()forvarandfvartypes. I think this was a bug where in the process of ADL the compiler was not finding definitions forstan::math::sum()for containers offvarandvartypes before the definition ofaccumulate<T>. You can replicate these by deleting the specializations foraccumulatethis PR created infwdandrev.The major change for this PR is eagerly summing matrices and vectors and then adding their sum to the internal buffer. From the benchmark here this seems to be good. Below are the results from this PR and develop. @t4c1 I also added the stuff for
matrix_cl<>as we will need that as well in the compiler.The first bench for each is for adding vars individually and calling .sum() at the end. The second is for adding an eigen matrix of vars all at once.
This PR
Develop
So no change for accumulating a bunch of vars and a good speedup for accumulating matrices. Seems good!
Tests
Same tests as before, with additional tests for
Eigen::Matrix<var>,var_value<Matrix>,var_value<matrix_cl<double>>,std::vector<var_value<matrix_cl>>, andstd::vector` types.Side Effects
This is used in every single Stan program so we should be rather careful with it.
Release notes
Allow accumulator to accept
var<Matrix>matrix typesChecklist
Math issue How to add static matrix? #1805
Copyright holder: Steve Bronder
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