Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
spinkney opened this issue Jul 17, 2023 · 7 comments · Fixed by #1338
Closed

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

spinkney opened this issue Jul 17, 2023 · 7 comments · Fixed by #1338

Comments

@spinkney
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Member

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

@WardBrian
Copy link
Member

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 WardBrian changed the title [BUG] Can pass array of integers into function that only has array of reals as a valid input [BUG] Tilde syntax does not properly generate promote_scalar for array arguments Jul 18, 2023
@WardBrian
Copy link
Member

@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
Copy link
Contributor Author

That was quick! Yes, please use that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants