Semantic check warnings #336

[enetsee]

Fixes #336. @seantalts @wds15 @bob-carpenter - you may want to set out some additional tests for this based on the failing code.

[wds15]

Can I test this PR easily? I mean... is there a stanc3 binary I can grab from somewhere (for macOS or Linux)...

[rok-cesnovar]

If you want I can send you the binary to your email or upload it somewhere. I just switch the branch and build. EDIT: I sent the binary to the email you get jenkins e-mails to. Hopefully that is the correct one :)

[enetsee]

Can I test this PR easily?

It would be good to have another example in the test suite. Would you be able to provide one the Stan models on which stanc3 was failing?

[wds15]

No. It doesn't work yet - possibly I have been unclear or we have different notions.

The attached model should not error, but just compile (with a warning).

oral_1cmt_mm_run_stan.txt

Right now it errors at the integrate_ode call.

[enetsee]

@wds15 - ok, I think we may have our wires crossed. This PR relaxes the requirement that arguments provided to user defined functions, not Stan math functions like integrate_ode. We will need to relax the requirement there too for this to work.

@seantalts I can do this but longer term we should really think if we want to allow this in the type and effect system. Maybe we should start grouping warnings for deprecations etc so users have fair warning if/when we turn this off?

[enetsee]

I've just pushed a change which applies to stan math functions. I had to correct a bunch of models which were missing a data declaration so someone may want to take a quick look at them:

• test/integration/bad/ode/adams/bad_x_var_type.stan
• test/integration/bad/ode/adams/bad_x_var_type_adams_control.stan
• test/integration/bad/ode/adams/bad_x_var_type_control.stan
• test/integration/bad/ode/bad_x_var_type.stan
• test/integration/bad/ode/bad_x_var_type_bdf.stan
• test/integration/bad/ode/bad_x_var_type_bdf_control.stan
• test/integration/bad/ode/bad_x_var_type_rk45.stan
• test/integration/bad/ode/bad_x_var_type_rk45_control.stan

[enetsee]

@wds15 I've included your model in the integration test set. It now compiles with the following warning:

|Warning: Semantic warning in 'ode_sebastian.stan', line 868, column 14 to column 99:
| -------------------------------------------------
| 866: theta_tilde[P+1] = lref[1];
| 867: lref_tilde[1] = lref[2];
| 868: int_sol = integrate_ode(pk_1cmt_mm_lode, lref_tilde, 0, to_array_1d(Dt), theta_tilde, x_r, x_i);//, 1e-4, 1e-4, 1000);
| ^
| 869: ka = theta[1];
| 870: for(i in 1:num_elements(Dt)) {
| -------------------------------------------------
|
|Warning: Argument to 'integrate_ode' has an incompatible autodiff level.

[seantalts]

It looks like we're not going to get stanc3 into the next release; a small minority of people at the Stan meeting were really adamant that we not release something with known bugs (which doesn't make sense w.r.t. how we release the rest of Stan, but whatever). There was also a surprising lack of familiarity with how semantic versioning, which has been used on this project I believe since its inception, actually works. I think I could have done a better job explaining where we are with stanc3 relative to the old compiler, but the damage seems done for 2.21. Sorry about that.

Anyway, could you make the warning say which arguments are incorrect and notify them that this could cause the C++ compiler to fail?

@seantalts I can do this but longer term we should really think if we want to allow this in the type and effect system. Maybe we should start grouping warnings for deprecations etc so users have fair warning if/when we turn this off?

Which thing is "this" referring to, in "if we want to allow this?" We definitely have a bunch of deprecated behavior we want to be warning about and be ready to remove in Stan 3, so coming up with some facility to handle that seems very prudent.

[seantalts]

That said, I really appreciate you jumping in to try to enable this use-case last second. Thanks @enetsee :)

[enetsee]

Closing; the work on validation is in #357 and I think we may want to think more about our approach to semantic warnings.