Skip to content

Bayesian_Cognitive_Modeling: extracted Stan code from R files; updated some deprecated Stan functions #233

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gilleslijnzaad
Copy link

This is my first pull request ever, so apologies if I did something wrong.

In many of the example models for Bayesian_Cognitive_Modeling the Stan code was defined as a string in the R file. In the README, @martin-smira was requesting that someone extract this code and create separate Stan files for them. So that's what I did.

I also changed some deprecated Stan functions, such as: <- is now =, increment_log_prob(u) is now target += u.

Copy link
Member

@bob-carpenter bob-carpenter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for submitting. This is a lot of feedback, but it should all be simplifying and you can apply the principles elsewhere.

My only concern is that we put some kind of red flag up for those bad "uninformative" priors.

@@ -0,0 +1,25 @@
// #### Notes to Stan model #######################################################
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cut all of these disclaimers. This is a very simple censored model! You don't have to go deep I the User's Guide---you can just refer people to the chapter on censoring.

Did that prior come from the book? We usually only recommend hard interval constraints (here 0.25--1.0) when there are physical constraints on the model.

@@ -40,7 +16,7 @@ parameters <- c("theta")

# The following command calls Stan with specific options.
# For a detailed description type "?rstan".
samples <- stan(model_code=model,
samples <- stan(file="ChaSaSoon.stan",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you really want to go to town, converting everything to cmdstanr would be a welcome improvement. RStan just can't keep up with CRAN's demands, so it's usually 1--2 years behind the Stan releases. For instance, it's now at 2.32, which is about 1.5 years old and we're about to release 2.37.

@@ -0,0 +1,42 @@
// #### Notes to Stan model #######################################################
// ## The model is not very effective.
// ## 1) Don't change seed or lower iterations. This model converges slowly so if
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just bad advice. I know it's not yours, but please just cut all this stuff out.

If the model only converges with a given seed, it's essentially broken and the model should be fixed. Fixing seeds does not guarantee the same behavior across platforms (C++ compiler version, C++ compiler options, different OSes, different hardware, etc.---they all affect floating point behavior).

}

transformed parameters {
real<lower=0> sigma;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be

real<lower=0> sigma = inv_sqrt(lambda);

But overall, for new models, I'd just model sigma directly. It only looks so roundabout because it was forced to use conjugate priors in BUGS for efficiency.

// Common Precision
lambda ~ gamma(0.001, 0.001);

// Which Side is Time of Change Point?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't document things that are obvious. I'd say all of this doc is useless.

Also, those priors on mu and lambda are a really bad idea---these attempts at being "uninformative" were standard in BUGS, but we've learned a lot in 30 years.

https://projecteuclid.org/journals/bayesian-analysis/volume-1/issue-3/Prior-distributions-for-variance-parameters-in-hierarchical-models-comment-on/10.1214/06-BA117A.full

So I wonder if some of these translate models are doing more harm than good. We definitely don't recommend that people follow this strategy for priors.

@@ -0,0 +1,42 @@
// #### Notes to Stan model #######################################################
// ## Stan has hypergeometric distribution implemented, so in one way the code
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, just drop all this.

// ## Survey example).
// ################################################################################

// Planes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drop this comment---it's outside the data block and it's not at all clear what it means!

}

transformed data {
int<lower=x> tmin;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one liner

else
lp_parts[t] = log(1.0 / tmax) + hypergeometric_lpmf(k | n, x, t - x);
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for (t in 1:tmax) {
  lp_parts[t] = -log(tmax) + (t < tmin) ? negative_infinity() : hypergeom...;

log(1 / x) = -log(x) and it avoids a division.

}

generated quantities {
int<lower=tmin, upper=tmax> t;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

int<lower=1, upper=tmax> t = categorical_logit_rng(lp_parts);

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 this pull request may close these issues.

2 participants