Parallel Adaptive Nuts #3033
Parallel Adaptive Nuts #3033
Conversation
…4.1 (tags/RELEASE_600/final)
…4.1 (tags/RELEASE_600/final)
…4.1 (tags/RELEASE_600/final)
…4.1 (tags/RELEASE_600/final)
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
| Model& model, const stan::io::var_context& init, | ||
| const stan::io::var_context& init_inv_metric, unsigned int random_seed, | ||
| Model& model, const InitContext& init, | ||
| const InitMetricContext& init_inv_metric, unsigned int random_seed, |
There was a problem hiding this comment.
Are the types changing here? I don't see an advantage to templating this. Now the template hides even the base polymorphic type.
There was a problem hiding this comment.
Ah they are! Notice that now when an initial inverse metric is not given we actually return back an stan::io::dump (or vector of). This is nice for the compiler because now it knows the real type coming in so devirtualization happens a lot easier.
There was a problem hiding this comment.
I don't like it. It's so hard to read code when the types info disappears like here. I doubt the virtual function calls here are killing our performance. Both of these things are used once at initialization and that is it.
There was a problem hiding this comment.
Is a var_context more informative? It's just an abstract base class so you still need to go look at the callee to see what's happening when this objects member functions are called. What if I included in the docs that this is either going to be a stan::io::dump or stan::io::json_context etc.?
There was a problem hiding this comment.
Is a var_context more informative
Well it's still easier to search for (you can find that it's an abstract base class, and then you can find what inherits from it and whatnot) and this is how everything is kinda done in the rest of services.
I think the reason we'd go templating here is if we were doing something we couldn't just solve with the existing polymorphism, or it's just easier to code the templated thing.
If that's the case, let's do templating. If that's not the case, let's do templating it if you really want to. Not the biggest deal but I prefer the existing stuff.
| } catch (const std::domain_error& e) { | ||
| return error_codes::CONFIG; | ||
| } | ||
| util::run_adaptive_sampler(samplers, model, cont_vectors, num_warmup, |
There was a problem hiding this comment.
Similarly to the other code, I don't like how the 1 chain and N chain implementations are separate. It seems like they could be the same code and the 1 chain thing would just have 1 chain and that's fine.
There was a problem hiding this comment.
We have two right now mostly because of cmdstan. Once we add the parallel version to cmdstan we can remove 1 chain version
|
@bbbales2 I think this PR should have everything in it that we need to do the cmdstan stuff. Once we think the |
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
…4.1 (tags/RELEASE_600/final)
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
|
The ball is in your court, right? |
|
Lol oh I was waiting on you, is there something in the review I missed? |
|
Sorry.. no, you did not miss things... I missed your update. The last thing to sort out is the chain id output labeling. See above. Once we got that, then I need to check that pre-compilation of the services is still all doing its job. Quick to do by just timing the bernoulli example compile time. Then we are good from my view. |
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
|
Alrighty @wds15 I also updated stan-dev/cmdstan#987 with this branch + the change for the init being set to 1. I moved the |
|
So one of my last tests is to compare the compile time with 2.27.0 is: and with this branch I got So possibly this branch does slow down things a tiny bit wrt to compilation (upon the second time to be clear), but I'd assume this is within noise. |
|
Yeah imo I'd expect possibly a tiny slowdown in compilation, but overall that seems to be fine. You think this is ready to merge? |
|
From my memory this is good now. Let me do one last round over this. This is my first bigger PR review for Stan, so that's why I take a bit more time. I'd guess we are good. |
| * @param[in] random_seed random seed for the random number generator | ||
| * @param[in] init_chain_id first chain id. The pseudo random number generator | ||
| will advance by for each chain by an integer sequence from `init_chain_id` to | ||
| `num_chains` |
There was a problem hiding this comment.
the last chain id won't be num_chains, but init_chain_id+num_chains-1
| @@ -19,14 +19,17 @@ namespace util { | |||
| * duplicated. | |||
| * | |||
| * @param[in] seed the random seed | |||
| * @param[in] chain the chain id | |||
| * @param[in] init_chain_id the chain id | |||
| * @param[in] chain_num For multi-chain, the ch | |||
There was a problem hiding this comment.
But the doc "For multi-chain, the ch" is not meaningful to me. What about
@param[in] init_chain_id start of chain ids
@param[in] chain_num chain id offset such that chain_id is init_chain_id+chain_num
Also... reading the comment here suggest to me that we should actually not allow for a chain id of 0, right??? (I mean the comment to the function)
| stepsize, stepsize_jitter, max_depth, delta, gamma, kappa, t0, | ||
| init_buffer, term_buffer, window, interrupt, logger, init_writer[0], | ||
| sample_writer[0], diagnostic_writer[0]); | ||
| } else { |
There was a problem hiding this comment.
The else branch is not needed. A simple if for the num_chains=1 case is sufficient (same for the other functions).
| interrupt, logger, init_writer[0], sample_writer[0], | ||
| diagnostic_writer[0]); | ||
| } else { | ||
| std::vector<std::unique_ptr<stan::io::dump>> unit_e_metrics; |
| * @param[in] random_seed random seed for the random number generator | ||
| * @param[in] init_chain_id first chain id. The pseudo random number generator | ||
| will advance by for each chain by an integer sequence from `init_chain_id` to | ||
| `num_chains` |
There was a problem hiding this comment.
doc needs correction as before. The range is init_chain_id to init_chain_id+num_chains-1
| * @param[in,out] sample_writer std vector of Writers for draws of each chain. | ||
| * @param[in,out] diagnostic_writer std vector of Writers for diagnostic | ||
| information of each chain. | ||
| * @param[in] num_chains The number of chains to run in parallel. `init`, |
There was a problem hiding this comment.
num_chains doc string should appear after Model so that it matches the order of the arguments.
| init_buffer, term_buffer, window, interrupt, logger, init_writer[0], | ||
| sample_writer[0], diagnostic_writer[0]); | ||
| } else { | ||
| using sample_t = stan::mcmc::adapt_diag_e_nuts<Model, boost::ecuyer1988>; |
| @@ -19,14 +19,17 @@ namespace util { | |||
| * duplicated. | |||
| * | |||
| * @param[in] seed the random seed | |||
| * @param[in] chain the chain id | |||
| * @param[in] init_chain_id the chain id | |||
| * @param[in] chain_num For multi-chain, the ch | |||
There was a problem hiding this comment.
BTW, do we actually need to have create_rng take a init_chain_id and a num_chains? Why don't we leave the function as is and just pre-compute the chain_id's in the parallel sample functions? That seems simpler to me.
| interrupt, logger, init_writer[0], sample_writer[0], | ||
| diagnostic_writer[0]); | ||
| } else { | ||
| std::vector<std::unique_ptr<stan::io::dump>> unit_e_metrics; |
| @@ -66,7 +71,7 @@ void run_adaptive_sampler(Sampler& sampler, Model& model, | |||
| auto start_warm = std::chrono::steady_clock::now(); | |||
| util::generate_transitions(sampler, num_warmup, 0, num_warmup + num_samples, | |||
| num_thin, refresh, save_warmup, true, writer, s, | |||
| model, rng, interrupt, logger); | |||
| model, rng, interrupt, logger, chain_id, n_chain); | |||
There was a problem hiding this comment.
Sorry for the late catch...but why is it called n_chain here and not num_chain? num_chain looks more consistent with all the other variable conventions...would need to be addressed in the entire PR.
…removes unneeded else branch
…4.1 (tags/RELEASE_600/final)
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
|
It's on me again? |
|
Yep go for it! |
|
@wds15 bump! |
|
Monday…sorry… |
|
All good! |
|
LGTM! |
Submission Checklist
./runTests.py src/test/unitmake cpplintSummary
Okay so this PR has the cmdstan facing version of
hmc_nuts_diag_e_adapt()(and I plan to also do dense in this PR). The signature differs from the originalhmc_nuts_diag_e_adapt()in that theinit,init_inv_metric,sample_writer, anddiagnostic_writerare std::vector<>`'s of what the normal signature is.My suggestion is that for cmdstan we should have all of these be std::vectors when they are created. Then for the algorithms that are not parallel yet we set
vec_argument[0]as the input argument for the algorithm. That will let us get diag/dense matric adaptive nuts into cmdstan and then start adding parallel versions of all the other algorithms. Does that work for everyone?Intended Effect
Allow multiple chains to be invoked for adaptive nuts with diag/dense metrics
How to Verify
Test added for parallel diag e adapt
Side Effects
Documentation
Still need to add docs
Copyright and Licensing
Please list the copyright holder for the work you are submitting (this will be you or your assignee, such as a university or company): Steve Bronder
By submitting this pull request, the copyright holder is agreeing to license the submitted work under the following licenses: