stan-dev / example-models

Example models for Stan
http://mc-stan.org/
775 stars 477 forks source link

updates to ARM model for normal_id_glm - variable name "cov" should be "x_as_mat" or something like that #216

Open mitzimorris opened 2 years ago

mitzimorris commented 2 years ago

the ARM models which have been updated for normal_id_glm have introduced a transformed data variable named "cov" - not sure what this name is supposed to be, but it's definitely not a covariance matrix. perhaps @andrjohns can explain?

would it be OK to change to "x_mat" or something like that?

./ARM/Ch.10/ideo_interactions.stan
15-}
16-model {
17:  score1 ~ normal_id_glm(cov, alpha, beta, sigma);
18-}

./ARM/Ch.10/ideo_two_pred.stan
14-}
15-model {
16:  score1 ~ normal_id_glm(cov, alpha, beta, sigma);
17-}

./ARM/Ch.12/radon_no_pool.stan
23-  
24-  a ~ normal(mu_a, sigma_a);
25:  y ~ normal_id_glm(cov, a[county], beta, sigma_y);
26-}

./ARM/Ch.12/radon_complete_pool.stan
14-model {
15-  sigma ~ cauchy(0, 2.5);
16:  y ~ normal_id_glm(cov, alpha, beta, sigma);
17-}

./ARM/Ch.12/radon_no_pool_chr.stan
23-  sigma_y ~ cauchy(0, 2.5);
24-  
25:  y ~ normal_id_glm(cov, alpha[county], beta, sigma_y);
26-}

./ARM/Ch.12/radon_group.stan
28-  sigma_beta ~ cauchy(0, 2.5);
29-  
30:  y ~ normal_id_glm(cov, alpha[county], beta, sigma);
31-}

./ARM/Ch.12/radon_group_chr.stan
24-  alpha ~ normal(mu_a, sigma_a);
25-  
26:  y ~ normal_id_glm(cov, alpha[county], beta, sigma);
27-}

./jupyter/radon/stan/radon_complete_pool.stan
14-model {
15-  sigma ~ cauchy(0, 2.5);
16:  y ~ normal_id_glm(cov, alpha, beta, sigma);
17-}
mitzimorris commented 2 years ago

apologies for blaming the wrong person - @WardBrian - why the name "cov"?

possible names: design_matrix, predictors, xs? xs seems best.

mitzimorris commented 2 years ago

why this:

data {
  int<lower=1> N;
  vector[N] x;
  vector[N] y;
}
transformed data {
  matrix[N, 1] xs = [x']';
}

instead of:

matrix[N, 1] xs = to_matrix(x);

is to_matrix less efficient? is it so inefficient that we don't want it in the example models?

WardBrian commented 2 years ago

I didn’t rename them, my name might show up in the blame because I ran the canonicalizer on the models a few months ago

As for why the coding was done that way, I don’t believe that will be more efficient than to_matrix. The implementation of to_matrix for vector types is just a forwarding constructor

mitzimorris commented 2 years ago

@WardBrian apologies - I was right the first time - https://github.com/stan-dev/example-models/pull/200

I've spent the past 30 minutes trying to find announcements or guidance on when normal_id_glm was introduced. it looks to me like a bunch of devs added stuff and assumed that what they added was just obvious. for our users, myself included, it's not. this is an ongoing problem - we get fabulous contributions to the code from sophisticated developers but the users need it explained to them like they're 7 - not that there's anything wrong with being 7.

mitzimorris commented 2 years ago

As for why the coding was done that way, I don’t believe that will be more efficient than to_matrix. The implementation of to_matrix for vector types is just a forwarding constructor

I think I see why this was done - in a few models, you need to past together several inputs, and you end up writing things like this:

data {
  int<lower=1> N;
  int<lower=1> J; // number of counties
  array[N] int<lower=1, upper=J> county;
  vector[N] u;
  vector[N] x;
  vector[N] y;
}
transformed data {
  matrix[N, 2] xs = [x', u']';
}

to_matrix requires something like this:

matrix[N, 2] xs = to_matrix({x', u'})';

clear? does it blend? will test.

mitzimorris commented 2 years ago

the names of the chapter 12 ARM models make no sense, and the description in the README is wrong. these were developed 9 years ago by a Columbia undergrad and no one had the time to check them.

"no-pooling" and "complete-pooling" should be non-hierarchical models.

any model with "chr" - which perhaps stands for "centered hierarchical regression" - just a guess - is a "partial pooling" model.

I doubt any of this is worth fixing, OTOH, keeping this around is an equally bad option. we should archive this altogether.