codebase review (R scripts)

[wjchulme]

The readme describes what each script is doing. A review for the data_process.R, data_process_long.R, data_selection.R, model_seqtrialcox.R and report_seqtrialcox.R scripts would be really helpful.

The sequential trial approach as outlined in the protocol is not yet implemented here -- instead of choosing recruitment periods where sufficiently many people have been vaccinated on each day, it just runs it across the entire recruitment period (16 sep to 29 dec). Just ignore this discrepancy for now.

[wjchulme]

There's also the functions/ folder which has a couple of scripts in. Many of the functions within are currently commented out. Ignore these ones and I'll tidy up when I'm confident they're not needed. But please do review the ones that are in use!

[milanwiedemann]

just adding this checklist for me as I go through the files

Analysis

• [x] analysis/data_process.R
• [x] analysis/data_process_long.R
• [x] analysis/data_selection.R
• [ ] analysis/model_seqtrialcox.R
• [ ] analysis/report_seqtrialcox.R

Functions

• [x] lib/functions/utility.R
• [x] lib/functions/survival.R
• [x] lib/functions/sampling.R
• [ ] lib/functions/redaction.R

[milanwiedemann]

data_process.R looks good! I noticed that the fct_case_when() only works when the new value is a character (because of TRUE ~ NA_character_) but I think it doesnt matter here

https://github.com/opensafely/booster-effectiveness/blob/5a073fe7d726201d906fed12c1a57f77228bce84/lib/functions/utility.R#L3-L10

this works: mutate(mtcars, new = fct_case_when(cyl <= 5 ~ "asd", TRUE ~ NA_character_))

this returns error mutate(mtcars, new = fct_case_when(cyl <= 5 ~ 1, TRUE ~ NA_character_))

[milanwiedemann]

datra_processing_long.R also looks good, just checking that you want the plus in the d+ in the regex in this script ... I guess it's theoretically possible that there were more than 9 admissions, first example here:

https://github.com/opensafely/booster-effectiveness/blob/5a073fe7d726201d906fed12c1a57f77228bce84/analysis/data_process_long.R#L32

[milanwiedemann]

data_selection.R:

has_sex

• could be more explicit here in case there are unexpected other values, but maybe unnecessary
• has_sex = sex %in% c("F", "M"),
• https://github.com/opensafely/booster-effectiveness/blob/5a073fe7d726201d906fed12c1a57f77228bce84/analysis/data_selection.R#L41

comma

• comma here not needed I think
• https://github.com/opensafely/booster-effectiveness/blob/5a073fe7d726201d906fed12c1a57f77228bce84/analysis/data_selection.R#L75

[milanwiedemann]

model_seqtrialcox.R:

• could add library(fastDummies) at the top of the script here and in report_seqtrialcox.R but maybe better to use :: if you only need one function https://github.com/opensafely/booster-effectiveness/blob/5a073fe7d726201d906fed12c1a57f77228bce84/analysis/model_seqtrialcox.R#L339
• use strict selection to avoid warning msg? Use all_of(matching_variables) instead of matching_variables to silence this message. https://github.com/opensafely/booster-effectiveness/blob/5a073fe7d726201d906fed12c1a57f77228bce84/analysis/model_seqtrialcox.R#L425
• I dont think %>% select(everything()) is needed here? I ran with and without this part and used identical() function to compare ... but you also use it further down in the script so maybe I'm missing something? https://github.com/opensafely/booster-effectiveness/blob/5a073fe7d726201d906fed12c1a57f77228bce84/analysis/model_seqtrialcox.R#L435

[milanwiedemann]

in survival.R:

• oh wow I didnt know that if else in R also works without curly brackets, thought I was looking at a python function, maybe change to be consistent with other if ... else ... statements in your repo? https://github.com/opensafely/booster-effectiveness/blob/5a073fe7d726201d906fed12c1a57f77228bce84/lib/functions/survival.R#L8-L11 https://github.com/opensafely/booster-effectiveness/blob/5a073fe7d726201d906fed12c1a57f77228bce84/lib/functions/survival.R#L22-L25

[milanwiedemann]

in report_seqtrialcox.R you assign rr <- rate/ref_rate but it never gets called in the function itself, maybe not needed? or missing?

• https://github.com/opensafely/booster-effectiveness/blob/5a073fe7d726201d906fed12c1a57f77228bce84/analysis/report_seqtrialcox.R#L121-L133

[milanwiedemann]

in redaction.R:

• you assign col_names_new <- names(x_wide) but never use the object (I think) https://github.com/opensafely/booster-effectiveness/blob/5a073fe7d726201d906fed12c1a57f77228bce84/lib/functions/redaction.R#L531

[milanwiedemann]

in tmerge() I haven't seen this before, but maybe that's a trick I don't know:

• tdc(): https://github.com/opensafely/booster-effectiveness/blob/c7d792a30bc91925fa8681a976f2f1f94c7d831d/analysis/merge_seqtrialcox.R#L236
• event(): https://github.com/opensafely/booster-effectiveness/blob/c7d792a30bc91925fa8681a976f2f1f94c7d831d/analysis/model_seqtrialcox.R#L147