From ff73d1354f8a6d5a100674a8cd5982a2b0f7c6bc Mon Sep 17 00:00:00 2001 From: Brian Ward Date: Wed, 21 Jan 2026 16:02:03 -0500 Subject: [PATCH 1/2] Fix multiple unconstrained params being read incorrectly in log_prob call --- src/cmdstan/command_helper.hpp | 13 ++++--- src/test/interface/log_prob_test.cpp | 38 +++++++++++++++++-- .../test-models/bern_unconstrained_multi.json | 16 ++++++++ 3 files changed, 58 insertions(+), 9 deletions(-) create mode 100644 src/test/test-models/bern_unconstrained_multi.json diff --git a/src/cmdstan/command_helper.hpp b/src/cmdstan/command_helper.hpp index 2fa29e7062..a09f811a8c 100644 --- a/src/cmdstan/command_helper.hpp +++ b/src/cmdstan/command_helper.hpp @@ -528,7 +528,7 @@ Eigen::VectorXd get_laplace_mode(const std::string &fname, * @param model Stan model * @return vector of vectors of parameter estimates */ -std::vector> get_uparams_r( +inline std::vector> get_uparams_r( const std::string &fname, const stan::model::model_base &model) { size_t u_params_cols = 0; size_t u_params_rows = 0; @@ -559,8 +559,8 @@ std::vector> get_uparams_r( std::vector> params_r_ind( u_params_rows, std::vector(u_params_cols)); size_t idx = 0; - for (size_t i = 0; i < u_params_rows; ++i) { - for (size_t j = 0; j < u_params_cols; ++j) { + for (size_t j = 0; j < u_params_cols; ++j) { + for (size_t i = 0; i < u_params_rows; ++i) { params_r_ind[i][j] = *(u_params_r.data() + idx); ++idx; } @@ -577,9 +577,10 @@ std::vector> get_uparams_r( * @param params_set array of unconstrained parameter values * @ */ -void services_log_prob_grad(const stan::model::model_base &model, bool jacobian, - std::vector> ¶ms_set, - stan::callbacks::writer &output) { +inline void services_log_prob_grad(const stan::model::model_base &model, + bool jacobian, + std::vector> ¶ms_set, + stan::callbacks::writer &output) { // header std::vector p_names{"lp__"}; model.unconstrained_param_names(p_names, false, false); diff --git a/src/test/interface/log_prob_test.cpp b/src/test/interface/log_prob_test.cpp index 28003b9c57..af888149bd 100644 --- a/src/test/interface/log_prob_test.cpp +++ b/src/test/interface/log_prob_test.cpp @@ -1,11 +1,8 @@ #include #include -#include #include -#include using cmdstan::test::convert_model_path; -using cmdstan::test::multiple_command_separator; using cmdstan::test::parse_sample; using cmdstan::test::run_command; using cmdstan::test::run_command_output; @@ -30,6 +27,8 @@ class CmdStan : public testing::Test { "bern_unconstrained_params_short.json"}; bern_constrained_params_short = {"src", "test", "test-models", "bern_constrained_params_short.json"}; + bern_unconstrained_params_multi + = {"src", "test", "test-models", "bern_unconstrained_multi.json"}; test_output = {"test", "output.csv"}; simplex_model = {"src", "test", "test-models", "simplex_model"}; simplex_constrained_bad_csv @@ -47,6 +46,8 @@ class CmdStan : public testing::Test { std::vector bern_constrained_params_csv; std::vector bern_unconstrained_params_short; std::vector bern_constrained_params_short; + std::vector bern_unconstrained_params_multi; + std::vector test_output; std::vector simplex_model; std::vector simplex_constrained_csv; @@ -73,6 +74,8 @@ TEST_F(CmdStan, log_prob_uparams_rdump) { ASSERT_EQ(values.size() % names.size(), 0); ASSERT_EQ(names[0].compare(0, 2, std::string("lp")), 0); ASSERT_EQ(names[1].compare(0, 2, std::string("g_")), 0); + + ASSERT_FLOAT_EQ(values[0], -26.1950918); } TEST_F(CmdStan, log_prob_cparams_rdump) { @@ -93,6 +96,8 @@ TEST_F(CmdStan, log_prob_cparams_rdump) { boost::split(names, header[0], boost::is_any_of(","), boost::token_compress_on); ASSERT_TRUE(values.size() % names.size() == 0); + + ASSERT_FLOAT_EQ(values[0], -26.1950918); } TEST_F(CmdStan, log_prob_uparams_json) { @@ -113,6 +118,32 @@ TEST_F(CmdStan, log_prob_uparams_json) { boost::split(names, header[0], boost::is_any_of(","), boost::token_compress_on); ASSERT_EQ(values.size() % names.size(), 0); + + ASSERT_FLOAT_EQ(values[0], -26.1950918); +} + +TEST_F(CmdStan, log_prob_uparams_multi_json) { + std::stringstream ss; + ss << convert_model_path(bern_log_prob_model) + << " data file=" << convert_model_path(bern_data) + << " output file=" << convert_model_path(test_output) + << " method=log_prob unconstrained_params=" + << convert_model_path(bern_unconstrained_params_multi); + std::string cmd = ss.str(); + run_command_output out = run_command(cmd); + ASSERT_FALSE(out.hasError); + std::vector config; + std::vector header; + std::vector values; + parse_sample(convert_model_path(test_output), config, header, values); + std::vector names; + boost::split(names, header[0], boost::is_any_of(","), + boost::token_compress_on); + ASSERT_EQ(values.size() % names.size(), 0); + + // log_p values calculated externally + ASSERT_FLOAT_EQ(values[0], -24.528835); + ASSERT_FLOAT_EQ(values[names.size()], -35.035283); } TEST_F(CmdStan, log_prob_cparams_json) { @@ -133,6 +164,7 @@ TEST_F(CmdStan, log_prob_cparams_json) { boost::split(names, header[0], boost::is_any_of(","), boost::token_compress_on); ASSERT_EQ(values.size() % names.size(), 0); + ASSERT_FLOAT_EQ(values[0], -26.1950918); } TEST_F(CmdStan, log_prob_cparams_csv) { diff --git a/src/test/test-models/bern_unconstrained_multi.json b/src/test/test-models/bern_unconstrained_multi.json new file mode 100644 index 0000000000..83c455555e --- /dev/null +++ b/src/test/test-models/bern_unconstrained_multi.json @@ -0,0 +1,16 @@ +{ + "params_r": [ + [ + 1.35584153, 0.27525052, 0.96573, -2.16005226, -1.37546212, -0.11006157, + 0.68632036, 0.89054693, 1.01844639, -1.63940773, -1.42824144, -0.58972464, + -0.27810713, 0.49700114, -0.66268587, -0.05818367, 0.21485661, -0.8569392, + 1.62690887 + ], + [ + 2.80681384, -0.50342205, -0.064571, -1.02065775, 0.96462088, -1.78105829, + 0.70339144, -0.13549738, 0.96043048, 1.43833813, 0.39750983, -0.59743302, + -1.34201349, 1.73491275, -0.70208818, -0.30521858, -1.58527664, + 0.02588404, 0.89910165 + ] + ] +} From 0f43960bcc7177e0e545c93074ad3f110229ed5a Mon Sep 17 00:00:00 2001 From: Brian Ward Date: Wed, 21 Jan 2026 16:27:04 -0500 Subject: [PATCH 2/2] Also test rdump --- src/test/interface/log_prob_test.cpp | 33 +++++++++++++++++-- .../test-models/bern_unconstrained_multi.R | 14 ++++++++ 2 files changed, 44 insertions(+), 3 deletions(-) create mode 100644 src/test/test-models/bern_unconstrained_multi.R diff --git a/src/test/interface/log_prob_test.cpp b/src/test/interface/log_prob_test.cpp index af888149bd..e00090f36c 100644 --- a/src/test/interface/log_prob_test.cpp +++ b/src/test/interface/log_prob_test.cpp @@ -27,8 +27,10 @@ class CmdStan : public testing::Test { "bern_unconstrained_params_short.json"}; bern_constrained_params_short = {"src", "test", "test-models", "bern_constrained_params_short.json"}; - bern_unconstrained_params_multi + bern_unconstrained_params_multi_json = {"src", "test", "test-models", "bern_unconstrained_multi.json"}; + bern_unconstrained_params_multi_rdump + = {"src", "test", "test-models", "bern_unconstrained_multi.R"}; test_output = {"test", "output.csv"}; simplex_model = {"src", "test", "test-models", "simplex_model"}; simplex_constrained_bad_csv @@ -46,7 +48,8 @@ class CmdStan : public testing::Test { std::vector bern_constrained_params_csv; std::vector bern_unconstrained_params_short; std::vector bern_constrained_params_short; - std::vector bern_unconstrained_params_multi; + std::vector bern_unconstrained_params_multi_json; + std::vector bern_unconstrained_params_multi_rdump; std::vector test_output; std::vector simplex_model; @@ -78,6 +81,30 @@ TEST_F(CmdStan, log_prob_uparams_rdump) { ASSERT_FLOAT_EQ(values[0], -26.1950918); } +TEST_F(CmdStan, log_prob_uparams_multi_rdump) { + std::stringstream ss; + ss << convert_model_path(bern_log_prob_model) + << " data file=" << convert_model_path(bern_data) + << " output file=" << convert_model_path(test_output) + << " method=log_prob unconstrained_params=" + << convert_model_path(bern_unconstrained_params_multi_rdump); + std::string cmd = ss.str(); + run_command_output out = run_command(cmd); + ASSERT_FALSE(out.hasError); + std::vector config; + std::vector header; + std::vector values; + parse_sample(convert_model_path(test_output), config, header, values); + std::vector names; + boost::split(names, header[0], boost::is_any_of(","), + boost::token_compress_on); + ASSERT_EQ(values.size() % names.size(), 0); + + // log_p values calculated externally + ASSERT_FLOAT_EQ(values[0], -14.90125); + ASSERT_FLOAT_EQ(values[names.size()], -17.19926); +} + TEST_F(CmdStan, log_prob_cparams_rdump) { std::stringstream ss; ss << convert_model_path(bern_log_prob_model) @@ -128,7 +155,7 @@ TEST_F(CmdStan, log_prob_uparams_multi_json) { << " data file=" << convert_model_path(bern_data) << " output file=" << convert_model_path(test_output) << " method=log_prob unconstrained_params=" - << convert_model_path(bern_unconstrained_params_multi); + << convert_model_path(bern_unconstrained_params_multi_json); std::string cmd = ss.str(); run_command_output out = run_command(cmd); ASSERT_FALSE(out.hasError); diff --git a/src/test/test-models/bern_unconstrained_multi.R b/src/test/test-models/bern_unconstrained_multi.R new file mode 100644 index 0000000000..0a1cdee10f --- /dev/null +++ b/src/test/test-models/bern_unconstrained_multi.R @@ -0,0 +1,14 @@ +params_r <- structure(c(-1.7826899385311055, 0.058362903176109024, -1.8870574951389159, +0.32182653494850982, -0.33279912821578023, 1.1755011233138752, +0.548323677676959, -0.3390893619749813, -0.36966005408259461, +-1.0469050778091906, -2.3571410564924653, -0.36226465437324951, +0.18476348400049031, 0.3538173513390801, -0.29572060754310731, +0.75118275172385274, -0.13441108926989107, -0.50208301598326377, +0.8422549630783327, -0.85559862482538085, 0.22163915310082957, +-0.0967955260147986, -0.42955366613672469, -1.2114667038261244, +-0.22780372272202434, -1.5982657722626148, 0.30269019545481407, +0.72620422198392964, 0.179903457308519, -2.599292902623854, -1.60779131334911, +0.046478898063598527, -0.60666582204989516, 0.44509877260725766, +0.2512029348511659, -0.99552555982651314, 1.1981866907239958, +-0.62839111843893136), .Dim = c(2, 19)) +