hmm rng states start at 1, not 0. #2113

charlesm93 · 2020-09-29T15:50:42Z

Summary

Solves issue#2112. The states returned by hmm_hidden_state_rng now live on {1, 2, ..., K}, rather than {0, 1, ..., K - 1}.

Tests

The existing unit tests were adjusted to handle state values that start at 1.

Release notes

The states returned by hmm_hidden_state_rng now live on {1, 2, ..., K}, rather than {0, 1, ..., K - 1}.

Checklist

bob-carpenter

Looks good. Thanks for the quick fix! The doc typo correction is optional, so I'll just approve this. If you want to correct it, I can approve again quickly.

bob-carpenter · 2020-09-29T16:27:14Z

test/unit/math/prim/prob/hmm_latent_rng_test.cpp

  // states are identifcal and follow a Bernoulli distribution parameterized
  // by rho.
+  // The samples live on {1, 2}, so we need to deduct one to male the


"male" -> "make"

syclik · 2020-09-29T17:29:37Z

stan/math/prim/prob/hmm_latent_rng.hpp

@@ -70,18 +70,22 @@ inline std::vector<int> hmm_latent_rng(const T_omega& log_omegas,
  Eigen::Map<Eigen::VectorXd> probs_vec(probs.data(), n_states);
  probs_vec = alphas.col(n_transitions) / alphas.col(n_transitions).sum();
  boost::random::discrete_distribution<> cat_hidden(probs);
-  hidden_states[n_transitions] = cat_hidden(rng);
+  hidden_states[n_transitions] = cat_hidden(rng) + 1;


@charlesm93, there's a terribly named variable that stores the indexing. If you wanted to be proper about this, you could change this (and other instances) of + 1 with + stan::error_index::value. https://github.com/stan-dev/math/blob/develop/stan/math/prim/meta/error_index.hpp

It's not only terribly named... it has no doc and I don't know how you would have known about it. I'll create issues to tackle that later.

The intention was if someone really wanted to 0-index things (like if it were from a C++ function), they'd compile with -DERROR_INDEX=0 and that would magically make things like this, bounds checks on arrays, and other places where there is indexing, be 0-indexed. By default, it's 1-indexed.

Alternately, we can just ditch the generality and explicitly say that everything is 1-indexed, as confusing as it is with C++ being 0-indexed, and make life simpler.

Anyway, just adding a comment so you know that there's a variable that defines the indexing. If you want to switch it, great. If not, also great.

@charlesm93 I think it makes sense to do what @syclik is saying here.

I think how this works is you just add/subtract stan::error_index::value everywhere before you added/subtracted 1 (including the tests). It's all tested in 1-indexing mode (how Stan uses it) and then if someone wants the 0-indexing they can do the make flag.

Release is the 19th and code freeze is the 12th for reference.

@charlesm93 since this was sitting here and it was a relatively simple but confusing change I just did it (and made Bob's edit). Have a look and if you approve we can merge this.

@bbbales2 Thanks for jumping in. I agree with what @syclik proposes. I'll take a quick look.

@bbbales2 you missed one +1, I translated it into an index error. Once the integration tests pass, I'll merge it (small caprice, I love pushing the merge button!)

stan-buildbot · 2020-09-30T00:35:28Z

Name	Old Result	New Result	Ratio	Performance change( 1 - new / old )
gp_pois_regr/gp_pois_regr.stan	3.58	3.6	1.0	-0.43% slower
low_dim_corr_gauss/low_dim_corr_gauss.stan	0.02	0.02	0.98	-2.5% slower
eight_schools/eight_schools.stan	0.12	0.12	0.99	-1.14% slower
gp_regr/gp_regr.stan	0.18	0.19	0.96	-4.56% slower
irt_2pl/irt_2pl.stan	5.66	5.74	0.98	-1.54% slower
performance.compilation	91.65	88.83	1.03	3.08% faster
low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan	9.2	9.15	1.01	0.54% faster
pkpd/one_comp_mm_elim_abs.stan	29.35	29.13	1.01	0.75% faster
sir/sir.stan	137.14	136.52	1.0	0.45% faster
gp_regr/gen_gp_data.stan	0.04	0.05	0.92	-9.0% slower
low_dim_gauss_mix/low_dim_gauss_mix.stan	3.17	3.17	1.0	0.19% faster
pkpd/sim_one_comp_mm_elim_abs.stan	0.39	0.39	1.02	1.48% faster
arK/arK.stan	2.76	2.74	1.0	0.46% faster
arma/arma.stan	0.74	0.75	0.99	-1.35% slower
garch/garch.stan	0.65	0.65	1.0	0.07% faster
Mean result: 0.991791636239

Jenkins Console Log
Blue Ocean
Commit hash: 4526753

Machine information

ProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010

CPU:
Intel(R) Xeon(R) CPU E5-1680 v2 @ 3.00GHz

G++:
Configured with: --prefix=/Applications/Xcode.app/Contents/Developer/usr --with-gxx-include-dir=/usr/include/c++/4.2.1
Apple LLVM version 7.0.2 (clang-700.1.81)
Target: x86_64-apple-darwin15.6.0
Thread model: posix

Clang:
Apple LLVM version 7.0.2 (clang-700.1.81)
Target: x86_64-apple-darwin15.6.0
Thread model: posix

…4.1 (tags/RELEASE_600/final)

bbbales2 · 2020-10-03T23:03:50Z

Got this error on the Mac integration tests (log):

bin/stanc src/test/test-models/good/array-expr/validate_array_expr_big.stan --o=test/test-models/good/array-expr/validate_array_expr_big.hpp
bin/stanc: line 1: Not: command not found
make: *** [test/test-models/good/array-expr/validate_array_expr_big.hpp] Error 127
make: *** Waiting for unfinished jobs....

I'm not sure what it's about. Just restarting the integration tests.

stan-buildbot · 2020-10-04T02:14:49Z

Name	Old Result	New Result	Ratio	Performance change( 1 - new / old )
gp_pois_regr/gp_pois_regr.stan	3.31	3.4	0.97	-2.8% slower
low_dim_corr_gauss/low_dim_corr_gauss.stan	0.02	0.02	0.97	-3.43% slower
eight_schools/eight_schools.stan	0.12	0.12	0.98	-2.29% slower
gp_regr/gp_regr.stan	0.18	0.18	0.96	-4.02% slower
irt_2pl/irt_2pl.stan	5.62	5.62	1.0	0.03% faster
performance.compilation	90.9	89.75	1.01	1.27% faster
low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan	9.1	9.06	1.0	0.46% faster
pkpd/one_comp_mm_elim_abs.stan	30.4	28.99	1.05	4.67% faster
sir/sir.stan	129.09	122.86	1.05	4.82% faster
gp_regr/gen_gp_data.stan	0.04	0.05	0.99	-1.03% slower
low_dim_gauss_mix/low_dim_gauss_mix.stan	3.17	3.17	1.0	-0.01% slower
pkpd/sim_one_comp_mm_elim_abs.stan	0.4	0.38	1.03	3.13% faster
arK/arK.stan	2.62	2.55	1.03	2.53% faster
arma/arma.stan	0.75	0.74	1.01	1.41% faster
garch/garch.stan	0.66	0.65	1.01	0.63% faster
Mean result: 1.00430194096

Jenkins Console Log
Blue Ocean
Commit hash: 497cf0d

Machine information

ProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010

CPU:
Intel(R) Xeon(R) CPU E5-1680 v2 @ 3.00GHz

G++:
Configured with: --prefix=/Applications/Xcode.app/Contents/Developer/usr --with-gxx-include-dir=/usr/include/c++/4.2.1
Apple LLVM version 7.0.2 (clang-700.1.81)
Target: x86_64-apple-darwin15.6.0
Thread model: posix

Clang:
Apple LLVM version 7.0.2 (clang-700.1.81)
Target: x86_64-apple-darwin15.6.0
Thread model: posix

bbbales2 · 2020-10-04T14:04:54Z

The same error popped up again. Do you recognize this one @rok-cesnovar?

Log here: https://jenkins.mc-stan.org/blue/organizations/jenkins/Stan/detail/downstream_tests/2044/pipeline

bin/stanc src/test/test-models/good/array-expr/validate_array_expr_big.stan --o=test/test-models/good/array-expr/validate_array_expr_big.hpp
bin/stanc: line 1: Not: command not found
make: *** [test/test-models/good/array-expr/validate_array_expr_big.hpp] Error 127
make: *** Waiting for unfinished jobs....

rok-cesnovar · 2020-10-04T14:08:50Z

This looks as if the stanc download failed and did not retry (it fails on the first attempt of using it). Restart it and I will make an issue to make this more robust.

rok-cesnovar · 2020-10-04T14:09:42Z

I restarted the tests, will go green once they pass. https://jenkins.mc-stan.org/blue/organizations/jenkins/Math%20Pipeline/detail/PR-2113/4/pipeline

stan-buildbot · 2020-10-04T16:00:00Z

Name	Old Result	New Result	Ratio	Performance change( 1 - new / old )
gp_pois_regr/gp_pois_regr.stan	3.27	3.31	0.99	-1.17% slower
low_dim_corr_gauss/low_dim_corr_gauss.stan	0.02	0.02	1.04	4.01% faster
eight_schools/eight_schools.stan	0.12	0.12	1.02	1.79% faster
gp_regr/gp_regr.stan	0.18	0.18	0.99	-0.84% slower
irt_2pl/irt_2pl.stan	5.66	5.68	1.0	-0.34% slower
performance.compilation	90.93	89.79	1.01	1.25% faster
low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan	9.07	9.1	1.0	-0.33% slower
pkpd/one_comp_mm_elim_abs.stan	29.2	28.66	1.02	1.86% faster
sir/sir.stan	129.27	125.08	1.03	3.24% faster
gp_regr/gen_gp_data.stan	0.04	0.05	0.94	-6.64% slower
low_dim_gauss_mix/low_dim_gauss_mix.stan	3.17	3.18	1.0	-0.17% slower
pkpd/sim_one_comp_mm_elim_abs.stan	0.38	0.38	1.0	-0.04% slower
arK/arK.stan	2.57	2.56	1.0	0.09% faster
arma/arma.stan	0.74	0.75	0.99	-0.94% slower
garch/garch.stan	0.66	0.66	1.0	-0.16% slower
Mean result: 1.00160651824

Jenkins Console Log
Blue Ocean
Commit hash: 497cf0d

Machine information

ProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010

CPU:
Intel(R) Xeon(R) CPU E5-1680 v2 @ 3.00GHz

G++:
Configured with: --prefix=/Applications/Xcode.app/Contents/Developer/usr --with-gxx-include-dir=/usr/include/c++/4.2.1
Apple LLVM version 7.0.2 (clang-700.1.81)
Target: x86_64-apple-darwin15.6.0
Thread model: posix

Clang:
Apple LLVM version 7.0.2 (clang-700.1.81)
Target: x86_64-apple-darwin15.6.0
Thread model: posix

…stan-dev/math into bugfix/issue-2112-hmm_rng_index

stan-buildbot · 2020-10-05T03:19:37Z

Name	Old Result	New Result	Ratio	Performance change( 1 - new / old )
gp_pois_regr/gp_pois_regr.stan	3.37	3.13	1.07	6.86% faster
low_dim_corr_gauss/low_dim_corr_gauss.stan	0.02	0.02	0.99	-1.21% slower
eight_schools/eight_schools.stan	0.12	0.12	1.05	4.41% faster
gp_regr/gp_regr.stan	0.18	0.18	1.0	-0.02% slower
irt_2pl/irt_2pl.stan	5.69	5.65	1.01	0.87% faster
performance.compilation	92.22	89.82	1.03	2.6% faster
low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan	9.08	9.07	1.0	0.1% faster
pkpd/one_comp_mm_elim_abs.stan	29.1	29.99	0.97	-3.07% slower
sir/sir.stan	130.97	124.82	1.05	4.69% faster
gp_regr/gen_gp_data.stan	0.05	0.05	0.99	-1.08% slower
low_dim_gauss_mix/low_dim_gauss_mix.stan	3.16	3.17	1.0	-0.24% slower
pkpd/sim_one_comp_mm_elim_abs.stan	0.39	0.38	1.01	1.24% faster
arK/arK.stan	2.57	2.56	1.0	0.32% faster
arma/arma.stan	0.77	0.74	1.03	2.82% faster
garch/garch.stan	0.66	0.65	1.01	0.83% faster
Mean result: 1.0135690709

Jenkins Console Log
Blue Ocean
Commit hash: 5e829da

Machine information

ProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010

CPU:
Intel(R) Xeon(R) CPU E5-1680 v2 @ 3.00GHz

G++:
Configured with: --prefix=/Applications/Xcode.app/Contents/Developer/usr --with-gxx-include-dir=/usr/include/c++/4.2.1
Apple LLVM version 7.0.2 (clang-700.1.81)
Target: x86_64-apple-darwin15.6.0
Thread model: posix

Clang:
Apple LLVM version 7.0.2 (clang-700.1.81)
Target: x86_64-apple-darwin15.6.0
Thread model: posix

charlesm93 · 2020-10-05T20:06:21Z

All the comments by @bob-carpenter and @syclik have been addressed. Can one of the reviewers approve the PR?

bob-carpenter

Thanks for updating!

charlesm93 added 2 commits September 29, 2020 11:40

hmm rng states start at 1, not 0.

85f8bf8

correct cpplint errors.

4526753

charlesm93 requested a review from bob-carpenter September 29, 2020 15:50

bob-carpenter previously approved these changes Sep 29, 2020

View reviewed changes

syclik reviewed Sep 29, 2020

View reviewed changes

Updated code to use stan::math::error_index (Issue #2112)

e07aafc

bbbales2 dismissed bob-carpenter’s stale review via e07aafc October 3, 2020 16:35

yashikno and others added 2 commits October 3, 2020 16:48

Merge commit '0cacb8d708dde02b0ffa003d2e3a1fb30057a04d' into HEAD

24ac12b

[Jenkins] auto-formatting by clang-format version 6.0.0-1ubuntu2~16.0…

497cf0d

…4.1 (tags/RELEASE_600/final)

charlesm93 added 2 commits October 4, 2020 14:11

Merge branch 'bugfix/issue-2112-hmm_rng_index' of https://github.com/…

5faf20c

…stan-dev/math into bugfix/issue-2112-hmm_rng_index

Fix error index.

5e829da

bob-carpenter approved these changes Oct 5, 2020

View reviewed changes

bob-carpenter merged commit d5d234d into develop Oct 5, 2020

rok-cesnovar mentioned this pull request Oct 13, 2020

Release of Stan Math 3.4 #2128

Closed

bbbales2 mentioned this pull request Oct 26, 2020

Simulated states from hidden Markov model should be sampled from {1, 2,...,N} (rather than 0,1,2,...,N-1) #2112

Closed

rok-cesnovar mentioned this pull request Feb 2, 2021

Simulated states from hidden Markov model should be sampled from {1, 2,...,N} (rather than 0,1,2,...,N-1) stan-dev/stan#2957

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

hmm rng states start at 1, not 0. #2113

hmm rng states start at 1, not 0. #2113

charlesm93 commented Sep 29, 2020

bob-carpenter left a comment

bob-carpenter Sep 29, 2020

syclik Sep 29, 2020

bbbales2 Oct 3, 2020

bbbales2 Oct 3, 2020

charlesm93 Oct 4, 2020

charlesm93 Oct 4, 2020

stan-buildbot commented Sep 30, 2020

bbbales2 commented Oct 3, 2020

stan-buildbot commented Oct 4, 2020

bbbales2 commented Oct 4, 2020

rok-cesnovar commented Oct 4, 2020 •

edited

Loading

rok-cesnovar commented Oct 4, 2020 •

edited

Loading

stan-buildbot commented Oct 4, 2020

stan-buildbot commented Oct 5, 2020

charlesm93 commented Oct 5, 2020

bob-carpenter left a comment

hmm rng states start at 1, not 0. #2113

hmm rng states start at 1, not 0. #2113

Conversation

charlesm93 commented Sep 29, 2020

Summary

Tests

Release notes

Checklist

bob-carpenter left a comment

Choose a reason for hiding this comment

bob-carpenter Sep 29, 2020

Choose a reason for hiding this comment

syclik Sep 29, 2020

Choose a reason for hiding this comment

bbbales2 Oct 3, 2020

Choose a reason for hiding this comment

bbbales2 Oct 3, 2020

Choose a reason for hiding this comment

charlesm93 Oct 4, 2020

Choose a reason for hiding this comment

charlesm93 Oct 4, 2020

Choose a reason for hiding this comment

stan-buildbot commented Sep 30, 2020

bbbales2 commented Oct 3, 2020

stan-buildbot commented Oct 4, 2020

bbbales2 commented Oct 4, 2020

rok-cesnovar commented Oct 4, 2020 • edited Loading

rok-cesnovar commented Oct 4, 2020 • edited Loading

stan-buildbot commented Oct 4, 2020

stan-buildbot commented Oct 5, 2020

charlesm93 commented Oct 5, 2020

bob-carpenter left a comment

Choose a reason for hiding this comment

rok-cesnovar commented Oct 4, 2020 •

edited

Loading

rok-cesnovar commented Oct 4, 2020 •

edited

Loading