Adding functions to produce impact, coverage and fvp plots by zegibney · Pull Request #15 · vimc/vimcheck

zegibney · 2026-04-29T11:33:20Z

This PR adds additional functions from [https://github.com/vimc/montagu-reports/tree/main/src/rapid-model-run-impact] a key report for the VIMC Science & Policy team.

Functions added generate central estimate impact plots and coverage and fvp plots per year, country and view.

First attempt at ensuring functions are package-ready/ utilising roxygen etc. so any comment welcome.

TODO's in script fn_plotting_impact.R with specific questions. I understand testing may need to be stronger/ the next step

…ots central impact estimates, coverage and fvps. Associated documentation also added from attempts at roxygen!

pratikunterwegs · 2026-04-30T11:05:47Z

Hi @zegibney - I'm taking a look at the code now. Just a few preliminary points:

I see that some package checks haven't launched. That's because of I've set up the GH Actions config files. Could you change Line 7 on .github/workflows/R-CMD-check.yaml to replace [main, master] with "*"? And the same in .github/workflows/lint-changed-files.yaml? That should allow these checks to run on PRs to any branch.
The {pkgdown} check is failing because the functions you've added don't have a place to live in the online docs. The organisation of the docs is controlled in _pkgdown.yml. Could you add the names of the functions you've added to this file. See the current organisation to get a sense of which section (determined by title) or subsection (subtitle) they should go under. Or add a new (sub)section if needed.

pratikunterwegs

I've just added some comments here. We should probably think of a testing strategy as well, but that's a bit bound up with the overall issue of how to provide data for testing as in PR #14.

pratikunterwegs · 2026-04-30T10:55:07Z

@@ -0,0 +1,240 @@
+#' Plot central impact estimates by cohort and year. 
+#' TODO: need to add importFrom ... to avoid package issues with testing? 


The way @importFrom is used is to import functions from packages that are used repeatedly, really only to avoid writing out the explicit namespacing each time (i.e., ggplot2::fn()). Tests might fail if you neither import nor explicitly namespace a function, but the tag isn't otherwise linked to test failures.

pratikunterwegs · 2026-04-30T10:57:21Z

+#' plot_impact(
+#'   data = impact_data,
+#'   burden_type = "cases",
+#'   title = "Cases averted",
+#'   view = "year"
+#' )


This example will fail as impact_data hasn't been defined. Each @example block is standalone, so you'll need to create impact_data here.

pratikunterwegs · 2026-04-30T11:24:34Z

+    data, 
+    burden_type, 
+    title, 
+    view


The convention for a small number of fixed argument options is to display them in the function signature (i.e., this bit).

So prefer f = function(arg = c("option_1", "option_2")). This way when users pull up the help page they can immediately see the valid options.

This should be paired with some argument checking which you already have. checkmate::assert_choice() is fine; in {vimcheck} I've gone with rlang::arg_match() whose error messages are a bit more informative.

pratikunterwegs · 2026-04-30T11:26:58Z

+    view
+){
+  checkmate::assert_tibble(data, min.rows = 1L, min.cols = 1L)
+  checkmate::assert_character(burden_type, len = 1)


Prefer using rlang::arg_match() here too. If keeping assert_choice(), this assert_character() isn't needed. assert_character(..., len = 1) is better replaced with assert_string() (in other use cases, can simply be removed here).

pratikunterwegs · 2026-04-30T11:33:37Z

+  )
+
+  Impact <- 
+    data %>% 


So I would recommend against using pipes in functions, because any errors that occur can only be traced to the top of the pipeline (here, L. 44), and not to the line where they actually are. It's tedious, but it's better to replace each step in the pipeline with an explicit assignment step.

pratikunterwegs · 2026-04-30T11:46:31Z

+#'     \item{fvps}{A plot of fully vaccinated persons over time.}
+#'   }
+#' @examples
+#' plots <- plot_coverage_fvps(fvps)


Need to define the data fvps.

pratikunterwegs · 2026-04-30T11:48:48Z

+    title, 
+    view
+){
+  checkmate::assert_tibble(data, min.rows = 1L, min.cols = 1L)


Might be good to check that data has at least as many cols as are used in this fn; would also be good to add checks on the specific column names. See e.g. fns in R/fn_plotting_prep_bur_diag.R which use checkmate::assert_names() for this.

pratikunterwegs · 2026-04-30T11:50:23Z

+plot_coverage_fvps <- function(fvps){
+  checkmate::assert_tibble(fvps, min.rows = 1L, min.cols = 1L)


Pretty much the same general suggestions as for the preceding fn.

pratikunterwegs · 2026-04-30T11:51:23Z

+    dplyr::select(
+      .data$country, 
+      .data$vaccine_delivery, 
+      .data$year, 
+      .data$coverage_adjusted) %>%


I'm not sure how advisable this syntax is. I would define the cols to select as a character vector and pass them to select using all_of() or {{ }}.

pratikunterwegs · 2026-04-30T11:52:36Z

+      .data$vaccine_delivery, 
+      .data$year, 
+      .data$coverage_adjusted) %>%
+    dplyr::rename(coverage = .data$coverage_adjusted) 


Here it might be safer to pass a named vector giving the renaming map. See 2nd example in rename docs: https://dplyr.tidyverse.org/reference/rename.html

zegibney added 2 commits April 29, 2026 11:45

Adding pine countries to constants for impact plotting key countries

14f1829

Adding plotting functions from rapid-model-run-impact report which pl…

0d446cd

…ots central impact estimates, coverage and fvps. Associated documentation also added from attempts at roxygen!

zegibney marked this pull request as draft April 29, 2026 11:34

pratikunterwegs reviewed Apr 30, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding functions to produce impact, coverage and fvp plots #15

Adding functions to produce impact, coverage and fvp plots #15
zegibney wants to merge 2 commits intovimc:developfrom
zegibney:develop_zoe

zegibney commented Apr 29, 2026

Uh oh!

pratikunterwegs commented Apr 30, 2026

Uh oh!

pratikunterwegs left a comment

Uh oh!

pratikunterwegs Apr 30, 2026

Uh oh!

pratikunterwegs Apr 30, 2026

Uh oh!

pratikunterwegs Apr 30, 2026

Uh oh!

pratikunterwegs Apr 30, 2026

Uh oh!

pratikunterwegs Apr 30, 2026

Uh oh!

pratikunterwegs Apr 30, 2026

Uh oh!

pratikunterwegs Apr 30, 2026

Uh oh!

pratikunterwegs Apr 30, 2026

Uh oh!

pratikunterwegs Apr 30, 2026

Uh oh!

pratikunterwegs Apr 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		@@ -0,0 +1,240 @@
		#' Plot central impact estimates by cohort and year.
		#' TODO: need to add importFrom ... to avoid package issues with testing?

		plot_coverage_fvps <- function(fvps){
		checkmate::assert_tibble(fvps, min.rows = 1L, min.cols = 1L)

Conversation

zegibney commented Apr 29, 2026

Uh oh!

pratikunterwegs commented Apr 30, 2026

Uh oh!

pratikunterwegs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants