-
-
Notifications
You must be signed in to change notification settings - Fork 45
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
Parallel reduce_sum #451
Parallel reduce_sum #451
Conversation
This model compiles in cmdstan for now:
The other model I tried to compile was this one https://gist.github.com/rok-cesnovar/832f9752fc2a8ba7ae7b3da3eb5f2743 but that spits out a bunch for ambigious functions warnings that I currently dont understand yet, example shown in the gist comment. |
v0.1 stanc3 with parallel_reduce binary for linux if anyone wants to test (unzip and move binary in cmdstan). |
OOoooooon it! |
I got:
When I tried to run the binary. How hard is it to build stanc? |
It does take some installing of all the required ocaml, dune, etc. But after you have that its a piece of cake. Weird that it doesnt run. I will check how Jenkins builds it for the release and we can try again. |
Please try this. I think this should run. |
I goofed the original functor arguments, it doesnt hurt the your example with normal. Cleaned that up now. The hierachiral reduce models still doesnt compile in c++ tho. stanc_parallel_reduce_sum_v0.2.zip p.s.: dont get your hopes up too much. Learning on the job :) |
Yeah I got a basic normal running. Those errors you posted look like the result of me not defining the count_var_impl_ correctly -- I hit those in the ODE stuff and can fix em' easy. Edit: Of course, it could be something else |
Alright I think it's working for a basic normal example for me :D. I'll see if I can get the poisson thing to compile. Yo Bob said the print function would have the variadic argument stuff in the stanc3 compiler. Is that something you can copy-paste over to this? |
parallel_reduce_test_model compiles for me. |
I posted some code over here: https://gist.github.com/rok-cesnovar/832f9752fc2a8ba7ae7b3da3eb5f2743 You might be able to add something like |
I got the same errors you did on gcc and added the require_not_ts and it worked. |
Yeah, tried looking at that, but print isnt implemented as the rest of them so not trivial for me. Might be for the real gurus :) Will try if this brute force approach looks promising.
Nice!
Thanks, saw the comment yeah. I am using g++ 8.3. Just switched to clang and that works and that is fine with me :) |
I'll push the changes to math. The example model is running for me and I'm getting different results for STAN_NUM_THREADS=8 and STAN_NUM_THREADS=1 so debug time wooo! |
Oh wait. It generates data randomly. Lemme generate the data in R. I might be lying. |
Alright with generated data Rhat = 1 so they're doing the same thing. I'm gonna go fix the indexing bug with start/end. |
@nhuurre Thanks again! I think I resolved all the comments apart from the one about functors where I have a question. @bbbales2 I updated the test model with your changes. The updated error message is now:
|
Just one thing: The error message says as a first line:
, but what about the signature
? Did I miss that? I mean, it's possible to instantiate this thing with an array of a scalar ( I am really looking forward to checkout BTW... for the future we could think about handling sums of
in an efficient way. That would be rat to do at some point. I got this thought when looking at the review comments here. |
Ah, it was an off-by-one error in printing the error message (vector of 1 dimension means zero commas not 1 :) ). Fixed now. Thanks for pointing that out. Once this passes I will start another build of the binaries. |
Place
|
For me this looks all great now. |
Just ran again one of my test models on this - speeds up nicely. |
The deadline to get this in for 2.23 is end of monday anywhere on earth. Does anyone have any more comments or requests? Would love to see this get in. But obviously not gonna pressure if anyone objects. Thanks! |
Indeed, it would be awesome to have this land in 2.23... Tagging @nhuurre , @rybern and @seantalts ... hopefully someone got time to look at this. Happy easter! |
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.
Looks good to me. I have a couple of suggestions but they're just optional style changes.
Thanks @nhuurre ! Your comments were are very much on point. I addressed all of them. Really excited to get this in and see it in action. |
This is a draft and a very brute force of making a prototype for parallel reduce_sum.
Any and all comments are welcome.
The main things is that I now duplicate the functors, the duplicate has the pstream as the third argument.
The other thing is supporting functions with variadic arguments.
... can be 0 to N arguments of any type.
For now I just brute force my way to support 0 to 3 arguments of all base types and their 1D arrays.