array-valued transform arguments

[nhuurre]

Fix #547
Non-scalar values are now allowed in lower, upper, offset and multiplier transforms if the value has the same type and size as the declared variable.

Note that container bounds check error messages have always been kind of bogus.

parameters {
  real p[5,5];
}
transformed parameters {
  real<lower=0> t[5,5] = p;
}

Rejecting initial value:
  Error evaluating the log probability at the initial value.
Exception: example_model_namespace::log_prob: t[sym1__, sym2__] is -0.574295,
  but must be greater than or equal to 0 (in 'example.stan', line 5, column 2 to column 27)

The opaque indexing is being fixed in math (stan-dev/math#1635) but it doesn't support array-valued bounds.

[bob-carpenter]

I don't quite feel prepared to review the OCaml, but this is huge! We've been wanting to do this for ages.

We've been improving indexing into error messages every place we can, so that'll also be great if that can be fixed.

[rok-cesnovar]

With develop cmdstan you can set this in make/local to test this PR:

STANC3_TEST_BIN_URL = https://jenkins.mc-stan.org/job/stanc3-test-binaries/106/artifact

I can review but would like some help testing if everything is fine in terms of modelling.

[rok-cesnovar]

@nhuurre can you merge master in? I can review this today.

[wds15]

whow 10k lines of code!

[rok-cesnovar]

The changes look good. I think the checks should go in the stan repo.

[rok-cesnovar]

Could we move this functions to the stan-dev/stan repo? Not sure of the best location but I guess under /model/checks.hpp and then add that header to the model_header? They would probably have to be named more in the style of math / stan C++ conventions we use. So something like check_shape().

The other functions that dont depend on model specific should probably also go there but that is not this PRs fault and most of them can actually just be removed.

[nhuurre]

Yeah, I was definitely being lazy. I think this actually belongs in the math library. FnCheckShape__ is a generalization of check_consistent_size. Alternatively it could be replaced by check_matching_dims if that recursed into arbitrary arrays.

[rok-cesnovar]

I am fine with either of the proposed solutions.

[rok-cesnovar]

One minor thing then its good to go.

[rok-cesnovar]

Thanks!

[nhuurre]

Hey, @rok-cesnovar ,just curious, why do you squash the history when merging these?
When I try to delete the branch locally git thinks it hasn't been merged.

[rok-cesnovar]

No reason, its the default option I think.

[nhuurre]

I don't think that's a good default. It doesn't matter for small fixes like this PR but optimization debugging was a sequence of small changes with associated explanations and now all of that is a single huge blob. Good luck figuring out which parts of the code those bullet points refer to.

[bob-carpenter]

I don't think it is the default. At least it wasn't when I set things up. We had a long go around on this ages ago and decided to keep all the commit and merge history bubbles. If you are going to squash history, whatever you do, don't rewrite it on the origin (GitHub)---only collapse commits locally before pushing to origin.

[rok-cesnovar]

stanc3 has these three options on:

Allow merge commits
Add all commits from the head branch to the base branch with a merge commit.

Allow squash merging
Combine all commits from the head branch into a single commit in the base branch.

Allow rebase merging
Add all commits from the head branch onto the base branch individually.

Should we just allow the first option then? I am fine with that.

[nhuurre]

Yes, I think the first is the preferred action.

[rok-cesnovar]

Done.