-
Notifications
You must be signed in to change notification settings - Fork 0
Separate plotting code from data prep #13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
This pull request:
Reach out on slack ( (Note that results may be inaccurate if you branched from an outdated version of the target branch.) |
|
This pull request:
Reach out on slack ( (Note that results may be inaccurate if you branched from an outdated version of the target branch.) |
|
This pull request:
Reach out on slack ( (Note that results may be inaccurate if you branched from an outdated version of the target branch.) |
1 similar comment
|
This pull request:
Reach out on slack ( (Note that results may be inaccurate if you branched from an outdated version of the target branch.) |
| ) | ||
|
|
||
| #' @name constants | ||
| burden_outcome_names <- c( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we also have deaths_cwyx, cases_cwyx, dalys_cwyx, yll_cwyx, rubella_deaths_congenital and rubella_cases_congenital.
|
This pull request:
Reach out on slack ( (Note that results may be inaccurate if you branched from an outdated version of the target branch.) |
|
This pull request:
Reach out on slack ( (Note that results may be inaccurate if you branched from an outdated version of the target branch.) |
xiangli313
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is looking good. I am approving it; and will be using it once merged (will feedback).
| #' @param year_min Minimum year. | ||
| #' | ||
| #' @export | ||
| prep_plot_fvp <- function(fvp, year_min, year_max) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xiangli313 - I had a question regarding this function that prepares the FVP data for plotting. This function is used to generate the data for the FVPs-over-time plot (usually Fig. 2 in burden diagnostics reports). Where can I find an example of the FVPs dataset that is used to make the FVPs plot? The related plotting function is plot_fvp().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FVP data is derived from both coverage and wpp data. It is calculated given specific version of coverage and wpp. If you just need example dataset, please find fvps.rds from here https://montagu.vaccineimpact.org/packit/rapid-model-run-burden/20251218-112844-86af9b70/downloads
kgaythorpe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fantastic, thanks Pratik, happy to approve from my side
| #' @param coverage WIP. Coverage data. | ||
| #' | ||
| #' @export | ||
| prep_plot_coverage_set <- function(coverage) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xiangli313 - similar question/request for some example coverage set data so that this function can be tested. This function prepares data that is used to generate the coverage set data (usually Fig. 1 in diagnostics reports). The related plotting function is plot_coverage_set().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may use coverage_scenario.csv from here https://montagu.vaccineimpact.org/packit/rapid-model-run-coverage/20251121-174950-8ce647c0/downloads
|
This pull request:
Reach out on slack ( (Note that results may be inaccurate if you branched from an outdated version of the target branch.) |
zegibney
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really good Pratik and very easy to follow, grateful for it all being in one place! A few minor comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor typo in the file name, only figured it out when I was questioning my ability to spell diagnostics!
| checkmate::assert_data_frame(burden_set) | ||
| checkmate::assert_data_frame(wpp) | ||
|
|
||
| gender <- rlang::arg_match(gender) | ||
|
|
||
| cols_to_select <- c("country", "year", "age", "cohort_size") | ||
| cols_to_select <- c("country", "year", "age", "cohort_size", "scenario") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could wait until the end to standardise across files but can we make this scenario_type to match existing conventions? @xiangli313 what do you think? I see in constants.R there is the file dict.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally agree. We used to have scenario/scenario_type as enum tables in montagu. For the future, we can create a similar constant table for vimcheck.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to check - should I change scenario to scenario_type throughout?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, rethink about, let's keep scenario. Usually, for per scenario_type, there could be a few incremental scenarios. And scenario naming convention usually have scenario_type as an suffix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure - I'll keep it as it is.
| @@ -21,3 +21,28 @@ scenario_data_colnames <- c( | |||
| "scenario", | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment about scenario/scenario_type.
|
This pull request:
Reach out on slack ( (Note that results may be inaccurate if you branched from an outdated version of the target branch.) |
|
This pull request:
Reach out on slack ( (Note that results may be inaccurate if you branched from an outdated version of the target branch.) |
|
I'm going to try and get the failing MacOS check to pass - seems like a question of waiting until the runner recovers - and then merge. |
This PR separates the data preparation for plotting into intermediate functions prefixed
prep_plot_*().Known issues:
- Two functions in; now removed.burden_diagnostics.Rare yet to be worked on