PBK review of measles template

[pbkeating]

The comments refer to measles template

• Worth specifying "Cases of measles by week of onset"?
  

  sitrep/inst/rmarkdown/templates/measles_outbreak/skeleton/skeleton.Rmd

  Line 123 in 9b44e40

  title = "Cases by week of onset",

  
  Alex: no - unecessary niggling between templates. The header of the document is measles outbreak report.

• Very minor but the generated dataset is in an unusual order ( just not what you'd expect )
  

  sitrep/inst/rmarkdown/templates/measles_outbreak/skeleton/skeleton.Rmd

  Line 140 in 9b44e40

  linelist_raw <- gen_data("Measles")

  
  Alex: yeah unsure when this happened but need to look in to.

• Could elaborate on what epitrix will do here, as it doesn't do anything to the dummy dataset
  

  sitrep/inst/rmarkdown/templates/measles_outbreak/skeleton/skeleton.Rmd

  Line 226 in 9b44e40

  cleaned_colnames <- epitrix::clean_labels(colnames(linelist_raw))

  
  Alex: not sure more explanation would help here - kind of explains itself in the next line?

• May want to clarify what this section of code is doing with the data dictionary, such that the values from the variables are originally recorded as option code in the dataset will be recoded to values of option_name, or something to that effect - awesome function!
  

  sitrep/inst/rmarkdown/templates/measles_outbreak/skeleton/skeleton.Rmd

  Lines 237 to 241 in 9b44e40

  	linelist_cleaned <- clean_variable_spelling(linelist_cleaned,
  	wordlists = filter(linelist_dict,
  	!is.na(option_code)),
  	spelling_vars = "data_element_shortname",
  	sort_by = "option_order_in_set")

  
  Alex: @zkamvar is this function still a thing or is it replaced by something else? Maybe an expalanation as above wouldnt hurt.
  Zhian: @aspina7 This has been replaced by matchmaker::match_df(), which also does not need us to rearrange the data beforehand or filter out keys with missing data either. The process is described here: https://r4epi.github.io/epidict/#cleaning-data-with-the-dictionaries

• Why do we specifically say "do no stratify by gender" here?
  

  sitrep/inst/rmarkdown/templates/measles_outbreak/skeleton/skeleton.Rmd

  Line 460 in 9b44e40

  strata = NULL) %>% # do not stratify by gender

  
  Alex: because the original function was written for age and sex, comment could just read do not stratify.

• would you consider using the summarytools package here? e.g. print(dfSummary(x), method = "viewer")
  

  sitrep/inst/rmarkdown/templates/measles_outbreak/skeleton/skeleton.Rmd

  Line 509 in 9b44e40

  str(linelist_cleaned)

  
  Alex: yes, but kate wanted this removed. We had it commented out and the package wasnt in dependents - think it might be quite a heavy one so not sure if worth adding back in. Maybe for the website? @zkamvar thoughts?
  Zhian: I think this just needs to be an opinionated decision best for what the user will be expecting. All of the summary packages can be a bit heavy. I like {skimr}, but that doesn't print well to a PDF/Word document, though if it's purely for interactive use, then there's no reason why we can't use {skimr} or {summarytools}, either one will work. Also, we can place things in suggests for the sitrep package and instruct users to use install.packages("sitrep", dep = TRUE).
  Alex: have added dfsummary back in and added summarytools to the suggests in DESCRIPTION - so we need to update install instructions, issue update website install instructions #264

• May be worth explaining ceiling = TRUE here as it isn't used in lines 687-688
  

  sitrep/inst/rmarkdown/templates/measles_outbreak/skeleton/skeleton.Rmd

  Lines 683 to 685 in 9b44e40

  	linelist_cleaned$age_group_mon <- age_categories(linelist_cleaned$age_months,
  	breakers = c(0, 6, 9, 12, 24),
  	ceiling = TRUE)

  
  Alex: could but help file...

• Could be worth mentioning which variables will be created, so that users know what to expect?
  

  sitrep/inst/rmarkdown/templates/measles_outbreak/skeleton/skeleton.Rmd

  Lines 730 to 731 in 9b44e40

  	This chunk will help you construct new variables from other variables. It
  	includes numeric, factor, and character vectors.

  
  Alex: would involve a big old wall of text and we decided against this in a previous review round.

• any reason there are brackets around date_of_last_vaccination?
  

  sitrep/inst/rmarkdown/templates/measles_outbreak/skeleton/skeleton.Rmd

  Lines 829 to 833 in 9b44e40

  	linelist_cleaned <- linelist_cleaned %>%
  	mutate(date_of_last_vaccination = ifelse(
  	previously_vaccinated != "Yes",
  	(date_of_last_vaccination), as.Date(NA)
  	))

  
  Alex: no pretty sure this can be removed, @zkamvar?
  Zhian: yes this can be removed

• may be worth highlighting what complete is doing here?
  

  sitrep/inst/rmarkdown/templates/measles_outbreak/skeleton/skeleton.Rmd

  Lines 1224 to 1237 in 9b44e40

  	linelist_cleaned %>%
  	filter(patient_facility_type == "Inpatient") %>%
  	case_fatality_rate_df(deaths = DIED, group = age_group, mergeCI = TRUE, add_total = TRUE) %>%
  	tidyr::complete(age_group,
  	fill = list(deaths = 0,
  	population = 0,
  	cfr = 0,
  	ci = 0)) %>% # Ensure all levels are represented
  	rename("Age group" = age_group,
  	"Deaths" = deaths,
  	"Cases" = population,
  	"CFR (%)" = cfr,
  	"95%CI" = ci) %>%
  	knitr::kable(digits = 1)

  
  Alex: yeah could say that it fills in 0 where category levels not represented.

• worth reminding them that this was predefined earlier?
  

  sitrep/inst/rmarkdown/templates/measles_outbreak/skeleton/skeleton.Rmd

  Line 1484 in 9b44e40

  epicurve_labels +

  
  Alex: part of the website walkthrough.

• by specifying e = 4, merge_ci_df() by default takes the next two columns as lower and upper CI, right?
  

  sitrep/inst/rmarkdown/templates/measles_outbreak/skeleton/skeleton.Rmd

  Line 1941 in 9b44e40

  merge_ci_df(e = 4) %>% # merge lower and upper CI in to one column

  
  Alex: yes - should be in help file.