stanc3 checks `data` keywords and doesn't allow potential non-data to be passed in.

[wds15]

When calling the integrate_ode functions inside other functions, then this is prone to over-propagation from data to var for the data arguments. With the old compiler it was possible to sub-select from bigger data structures smaller ones and these then still stay as data (as it should be). The new compiler does not do that. The new compiler does "stronger" over-propagation from data to var which makes the programs fail to compile due to a syntax error.

Attached is a Stan program example which illustrates the problem.

The error message is:

Semantic error in '../stan_pkpd/oral_1cmt_mm/oral_1cmt_mm_run.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)) {
   -------------------------------------------------

Ill-typed arguments supplied to function 'integrate_ode'. Available signatures:
((real, real[], real[], data real[], data int[]) => real[], real[], real, real[], real[], data real[], data int[]) => real[,]
Instead supplied arguments of incompatible type: (real, real[], real[], real[], int[]) => real[], real[], int, real[], real[], real[], int[].

I really think this should be fixed ... if possible before the 2.21 release.

stanc3-overpropagate-error.zip

[seantalts]

@VMatthijs any chance you can quickly see what's going on here? Do we need to turn on your optimization for optimal autodiff levels? We're trying to get the compiler released by Friday :)

[VMatthijs]

I think what's going on may be that stanc2 was too lenient in my mind with enforcement of data-only arguments. In stanc3, you need to annotate arguments explicitly as data-only. Otherwise they will get promoted to autodiff. Sorry to not be more helpful at the moment! My teaching is very busy and I am submitting a paper in a week. Time will free up again at the start of November.

…

On Mon, 14 Oct 2019 at 14:12, seantalts ***@***.***> wrote: @VMatthijs <https://github.com/VMatthijs> any chance you can quickly see what's going on here? Do we need to turn on your optimization for optimal autodiff levels? We're trying to get the compiler released by Friday :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#336?email_source=notifications&email_token=ADIVJE66KQ4I6NIDBYURVFDQOROZ5A5CNFSM4JAMHDZ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBEMQ3I#issuecomment-541640813>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADIVJE7E5QNCQZCOM3BQ2DTQOROZ5ANCNFSM4JAMHDZQ> .

[wds15]

So I should change my model and declare all data-only function arguments explicitly?

The code I shared worked all along. It was almost magic to have data being passed around and finally being called in the integrate_ode calls which lives inside functions. There is a lot of sweat to make this work and unfortunate to see this being broken now.

If it is now required to declare some specific arguments as data...then that is a solution, but it disqualifies at least my old programs.

[VMatthijs]

You can also try to get the compiler to infer whether something can be safely assumed be data without you writing it explicitly. I wrote a static analysis that does that, but it operates on the MIR rather than AST, while these programs are being rejected before they even reach the MIR. It's a basic information-flow/access control analysis so it's nothing fancy and it could also be achieved on the AST by implementing some form of type inference. But that's not going to be done by Friday. At that point, you should probably ask yourself whether you'd generally like full type inference in Stan or if there is some reason to only limit the type inference to the data vs autodiff levels.

…

On Mon, 14 Oct 2019 at 14:27, wds15 ***@***.***> wrote: So I should change my model and declare all data-only function arguments explicitly? The code I shared worked all along. It was almost magic to have data being passed around and finally being called in the integrate_ode calls which lives inside functions. There is a lot of sweat to make this work and unfortunate to see this being broken now. If it is now required to declare some specific arguments as data...then that is a solution, but it disqualifies at least my old programs. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#336?email_source=notifications&email_token=ADIVJE4OS2ERVGIO2CRRU3LQORQRXA5CNFSM4JAMHDZ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEBEOH6Q#issuecomment-541647866>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADIVJEZST7GJMI4AREN2YRLQORQRXANCNFSM4JAMHDZQ> .

[seantalts]

Yeah, now that I look at that model I'm not sure how Stan 2 knew that those arguments were data. I think the spec would say that you don't know it's data unless it's marked data, right? @bob-carpenter do you have a minute? Was stanc 2 doing some kind of fancy autodiff-level inference or was it just relying on C++ templates somehow?

[seantalts]

I'm going to close this for now as I think the behavior we have is adhering to the Stan spec. I'll add it to the list of changes since Stan 2.

[wds15]

I do disagree here... this is valid Stan code to my best knowledge. Could you please tell me how it is otherwise possible to pass data to function a which then subsets this and passes this to function b, please? In function b the subsetted data must still be data.

This has always been possible and it should by all means remain possible.

Let's have a take from @bob-carpenter on this, before we close.

[wds15]

Ok... I can get stanc3 to accept the program, but now it generates invalid C++ code. I had to pour in a number of "data" qualifiers to get stanc3 to compile it to C++...that's OK.

However, I would like to get a working C++ obviously instead of a compiler error.

oral_1cmt_mm_run_stan.txt

EDIT: It appears to me that there is a major change here. In stan2 it was possible to pass data subsets to another function. That other function then was only needed to have a real[] argument and that was just fine. So the other function then worked for passing in data OR vars. This is no longer possible now. With stanc3 I have to declare the argument as data real[] in order to get the desired behavior for data subsets. Thus, the function cannot be used any more with data subsets or vars! The possibility of stan2 to allow for data or vars was to my knowledge not by accident, but rather by design. I think that should be allowed in stanc3 and it is supposed to be allowed to my understanding of the language.

[wds15]

Ok... so here is a minimal example which should give the key elements of the big example. So the following program is just fine under Stan 2 (I can compile to C++ and I can compile that C++ to binary):

functions {
  real foo(data real[] inputs, real num) {
    return sum(inputs) + num;
  }

  real bar(real[] inputs, vector more_inputs, real num, int day) {
    real res = 0;
    if (day == 1) {
      res = num + sum(inputs) + foo(to_array_1d(more_inputs[1:2]), num);
    } else {
      res = sum(inputs) + foo(to_array_1d(more_inputs[1:2]), num);
    }
    return res;
  }
}
data {
}
transformed data {
  real some_data[5] = {1., 2., 3., 4., 5.};
  vector[3] other_data = [1., 2., 3.]';
}
parameters {
  real theta;
}
model {
  real temp1 = foo(some_data, theta);
  real temp2 = bar(some_data, other_data, theta, 1);
  theta ~ normal(temp1, temp2);
}

This is legal Stan2 syntax, but the stanc3 compiler will propagate the to_array_1s(more_data[1:2]) expression to become a var and thus it will refuse to compile it and it complains as:

--- Translating Stan model to C++ code ---
bin/stanc  --o=../stanc3-examples/data-munging.hpp ../stanc3-examples/data-munging.stan

Semantic error in '../stanc3-examples/data-munging.stan', line 9, column 32 to column 71:
   -------------------------------------------------
     7:      real res = 0;
     8:      if (day == 1) {
     9:        res = num + sum(inputs) + foo(to_array_1d(more_inputs[1:2]), num);
                                         ^
    10:      } else {
    11:        res = sum(inputs) + foo(to_array_1d(more_inputs[1:2]), num);
   -------------------------------------------------

Ill-typed arguments supplied to function 'foo'. Available signatures:
(data real[], real) => real
Instead supplied arguments of incompatible type: real[], real.

However, it would be a huge limitation if this would be disallowed in the future. All I want is that sub-expressions formed from data-only things will stay data only. This was totally fine in the Stan2 world... I don't really see why this should change. I am also very sure that this is intended behavior.

[seantalts]

I don't understand - this still seems like a bug in Stan 2 if this worked there. Here you have an array filled with a parameter (theta gets passed in as num), so it seems pretty clear that it can't be marked data. If you fix the signature and pass in a transformed data variable for num instead of theta, it works fine.

[wds15]

Nope. This is clearly a bug in stanc3 not being able to deal with nested things... and here is the example which should illustrate the point:

functions {
  real foo(data real[] inputs, real num) {
    return sum(inputs) + num;
  }

  real bar(real[] inputs, vector more_inputs, real num, int day) {
    real res = 0;
    // comment out this line => and things work
    res = num + sum(inputs) + foo(to_array_1d(more_inputs[1:2]), num);
    return res;
  }
}
data {
}
transformed data {
  real some_data[5] = {1., 2., 3., 4., 5.};
  vector[3] other_data = [1., 2., 3.]';
  real derived_data_1 = foo(to_array_1d(other_data[1:2]), 3.0);
}
parameters {
  real theta;
}
transformed parameters {
  // here we pass to_array_1d(other_data[1:2]) to a data only
  // argument. So we form a subexpression which involves only data and
  // we can pass this to the data-only argument of foo.
  real derived_2 = foo(to_array_1d(other_data[1:2]), theta);
}
model {
  real temp1 = foo(some_data, theta);
  real temp2 = bar(some_data, other_data, theta, 1);
  theta ~ normal(temp1, temp2);
}

So in the transformed parameters it is legal to have a sub-expression which involves only data to be passed to a data only argument of foo. Thus, sub-expressions with only data should always output data.

But the same thing is NOT allowed at the moment inside bar. There the more_inputs argument is being instantiated with a data only argument. The sub-expression I am forming (same as in transformed parameters) is wrongly being cast into a var. This makes stanc3 reject the code as it cannot pass this into the first argument of foo.

This is a major problem for most ODE models in Stan. Whenever you call the ODE functions in Stan and want to act on subsetted data within functions, then this is not anymore possible.

The reason why this shows clearly that this is a stanc3 bug to me is that in transformed parameters things work as they should while the same is rejected if nested in a function.

[seantalts]

If there was a spec, I think it would say that you need to mark bar's more_inputs as data, right? In stanc3 we keep track of the autodiff level in the type system and check based on function labels. It would be much harder to perform this checking if we tried to keep track of all of the possible uses of the function instead of requiring the data annotation on bar more_inputs.

Are you saying there's some reason you can't use this signature for bar:

  real bar(real[] inputs, data vector more_inputs, real num, int day) {
    real res = 0;
    // comment out this line => and things work
    res = num + sum(inputs) + foo(to_array_1d(more_inputs[1:2]), num);
    return res;
  }