-
Notifications
You must be signed in to change notification settings - Fork 5
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
Pull bulk and tail ESS warnings out of EpiNow2 fit #74
Comments
cc: @seabbs |
So, you're proposing that we extract these from the EpiNow2 fit and put them into the diagnostic file? Or that we export them into the flat files (e.g. in the samples file)? |
If @natemcintosh takes this one, I or Micah can help mentor, but I need to understand the goal myself first. |
I was thinking we would look at if bulk/tail ESS was low for (1) all parameters and (2) just Rt and sticking that information in the diagnostic file. That should probably be a discrete flag (is_low_neff_all_params_bulk: TRUE/FALSE) and also a quantitative result (neff_per_iter_all_params_bulk: 0.8). |
Ok, I'm following the goal. I'm still not 100% following though, whether the issue is that we need to calculate n_eff bulk and tail ourselves, or whether those are already output in the epinow2 object. (Not following reference to this EpiNow2 line of code either: https://github.com/epiforecasts/EpiNow2/blob/2e1aa86598f13364c1af05a5a83b3ac7790db41a/R/summarise.R#L599) |
Yeah that may have been the wrong link. This one looks better: https://github.com/epiforecasts/EpiNow2/blob/439cf62cdb249f44a7740027511ea3334bce2136/R/extract.R#L303 We shouldn't calculate the statistics ourselves. We should pull them out of the Stan summary object like we do here: cfa-epinow2-pipeline/R/diagnostics.R Line 77 in e8dc4ac
|
Perfect. Thanks. I can talk @micahwiesner67 through this from here! |
@zsusswein I think that @micahwiesner67 has this. Can you just clarify -- do we need to get rid of the rstan dependency around line 77 of the diagnostic.R file you quoted above, or is that bit interoperable with cmdstanr? |
I'm not sure whether or not it will be interoperable. But I don't think it's necessarily relevant for this issue? In this issue, we should get an approach working to pull out the values from the summary dataframe and in #10 we can work out whether the dataframe is constructed via RStan or with an equivalent |
Ok, I'm fine with that. But if the goal is to drop the rstan dependency entirely, then we will have to switch eventually. |
Sam says they look at the minimum of eff_bulk and eff_tail and pull it out over all vars. We could do this for just Rt to start, but we should probably do it for all now that we're looking at more variables.
We should also look at the per-Rt n_eff as a useful metric.
https://github.com/epiforecasts/EpiNow2/blob/2e1aa86598f13364c1af05a5a83b3ac7790db41a/R/summarise.R#L599
The text was updated successfully, but these errors were encountered: