-
Notifications
You must be signed in to change notification settings - Fork 9
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
Create data comparing aggregated Naomi and national Spectrum programme data #443
Conversation
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.
This looks great, a few comments. Mainly about need to potentially run this with one of anc or art missing.
I also wonder if we should move this out of input-time-series.R
that is getting quite long and this is data aggregation for a different plot.
R/input-time-series.R
Outdated
# Get spectrum level to select correct area names | ||
spectrum_region_code <- unique(shape$spectrum_region_code) | ||
|
||
if(length(spectrum_region_code) > 1){spectrum_level <- 1}else{spectrum_level <- 0} |
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 also do
spectrum_level <- as.integer(length(spectrum_region_code) > 1))
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.
Yup much better - thanks!
R/input-time-series.R
Outdated
##' @param pjnz Path to zip file containing spectrum pjnz file/s | ||
##' @keywords internal | ||
|
||
prepare_spectrum_naomi_comparison <- function(art, anc, shape, pjnz){ |
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 we need to handle the case when either anc or art is missing? Users could upload 1 but not the other.
I'd change signature to
prepare_spectrum_naomi_comparison(shape, pjnz, art = NULL, anc = NULL)
Then have a check,
if (!is.null(art)) {
if(!inherits(art, c("spec_tbl_df","tbl_df","tbl","data.frame" ))) {
art <- read_art_number(art, all_columns = TRUE)
}
art_comparison <- prepare_art_spectrum_comparison(art, shape, pjnz)
}
and similar for ART. Then only need to bind if both present.
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.
Can you add a quick test for this too please? You can mock our prepare_art_spectrum_comparison
and prepare_anc_spectrum_comparison
to just test that it reads the data and that handles case above if ANC or ART are missing
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.
Have added both and to produce an empt table when user provides no programme data. Kept rbind
in to be able to bind empty df.
R/input-time-series.R
Outdated
art_comparison <- prepare_art_spectrum_comparison(art, shape, pjnz) | ||
anc_comparison <- prepare_anc_spectrum_comparison(anc, shape, pjnz) | ||
|
||
rbind(art_comparison, anc_comparison) |
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.
Possibly worth using dplyr::bind_rows
here. I think it should generally be faster for large data frames (makes a big difference with a list of data frames) but seems to even with test data here
Browse[1]> bench::mark(rbind(art_comparison, anc_comparison), dplyr::bind_rows(art_comparison, anc_comparison))
# A tibble: 2 × 13
expression min median `itr/sec` mem_alloc `gc/sec` n_itr n_gc total_time result memory time gc
<bch:expr> <bch:> <bch:> <dbl> <bch:byt> <dbl> <int> <dbl> <bch:tm> <list> <list> <list> <list>
1 rbind(art_compar… 68.9µs 89.9µs 10047. 4.31KB 4.32 4648 2 463ms <tibble> <Rprofmem> <bench_tm> <tibble>
2 dplyr::bind_rows… 47.5µs 63.3µs 13903. 4.01KB 4.74 5867 2 422ms <tibble> <Rprofmem> <bench_tm> <tibble>
Co-authored-by: Rob <39248272+r-ash@users.noreply.github.com>
…com/mrc-ide/naomi into spectrum-naomi-comparison-data-prep
…nction and format output
1c788e5
to
f13a307
Compare
To do: