Stan model changes#28
Conversation
There was a problem hiding this comment.
Pull request overview
Updates the simulation study vignette to reflect an expanded set of distribution families and a richer set of evaluation metrics/plots.
Changes:
- Expands the vignette description to include Burr XII and Generalised Gamma (GG) and adds an identifiability note for GG.
- Reworks the results-loading and summarisation narrative and adds additional predictive-performance sections (P50/P95 coverage & bias, IQD/WIS).
- Adds a grouped summary table for aggregated performance metrics.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 3 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 5 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 6 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| n_skipped <- sum(!is.na(res_out$skipped_reason)) | ||
| if (n_skipped > 0L) |
There was a problem hiding this comment.
res_out$skipped_reason is used without checking the column exists. If the bundled results were produced by the legacy runner (which doesn’t include skipped_reason), this chunk will error even though the rest of the vignette could still run. Consider guarding with "skipped_reason" %in% names(res_out) (or defaulting to 0 skipped) before computing/printing skipped rows.
| n_skipped <- sum(!is.na(res_out$skipped_reason)) | |
| if (n_skipped > 0L) | |
| has_skipped_reason <- "skipped_reason" %in% names(res_out) | |
| n_skipped <- if (has_skipped_reason) sum(!is.na(res_out$skipped_reason)) else 0L | |
| if (has_skipped_reason && n_skipped > 0L) |
| n_skipped <- sum(!is.na(res_out$skipped_reason)) | ||
| if (n_skipped > 0L) | ||
| cat(sprintf("Note: %d rows excluded as non-identifiable (skipped_reason = '%s').\n", | ||
| n_skipped, unique(na.omit(res_out$skipped_reason)))) |
There was a problem hiding this comment.
sprintf("... '%s'", unique(na.omit(res_out$skipped_reason))) will recycle/print multiple strings if there is more than one skip reason (e.g., both GG and Gamma heuristics). Collapse the unique reasons into a single string (e.g., paste(..., collapse=", ")) so the output is deterministic and doesn’t warn.
| n_skipped, unique(na.omit(res_out$skipped_reason)))) | |
| n_skipped, paste(unique(na.omit(res_out$skipped_reason)), collapse = ", "))) |
| library(kableExtra) | ||
|
|
There was a problem hiding this comment.
The vignette uses kableExtra, but it is not listed in DESCRIPTION (Imports/Suggests). In a clean build this will fail at render time. Add kableExtra to Suggests or switch to a dependency already present (e.g., knitr::kable() only).
| library(kableExtra) |
| summary_type_label = factor(summary_type, levels = 1:5, | ||
| labels = c("Median+Range", "Median+IQR", | ||
| "Mean+SD", "Freq Table", "Mixed")), | ||
| n_obs_bucket = case_when( | ||
| n_obs == 5 ~ "5", | ||
| n_obs == 10 ~ "10", | ||
| n_obs == 20 ~ "20", | ||
| n_obs > 25 ~ "25+", | ||
| ) %>% factor(levels = c("5", "10", "20", "25+")), | ||
| n_datasets_bucket = case_when( | ||
| n_datasets < 10 ~ "<10", | ||
| n_datasets < 20 ~ "<20", | ||
| n_datasets < 30 ~ "<30", | ||
| n_datasets >= 30 ~ "30+", | ||
| ) %>% factor(levels = c("<10", "<20", "<30", "30+")) |
There was a problem hiding this comment.
This introduces/relies on trailing commas in case_when() calls (e.g., after the last condition). Trailing commas require R >= 4.1, but DESCRIPTION doesn’t declare a minimum R version. Either remove the trailing commas for compatibility or add an explicit Depends: R (>= 4.1) so the package parses on supported R versions.
| by = "scenario_name") %>% | ||
| # summary_type is already present in res_out when produced by the new runner. | ||
| # If a legacy scenarios data frame is supplied, join summary_type from it | ||
| # (dropping any existing column first to avoid .x/.y suffixes). |
There was a problem hiding this comment.
The comment mentions “dropping any existing column first to avoid .x/.y suffixes”, but the code no longer drops summary_type (and the join only happens when it’s absent). Please update or remove the outdated comment to avoid confusion about the join behavior.
| # (dropping any existing column first to avoid .x/.y suffixes). | |
| # only when it is missing from res_out, avoiding .x/.y suffixes. |
Improve simulation study vignette