stan-dev / httpstan

HTTP interface to Stan, a package for Bayesian inference.
ISC License
39 stars 15 forks source link

C++ method transform_inits(data, constrained_parameters) returning two additional values in Stan 2.28 #577

Closed riddell-stan closed 2 years ago

riddell-stan commented 2 years ago

In 2.28-rc1 we're getting {'params_r_unconstrained': [x1, x2, 0.0, 0.0]} where we previously got {'params_r_unconstrained': [x1, x2]}.

@SteveBronder @rok-cesnovar Does this ring a bell? I couldn't quickly find a PR/issue in Stan or Stanc3.

SteveBronder commented 2 years ago

We made some changes to transform inits so this might be a bug, are you able to share the code?

SteveBronder commented 2 years ago

Oh I see this is happening in a test. I'm running out rn but I'll run this tmrw morning

SteveBronder commented 2 years ago

Actually think I got it, in transform_inits we were flattening the input array var context asking for all parameters and transformed parameters. Like in the below we ask if the user gave us a value for td_arr33 and then we look for it in the array var context, but we shouldn't do that.

  inline void transform_inits(const stan::io::var_context& context,
                              std::vector<int>& params_i,
                              std::vector<double>& vars,
                              std::ostream* pstream__ = nullptr) const {
    const std::array<std::string, 4> names__ = std::array<std::string, 4>{"xx",
      "y", "w", "td_arr33"};

     std::vector<double> params_r_flat__;
     for (auto&& param_name__ : names__) {
       const auto param_vec__ = context.vals_r(param_name__);
       params_r_flat__.reserve(params_r_flat__.size() + param_vec__.size());
       for (auto&& param_val__ : param_vec__) {
         params_r_flat__.push_back(param_val__);
       }
     }
    vars.resize(params_r_flat__.size());
    transform_inits_impl(params_r_flat__, params_i, vars, pstream__);
    }

We should only be asking for the parameters like.

  inline void transform_inits(const stan::io::var_context& context,
                              std::vector<int>& params_i,
                              std::vector<double>& vars,
                              std::ostream* pstream__ = nullptr) const {
    const std::array<std::string, 4> names__ = std::array<std::string, 3>{"xx",
      "y", "w"};

     std::vector<double> params_r_flat__;
     for (auto&& param_name__ : names__) {
       const auto param_vec__ = context.vals_r(param_name__);
       params_r_flat__.reserve(params_r_flat__.size() + param_vec__.size());
       for (auto&& param_val__ : param_vec__) {
         params_r_flat__.push_back(param_val__);
       }
     }
    vars.resize(params_r_flat__.size());
    transform_inits_impl(params_r_flat__, params_i, vars, pstream__);
    }

I made a patch for this, but I'd like to add one other quick fix tmrw which is to check that the size of the array the user is giving us matches up with the size each parameter is supposed to be

rok-cesnovar commented 2 years ago

This has been addressed and an RC2 for stanc3 has been built.

If you have the time to verify that RC2 does indeed fix this issue that would be great. Our plan is to release on Monday or Tuesday but we would like to have a green light from httpstan/pystan before we do it.

SteveBronder commented 2 years ago

^Don't we normally have two weeks of freeze? Or did we and it just flew by without me noticing?

rok-cesnovar commented 2 years ago

So far its been a week plus a few days if issues were discovered. We dont really get a ton of RC testers so it doesnt really make sense to prolong it much once we hash out the first issues reported by the devs/few early adopters.

we can definitely bump it a few days to make sure if needed.

riddell-stan commented 2 years ago

Thanks for the fix @SteveBronder . Tests are passing now. Closing this.

@SteveBronder @rok-cesnovar I definitely support a solid two weeks freeze. I'm pretty diligent about testing the release candidates now but I definitely need at least a week slack to make the initial tests. So ca. one week total freeze is cutting it very close.

If you really want to push it to 1 week, perhaps there's some workaround. Tagging a beta release a week or two before the release candidate would probably do the trick. Might not be a bad idea in any event. I usually associate "beta" with "please test" and "release candidate" with "we're pretty sure we've fixed all the bugs".

rok-cesnovar commented 2 years ago

Thanks for checking. And thanks for elaborating on why it would make sense to have 2 week freeze. Will make a note to bring that up before the January release.