Bugfix/2241 rng model ctor

[mitzimorris]

Submisison Checklist

• Run unit tests: ./runTests.py src/test/unit
• Run cpplint: make cpplint
• Declare copyright holder and open-source license: see below

Summary

Stan model's 3-arg constructor takes unsigned int (RNG seed) instead of RNG object itself; change discard strategy for RNG inits accordingly.

Intended Effect

Move burden of choosing/instantiating the RNG object used by the model constructor from the interfaces to core Stan.

How to Verify

Rung unit test src/test/unit/lang/generator_test.cpp

Side Effects

The function stan/services/util/create_rng.hpp use the chain id directly as the stride (discard) instead of using (chain_id - 1). Interfaces should continue to enforce chain id > 0; the Stan model constructor calls the function create_rng using chain id 0.

Documentation

None

Reviewer Suggestions

Bob Carpenter or Sean Talts or Daniel Lee

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): Mitzi Morris

By submitting this pull request, the copyright holder is agreeing to 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/)

[seantalts]

minor comments, LGTM

[seantalts]

Where does base_rng__ end up getting used, out of curiosity? Can the user refer to base_rng__ or something in their transformed data section?

[bob-carpenter]

The user can call _rng functions in transformed data.

[seantalts]

no longer need parens around chain, but maybe more importantly do you know why there was a -1 / that you don't need it anymore? I'm curious at least, tho maybe doesn't matter - I think I see that it was breaking some tests sometimes, no?

[bob-carpenter]

The -1 was there before to make sure chain 1 (lowest legal value in the interfaces) didn't advance the PRNG. Now we're going to use the PRNG to generate transformed data and then advance chain 1 by 2^50 draws.

[seantalts]

I think you can do this and keep the existing semantics here:

-  std::stringstream output;
-  stan_model model(data_var_context, &output);
+  stan_model model(data_var_context, (std::stringstream*)0);

[bob-carpenter]

I fixed the merge conflict, but this now failing here on my local tests:

test/unit/services/sample/hmc_nuts_dense_inv_metric --gtest_output="xml:test/unit/services/sample/hmc_nuts_dense_inv_metric.xml"
Running main() from gtest_main.cc
[==========] Running 4 tests from 1 test case.
[----------] Global test environment set-up.
[----------] 4 tests from ServicesSampleHmcNutsDenseEMassMatrix
[ RUN      ] ServicesSampleHmcNutsDenseEMassMatrix.ident_no_adapt
[       OK ] ServicesSampleHmcNutsDenseEMassMatrix.ident_no_adapt (6 ms)
[ RUN      ] ServicesSampleHmcNutsDenseEMassMatrix.ident_adapt_250
src/test/unit/services/check_adaptation.hpp:63: Failure
The difference between param_vals[ij] and test::unit::stod(strs[j]) is 0.45182600000000006, which exceeds err_margin, where
param_vals[ij] evaluates to 1.2162200000000001,
test::unit::stod(strs[j]) evaluates to 0.76439400000000002, and
err_margin evaluates to 0.20000000000000001.
[  FAILED  ] ServicesSampleHmcNutsDenseEMassMatrix.ident_adapt_250 (103 ms)
[ RUN      ] ServicesSampleHmcNutsDenseEMassMatrix.use_metric_no_adapt
[       OK ] ServicesSampleHmcNutsDenseEMassMatrix.use_metric_no_adapt (1 ms)
[ RUN      ] ServicesSampleHmcNutsDenseEMassMatrix.use_metric_skip_adapt
[       OK ] ServicesSampleHmcNutsDenseEMassMatrix.use_metric_skip_adapt (1 ms)
[----------] 4 tests from ServicesSampleHmcNutsDenseEMassMatrix (111 ms total)

@mitzimorris and @betanalpha --- can you work this out?

[betanalpha]

Looks like the test assumed a particular input RNG seed which changes in this PR.

…

On Jun 11, 2017, at 10:40 PM, Bob Carpenter ***@***.***> wrote: I fixed the merge conflict, but this now failing here on my local tests: test/unit/services/sample/hmc_nuts_dense_inv_metric --gtest_output="xml:test/unit/services/sample/hmc_nuts_dense_inv_metric.xml" Running main() from gtest_main.cc [==========] Running 4 tests from 1 test case. [----------] Global test environment set-up. [----------] 4 tests from ServicesSampleHmcNutsDenseEMassMatrix [ RUN ] ServicesSampleHmcNutsDenseEMassMatrix.ident_no_adapt [ OK ] ServicesSampleHmcNutsDenseEMassMatrix.ident_no_adapt (6 ms) [ RUN ] ServicesSampleHmcNutsDenseEMassMatrix.ident_adapt_250 src/test/unit/services/check_adaptation.hpp:63: Failure The difference between param_vals[ij] and test::unit::stod(strs[j]) is 0.45182600000000006, which exceeds err_margin, where param_vals[ij] evaluates to 1.2162200000000001, test::unit::stod(strs[j]) evaluates to 0.76439400000000002, and err_margin evaluates to 0.20000000000000001. [ FAILED ] ServicesSampleHmcNutsDenseEMassMatrix.ident_adapt_250 (103 ms) [ RUN ] ServicesSampleHmcNutsDenseEMassMatrix.use_metric_no_adapt [ OK ] ServicesSampleHmcNutsDenseEMassMatrix.use_metric_no_adapt (1 ms) [ RUN ] ServicesSampleHmcNutsDenseEMassMatrix.use_metric_skip_adapt [ OK ] ServicesSampleHmcNutsDenseEMassMatrix.use_metric_skip_adapt (1 ms) [----------] 4 tests from ServicesSampleHmcNutsDenseEMassMatrix (111 ms total) @mitzimorris <https://github.com/mitzimorris> and @betanalpha <https://github.com/betanalpha> --- can you work this out? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2313 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABdNlgPt4Le_8JPhy5TZQKQsNTEjRSELks5sDKU3gaJpZM4NjJGV>.