I1728: Create burden estimate templates in jenner#17
I1728: Create burden estimate templates in jenner#17xiangli313 wants to merge 3 commits intomasterfrom
Conversation
richfitz
left a comment
richfitz
left a comment
There was a problem hiding this comment.
It might be most efficient to work through this one together Xiang - can you pick a time next week?
| whisker, | ||
| yaml | ||
| yaml, | ||
| dplyr |
There was a problem hiding this comment.
totally minor, but can you order dependencies alphabetically please (but capitals first) - so this goes just above vaultr
| assert_has_files <- function(x, files) { | ||
| msg <- setdiff(files,x) | ||
| if (length(msg) > 0L) { | ||
| stop(sprintf("Missing file '%s' from: %s.", |
There was a problem hiding this comment.
this will behave badly if there are more than one missing file (try it!). Once you've tried it can you write some unit tests for practice?
| ##' @export | ||
| create_burden_template <- function(files, files_path, assert_files = c("Country_Disease_Table.csv", "model_meta.csv", "model_outcomes.csv"), central_templates_only = FALSE) { | ||
| assert_has_files(files, assert_files) | ||
| files <- paste0(files_path, files) |
There was a problem hiding this comment.
You should use file.path to construct full paths (deals with the case where files_path does not include a trailing slash for example
| ##' | ||
| ##' @param files_path the directory that contains those files above | ||
| ##' | ||
| ##' @param assert_files needed meta files |
There was a problem hiding this comment.
I am confused about this - why is this an argument to the function?
| assert_has_files(files, assert_files) | ||
| files <- paste0(files_path, files) | ||
|
|
||
| country_disease_table <- read_csv(files[1]) |
There was a problem hiding this comment.
This is fragile, and the interface is a bit odd, really. I think what you're wanting is:
- the user provides a directory
- by default we look for a pre-defined set of csv files
- the user can override that with their own name?
But this is not quite what happens - you assert the presence of a set of files in assert_files and then read files in order
|
|
||
| model_name <- unique(model_meta_data[model_meta_data$disease == disease_name[j], ]$modelling_group) | ||
|
|
||
| sapply(1:length(model_name), generate_csv, model_name, j) |
There was a problem hiding this comment.
generate_csv returns nothing so don't use csv, use a for loop. Avoid 1:length(foo) in favour of seq_along(foo)
|
|
||
| } | ||
|
|
||
| if (disease_name[j] != "HPV") { |
There was a problem hiding this comment.
this should be an else against the previous if
|
|
||
| outcome_name <- unique(model_outcomes[model_outcomes$disease == "standard" & model_outcomes$model == "standard", ]$outcome) | ||
|
|
||
| if (disease_name[j] == "Hib3" & model_name[i] == "JHU-Tam") { |
There was a problem hiding this comment.
why is there so much repetition here?
| write.csv(country_data_stochastic, file = title_stochastic, row.names = FALSE) | ||
| write.csv(country_data, file = title, row.names = FALSE) | ||
|
|
||
| return() |
| return(countries_for_model) | ||
| } | ||
|
|
||
| make_filled_columns <- function(model_meta_line, |
There was a problem hiding this comment.
can you use a more informative name here?
|
OK, what we have here is Nick's R-style. It may take me some time to understand those functions and clear those requested/suggested changes. |
2 participants
Youtrack:
https://vimc.myjetbrains.com/youtrack/issue/VIMC-1728
Task:
Move Nick's script - generate burden templates - to jenner
Past test:
Yes, locally. You need to pull from jenner-test-data. I added test data.