Skip to content
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

[FR] Use new num_chains arg in cmdstan for hmc, fixed params, and generated quantities #534

Open
SteveBronder opened this issue Aug 13, 2021 · 13 comments
Labels
feature New feature or request

Comments

@SteveBronder
Copy link
Collaborator

Is your feature request related to a problem? Please describe.

So once stan-dev/cmdstan#987 goes into cmdstan, as long as the user compiled with threading on then we should be able to run multiple chains within one Stan program by specifying num_chains inside of the sample argument!

Describe the solution you'd like

Is there a reasonable line for cmdstanr to check if the method is hmc with dense or diag_e inverse metric and then when the user specifies parallel chains, internally cmdstanr passes those as command args to cmdstan instead of using R level parallelism? This should save folks a lot of memory and likewise give a nice little speed bump.

This would also have to wait / check that cmdstanr is at least greater than version 2.27 and has compiled with threading on

Describe alternatives you've considered

I'm not familiar enough with the internals of cmdstanr to know other alts, though I'm happy to discuss whatever can work for yinz!

Additional context
Add any other context about the feature request here.

@SteveBronder SteveBronder added the feature New feature or request label Aug 13, 2021
@rok-cesnovar
Copy link
Member

@SteveBronder Thanks! I set the milestone for after the 2.27++ release.

Yeah, I think this will just be a special case if version > 2.27, STAN_THREADS is on and the appropriate methods are used then just run a single executable instead of running multiple and waiting for feedback (https://github.com/stan-dev/cmdstanr/blob/master/R/run.R#L344).

So this will work with sampling for both hmc and nuts and for both dense and diag inverse metric?

@SteveBronder
Copy link
Collaborator Author

So this will work with sampling for both hmc and nuts and for both dense and diag inverse metric?

It's just nuts with the dense and diag metric. Once I have all the cmdstan stuff in for nuts then I'm going to go back to the Stan repo and add it for hmc and all the other stuff in sampling

@rok-cesnovar
Copy link
Member

Yeah, no rush. Was just trying to double check. These 2 are definitely the most used so no rush for the rest.

@SteveBronder
Copy link
Collaborator Author

Just to update this is in now! Should we wait for cmdstan 2.27+? I think if folks are using an older version of cmdstan with multiple threads we would have to do this check anyway?

@rok-cesnovar
Copy link
Member

rok-cesnovar commented Aug 26, 2021

I am going to start playing with this and add it soon-ish, just because I am excited for the feature.

For these kind of new features we always check for versions anyway, so we will definitely only use this with 2.28+
The only problem will be checking the version with a cloned cmdstan (before the next release), because we leave the Github version 2.x.0 and its hard to distinguish. But there will be a release in a few weeks anyway.

@SteveBronder
Copy link
Collaborator Author

^wonder if once we do a release we should bump devel up to like 2.x.x.1 just to be like "this is not 2.x.x"

@rok-cesnovar
Copy link
Member

Yeah, I proposed awhile back (here: https://discourse.mc-stan.org/t/versioning-after-release/19803) something along those lines and I am not sure if I didnt make myself clear enough or others just werent for it.

For CmdStan actually bumping to 2.X.9000 or 2.X.9999 is actually all we need. Because after a single feature is added that code wont ever be released as 2.X.1 or anything the like, it can only be released as 2.X+1.0 or 3.0.

@SteveBronder
Copy link
Collaborator Author

^I'll do this next week

@rok-cesnovar
Copy link
Member

Blocked by stan-dev/cmdstan#1058 (should be fixed fast and accompanied by a 2.28.2 release).

@SteveBronder
Copy link
Collaborator Author

Is this still blocked? @rok-cesnovar if you don't have an impl yet would it be a lot easier if all the NUTS cmdstan calls could use the same num_threads scheme? I think for cmdstanr we only need NUTS with and without adaptation

@rok-cesnovar
Copy link
Member

Not blocked. I got distracted and forgot…

I have a semi done branch. Will finish it up this weekend.

@SteveBronder SteveBronder changed the title [FR] Use new num_chains arg in cmdstan for dense_e and diag_e [FR] Use new num_chains arg in cmdstan for hmc, fixed params, and generated quantities Jul 26, 2023
@SteveBronder
Copy link
Collaborator Author

Update on this, after stan-dev/cmdstan#1176 is merged into cmdstan all of hmc, fixed params, and generated quantites can use the num_chains argument. This should simplify some of the higher level code and is also more performant because instead of making N copies of data / model for each chain we just have one data/model that is shared across all chains

@jgabry
Copy link
Member

jgabry commented Jul 26, 2023

Cool, thanks @SteveBronder

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants