stan-dev / stanc3

The Stan transpiler (from Stan to C++ and beyond).
BSD 3-Clause "New" or "Revised" License
140 stars 44 forks source link

[BUG] Tilde syntax does not properly generate promote_scalar for array arguments #1337

Closed spinkney closed 1 year ago

spinkney commented 1 year ago

This compiles in cmdstan version 2.32.2:

functions {
real test(data array[] real k) {
  return 1.;
  }
}
data {
  int<lower=0> N;
}
transformed data {
   array[0] int x_i;
}
parameters {
  real x;
}
model {
  real z = test(x_i);
  x ~ std_normal();
}
WardBrian commented 1 year ago

Unless I misunderstand the example, this is a feature: https://mc-stan.org/docs/reference-manual/calling-functions.html#argument-promotion

I believe the implementation dates back to #1004

spinkney commented 1 year ago

Interesting. It gave me a terrible error when I did this with the integrate_1d function. At least the error could point to the line where the promotion occurred instead of a bunch of c++ errors.

spinkney commented 1 year ago

Here's the program that doesn't compile and gives c++ errors instead. Note the x_i passed in instead of x_r.

functions {
  real multi_wallenius_integral(real t, // Function argument
                              real xc, array[] real theta, // parameters
                              array[] real x_r, // data (real)
                              array[] int x_i) {
  // data (integer)
  real Dinv = 1 / theta[1];
  int Cp1 = num_elements(x_i);
  int n = x_i[1];
  real v = 1;

  for (i in 2 : Cp1) 
    v *= pow(1 - t ^ (theta[i] * Dinv), x_i[i]);

  return v;
}

real multi_wallenius_lpmf(data array[] int k, vector m, vector p, data array[] real x_r,
                          data real tol) {
  int C = num_elements(m);
  real D = dot_product(to_row_vector(p), (m - to_vector(k[2 : C + 1])));
  real lp = log(integrate_1d(multi_wallenius_integral, 0, 1,
                             append_array({D}, to_array_1d(p)), x_r, k, tol));

  for (i in 1 : C) 
    lp += -log1p(m[i]) - lbeta(m[i] - k[i + 1] + 1, k[i + 1] + 1);

  return lp;
}
}
data {
  int<lower=0> N;
  int<lower=0> C;
  array[N, C + 1] int y;
  vector[C] m;
  real tol;
}
transformed data {
  array[0] real x_r;
   array[0] int x_i;
}
parameters {
  simplex[C] probs;
}
model {
  for (i in 1:N) 
    y[i] ~ multi_wallenius(m, probs, x_i, tol);
}
WardBrian commented 1 year ago

Interesting, it’s definitely possible there’s some missing edge cases with functions like that. I’ll try to investigate

WardBrian commented 1 year ago

Huh, that seems like an edge case of the ~ syntax we missed. Changing the line to

    target += multi_wallenius_lpmf(y[i] | m, probs, x_i, tol);

Fixes compilation, though it could still have a semantic bug depending on your intent.

WardBrian commented 1 year ago

@spinkney Do I have your permission to include that example as a new regression test? I think I have the fix for this ready to go

spinkney commented 1 year ago

That was quick! Yes, please use that.