Existing model not compiling with stanc3 compiler

Developers
1

testing current develop versions of cmdstan et al for CmdStanPy - trying to compile my favorite model - bym2.stan

functions {
  real icar_normal_lpdf(vector phi, int N, int[] node1, int[] node2) {
    return -0.5 * dot_self(phi[node1] - phi[node2])
      + normal_lpdf(sum(phi) | 0, 0.001 * N);
 }
}
data {
  int<lower=0> N;
  int<lower=0> N_edges;
  int<lower=1, upper=N> node1[N_edges];  // node1[i], node2[i] neighbors
  int<lower=1, upper=N> node2[N_edges];  // node1[i] < node2[i]

  int<lower=0> y[N];             // count outcomes
  vector<lower=0>[N] E;          // exposure
  int<lower=1> K;                // num covariates
  matrix[N, K] x;                // design matrix
  real<lower=0> scaling_factor;  // scales the variance of the spatial effects
}
transformed data {
  vector[N] log_E = log(E);
}
parameters {
  real beta0;            // intercept
  vector[K] betas;       // covariates
  real logit_rho;

  vector[N] phi;         // spatial effects
  vector[N] theta;       // heterogeneous effects
  real<lower=0> sigma;   // overall standard deviation
}
transformed parameters {
  real<lower=0, upper=1> rho = inv_logit(logit_rho);
  vector[N] convolved_re = sqrt(rho / scaling_factor) * phi
                           + sqrt(1 - rho) * theta;
}
model {
  y ~ poisson_log(log_E + beta0 + x * betas + convolved_re * sigma);

  beta0 ~ normal(0, 1);
  betas ~ normal(0, 1);
  logit_rho ~ normal(0, 1);
  sigma ~ normal(0, 1);
  theta ~ normal(0, 1);
  phi ~ icar_normal_lpdf(N, node1, node2);
}
generated quantities {
  vector[N] eta = log_E + beta0 + x * betas + convolved_re * sigma;
  vector[N] mu = exp(eta);
  int y_rep[N];
  if (max(eta) > 20) {
    // avoid overflow in poisson_log_rng
    print("max eta too big: ", max(eta));  
    for (n in 1:N)
      y_rep[n] = -1;
  } else {
      for (n in 1:N)
        y_rep[n] = poisson_log_rng(eta[n]);
  }
}

compiler complains about the name of my user-defined function:

ERROR:cmdstanpy:file bym2/bym2.stan, ERROR
 Semantic error in 'bym2/bym2.stan', line 44, column 8 to column 24:
   -------------------------------------------------
    42:    sigma ~ normal(0, 1);
    43:    theta ~ normal(0, 1);
    44:    phi ~ icar_normal_lpdf(N, node1, node2);
                 ^
    45:  }
    46:  generated quantities {
   -------------------------------------------------

~-statement expects a distribution name without '_lpdf' or '_lpmf' suffix.
2

It doesn’t need the _lpdf suffix. The old parser shouldn’t have allowed it. It should be:

phi ~ icar_normal(N, node1, node2);

So the question is what to do going forward?

3

too bad it’s in the published paper on the BYM2 model.

agreed, I should have called it icar_normal except that I went into some detail explaining how the sampling statement works, and this is how the functions that are exposed to the language from the math library work - but now I realize I that this detail should have been explained differently. and I didn’t realize it was a bug in the compiler - I thought that it had been ruled legal.

4

I don’t think anybody realized what was going on given how convoluted the logic around all that name mangling with _log vs. _lpdf and using vs. not using suffixes. This only came up when I was reviewing stanc3 syntax.

That’s why I think it’s an issue of what we should do going forward. We could try to accept it and mark it as deprecated.

5

just to be clear, in the above model, I was correct in naming the function, and explaining the name.
the error was that the sampling statement should have been

phi ~ icar_normal(N, node1, node2)

the mistake I made was thinking that the function name as defined was the one to use in the sampling statement - under the old compiler, both icar_normal and icar_normal_lpdf were OK.

the question is, how many other people have made this error? if it’s common, then it would be nice to flag this behavior but not to disallow it.

6

How about we discuss this at the meeting today?

I think bugs like this should be supported by the stanc3 compiler if its reasonable effort to do so… but we can drop support with stan 3.

That would be my take.

7

There’s an issue about this: https://github.com/stan-dev/stan/issues/2801

8

agreed.

FWIW, the model above is the one that I’ve been presenting at talks and is in the paper on the BYM2 model, and this error wasn’t spotted by anyone - and that might include Bob.

it would be nice if the new compiler were able to provide a better error message - again, naively, I tried to change the function name as well as the function call. and/or the documentation needs to be clearer. it’s not how anything else in the language (current language) works - or is it?

9

I think that’s a bad idea. If “bug compatibility” is necessary, one can always resort to the older stanc. Chances are that if you “temporarily” add support for the bug, you may never get around to removing it or, even worse, introduce new bugs by attempting to remove it.

10

this puts everything on the user. their code which used to compile now doesn’t. there’s an error message that they don’t quite grok. and now they have to figure out how to use the old compiler.

yes it’s more work for us. this was a bug that the old compiler allowed, and IIRC, it was pretty much impossible to fix using the old bad Boost Spirit Qi. so we had to allow it. now that we can handle it, I think we should do the kindest possible thing for our users.

11

The error message is already quite clear: it points to the exact location and the message is very readable. Perhaps it could suggest the correction to make (as the Rust compiler would):

~-statement expects a distribution name without '_lpdf' or '_lpmf' suffix.
Replace 'icar_normal_lpdf' with 'icar_normal'.
2 Likes
12

it’s the “replace” that’s problematic - which is implied in the message as is.

my next move was to remove the “_lpdf” from both the function definition and the function call. please understand, I was trying to fix code that I’d written 1.5 years ago - I’d forgotten all the details. this is the situation that people will be in if something changes that breaks previously working code. the user friendly thing to do is let is go for now and flag it - because not everyone has time for a teaching moment.

13

This isn’t a bug in the sense that it’s a bad idea and breaks things, just that it’s not our preferred syntax.

We can break backward compatibility on a major version bump, next Stan 3. But this isn’t a backward compatibility issue because it’s undocumented behavior. Nevertheless, I think we should treat it like it is as it’s come up in lots of user models, not just this one.

Ideally, when it no longer works (now or in Stan 3), we’ll have informative error messages that’d say things like “can’t use foo_lpdf(…) on right-hand side of ~, use foo(…) instead.”

2 Likes