Skip to content
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

Add epiparameter & EpiNow2 #31

Merged
merged 9 commits into from
Oct 25, 2022
Merged

Add epiparameter & EpiNow2 #31

merged 9 commits into from
Oct 25, 2022

Conversation

Bisaloo
Copy link
Member

@Bisaloo Bisaloo commented Oct 20, 2022

Output formatting for EpiNow2 directly copied from what Thibaut wrote for EpiEstim. I'm aiming for uniformity at the moment and will probably write shared wrappers to possibly update the output later (#29).

The number of chains & samples is ridiculously low but it's to allow you to try and render the document locally in a reasonable amount of time.

@joshwlambert
Copy link
Member

The updates look good. I'm not experienced with developing Shiny apps so I may have missed a couple of things. The plots and tables are generated nicely for the report.

A few points on using the app:

  • The epicurve unit bar does not toggle, I’m assuming this is known but wanted to raise in case not.
  • When running for ebola on the default settings an “Error: wrong sign in ‘by’ argument” is produced. This error remains when changing incomplete days and R package.
  • For custom input, when specifying “gamma” or “lnorm” in serial interval distribution, serial interval mean as 2 and serial interval sd as 1 I get “Error: non-numeric argument to binary operator”
  • Could rename “R package” options to something like “Rt estimator”.
  • Often a large print out after “Transmissibility by group”, I’m think from regional_epinow(), could possible hide this from user depending how much detail they want.
  • I’m unsure what the R package option is changing as it seems to be that Rt is calculated using multiple methods independent of the choice of R package.
  • In the actions output, I think the warning for Node.js being deprecated can be solved by updating to actions/checkout@v3

@Bisaloo
Copy link
Member Author

Bisaloo commented Oct 24, 2022

Sorry, I should have been clearer that the shiny app doesn't work at this time. I have currently shelved it to focus on the Rmd file exclusively.

@joshwlambert
Copy link
Member

No worries, will take one more look at the Rmd. Just to check, the output of the rendered Rmd and the report generated by the Shiny app are identical with particular settings?

@Bisaloo
Copy link
Member Author

Bisaloo commented Oct 24, 2022

Just to check, the output of the rendered Rmd and the report generated by the Shiny app are identical with particular settings?

Yes, exactly.

@Bisaloo
Copy link
Member Author

Bisaloo commented Oct 24, 2022

Often a large print out after “Transmissibility by group”, I’m think from regional_epinow(), could possible hide this from user depending how much detail they want.

I must admit I couldn't figure out how to remove this 🙈. Do you have any idea? If not, I'll ask Sam.

@joshwlambert
Copy link
Member

Everything seems correct to me. The report is much more succinct with the output of only a single Rt analysis which I personally like. One small correction: typo on https://github.com/epiverse-trace/data_pipelines/blob/epiparameter/reports/transmissibility.Rmd#L49

#### Estimating $R$ from the growth rates

Wallinga and Lipsitch have introduced methods for converting daily growth rates
($r$) into reproduction numbers ($R$) [@Wallinga2007-hw], when information on
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This citation is not rendered correctly as the bibliography is currently commented out.

@avallecam
Copy link
Member

avallecam commented Oct 24, 2022

Hi all,

one comment from my experience running the transmissibility.Rmd file

  • after running renv::restore() the {epiparameter} package was still not available (fortunately, I'm a fresh user) so I though about using the {pacman} package, replacing

https://github.com/epiverse-trace/data_pipelines/blob/92d4d67032ecc8a8535802dc0bec46f7990e09bb/reports/transmissibility.Rmd#L74

with

if (!require("pacman")) install.packages("pacman")
pacman::p_install_gh("epiverse-trace/epiparameter")

it helped me to render the Rmd, and as far as I read, it should work well with renv.


footnote: For another discussion, I highly encourage to use pacman::p_load() or pacman::p_install_gh() instead of library(). It may be highly useful for novice users to avoid issues right at the beginning of their epiverse-journeys

@avallecam
Copy link
Member

To improve the identification of patterns in bars plots and tables, I mean, looking first at the regions with highest incidence, we can reorder them with respect to the incidence.

adding to this pipe:

https://github.com/epiverse-trace/data_pipelines/blob/92d4d67032ecc8a8535802dc0bec46f7990e09bb/reports/transmissibility.Rmd#L215

this

total_cases <- dat %>%
  select_tags(location, counts) %>%
  group_by(location) %>%
  summarise(cases = sum(counts)) %>%

  # modification ------------- here!
  arrange(location) %>% 
  mutate(location = fct_reorder(.f = location,
                                .x = cases,
                                .desc = F)) %>% 
  arrange(desc(location))

and if looking for uniformidity in this aspect, this could be also included in the Rt estimates plot and table

replace all this chunk

https://github.com/epiverse-trace/data_pipelines/blob/92d4d67032ecc8a8535802dc0bec46f7990e09bb/reports/transmissibility/EpiEstim.Rmd#L137

with (I only moved the filter and pipe it with the fct_reorder)

res_epiestim_group_fct <- res_epiestim_group %>% 
  filter(end == max(end)) %>%
  mutate(region = fct_reorder(.f = region,
                                .x = median,
                                .desc = F)) %>% 
  arrange(desc(region))

# Plot of latest estimates
res_epiestim_group_fct %>%
  ggplot(aes(y = .data[[group_var]]), fill = custom_grey) +
  geom_point(aes(x = median), color = dark_green) +
  geom_errorbar(aes(xmin = lower, xmax = upper), color = dark_green) +
  geom_vline(xintercept = 1, color = dark_pink) +
  labs(title = "Estimates of Rt (EpiEstim)",
       subtitle = sprintf(
         "based on data from %s - %s",
         format(max(get_dates(dat_i_day) - 7), "%d %B %Y"),
         format(max(get_dates(dat_i_day)), "%d %B %Y")),
       y = "",
       x = "Instantaneous Reproduction Number (Rt)")

res_epiestim_group_fct %>%
  mutate(
    mean = round(mean, 2),
    median = round(median, 2),
    `95% ci` = sprintf(
           "[%1.2f ; %1.2f]",
           lower,
           upper)) %>%
  dplyr::select(-c(sd, lower, upper)) %>%
  rename(
    "mean $R$" = mean,
    "median $R$" = median) %>% 
  set_names(toupper) %>%   
  kbl() %>%
  kable_paper("striped", font_size = 18, full_width = FALSE) %>%
  row_spec((1:n_groups)[c(FALSE, TRUE)],
           background = pale_green)

@Bisaloo do your prefer the review to make this edits and push commits? (I think I should've read/write the contribution guideline of the packages 😅)

@avallecam
Copy link
Member

avallecam commented Oct 24, 2022

a last one is also about output aesthetics. The ggplot2 output in y-axis of this "... number\\(Rt)" is ... number\(Rt). Probably, we only need "... number (Rt)".

https://github.com/epiverse-trace/data_pipelines/blob/92d4d67032ecc8a8535802dc0bec46f7990e09bb/reports/transmissibility/EpiEstim.Rmd#L88

https://github.com/epiverse-trace/data_pipelines/blob/92d4d67032ecc8a8535802dc0bec46f7990e09bb/reports/transmissibility/EpiEstim.Rmd#L133

@Bisaloo let me know if this satisfy the uniformidity review that you needed. I can focus on some interpretation text during this week. With respect to the aesthetics of plots and tables, I do not have further comments.

@Bisaloo
Copy link
Member Author

Bisaloo commented Oct 25, 2022

Thanks for your comments:

I highly encourage to use pacman::p_load() or pacman::p_install_gh() instead of library(). It may be highly useful for novice users to avoid issues right at the beginning of their epiverse-journeys

Thibaut initially used pacman but I argued for its removal because I'm not a big fan of using yet another, somewhat uncommon, tool, to sidestep something that we should take the time to explain. I'll reconsider and open an issue for input from others though.

do your prefer the review to make this edits and push commits?

The best would be to do "commit suggestions" via GitHub. This is documented in this (still in progress) chapter of the blueprints.

The ggplot2 output in y-axis of this "... number\(Rt)" is ... number(Rt). Probably, we only need "... number (Rt)"

Yes, I inherited this when I started to work on this doc. Not sure what the intention was 🤷. I'll update it after merging this PR.

@Bisaloo Bisaloo merged commit 55b847c into main Oct 25, 2022
@Bisaloo Bisaloo deleted the epiparameter branch October 25, 2022 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants