-
-
Notifications
You must be signed in to change notification settings - Fork 271
Rely more on posterior #1171
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
base: develop
Are you sure you want to change the base?
Rely more on posterior #1171
Conversation
Rewrote some functions to act as wrappers around posterior functions, and swap in posterior functions in check_hmc_diagnostics.R
rstan/rstan/R/monitor.R
Outdated
|
|
||
| out <- as.data.frame(do.call(rbind, out)) | ||
| probs_str <- names(quantile(sims_i, probs = probs, na.rm = TRUE)) | ||
| probs_str <- names(posterior::quantile2(sims_i, probs = probs)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the names that posterior::quantile2 uses the same as the names used by quantile? That is, will these names be any different than in the current version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The names are different; it should be easy to get the names from quantile() and rename the posterior::quantile2() results, would that be preferred?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we'd want to stick to the names that are currently used, so yeah I guess we should rename the posterior::quantile2 results. I just made another comment about some quantiles not being printed. I think this could be related.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, thanks! I put one question in a review comment. As the maintainer, @bgoodri should probably also approve of making this change, but it seems like a good idea to me.
|
Oh and posterior also needs to be added to the DESCRIPTION file |
rstan/rstan/R/monitor.R
Outdated
| quan <- unname(posterior::quantile2(sims_i, probs = probs)) | ||
| quan2 <- posterior::quantile2(sims_i, probs = c(0.05, 0.5, 0.95)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I run monitor() for some reason the quan2 are printed but not the quan. For example:
> monitor(fit1, probs = c(0.37, 0.99))
Inference for the input samples (4 chains: each with iter = 10; warmup = 0):
Q5 Q50 Q95 Mean SD Rhat Bulk_ESS Tail_ESS
y[1] -1.2 0.0 1.3 0.0 0.9 1.19 20 20
y[2] -1.1 -0.1 1.8 0.1 1.0 1.08 20 20
lp__ -1.6 -0.6 -0.1 -0.7 0.4 1.14 20 20There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it could be because the print method print.simsummary is looking for quantile columns with a capital Q but in the monitor function there's this:
probs_str <- names(posterior::quantile2(sims_i, probs = probs))
str_quan <- paste0("Q", probs * 100)
str_quan2 <- paste0("Q", c(0.05, 0.5, 0.95) * 100)
str_mcse_quan <- paste0("MCSE_", str_quan)
colnames(out) <- c("mean", "se_mean", "sd", probs_str, "n_eff", "Rhat",
"valid", str_quan2, str_mcse_quan, "MCSE_SD", "Bulk_ESS", "Tail_ESS")
Here probs_str will have a lowercase q but the others will have capital Q.
…t run the test locally [ci skip]
|
I added a snapshot based on an example model Jonah wrote for |
|
I think for testing monitor we should pass it a fixed set of posterior draws instead of creating a stanfit object (it can handle both). That's because with different hardware and different compilers results can be slightly different. So we'd have to set a tolerance but that could be fragile. I just pushed a commit that updates the test to do this. |
rstan/rstan/R/monitor.R
Outdated
|
|
||
| out <- as.data.frame(do.call(rbind, out)) | ||
| probs_str <- names(posterior::quantile2(sims_i, probs = probs)) | ||
| probs_str <- names(quantile(sims_i, probs = probs, na.rm = TRUE)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't working correctly either, still not being printed. I will look into this.
|
@bgoodri When reviewing this PR I noticed that the monitor function is a bit of a mess (not @VisruthSK's fault, this was the case before this PR). I don't understand why there's a |
|
Thanks for generating the snapshot. |
Uses more
posteriorfunctions inmonitor.Rfor various functions. Exported functions are replaced with minimal wrappers, others are removed.There are no tests for
monitor.R, but running some examples seems to be ok. I can add tests if required (probably just snapshots using those examples).Fixes #1169.
@jgabry @avehtari