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

Vector inputs for ODE models #176

Merged
merged 88 commits into from Mar 14, 2024
Merged

Vector inputs for ODE models #176

merged 88 commits into from Mar 14, 2024

Conversation

pratikunterwegs
Copy link
Member

@pratikunterwegs pratikunterwegs commented Feb 9, 2024

This is a substantial user-facing and internal update to {epidemics}. Please treat any review as a full-package review.

Context

  • The {epidemics} use case was identified to be multiple (100s -- 1000s) runs of each model; this is because:
  • Users are expected to want to incorporate parameter uncertainty directly rather than manually looping over parameters;
  • Users are expected to want to run multiple scenarios with parameter uncertainty, and compare across them without the comparison being biased by differences in the random number draws.
  • See also

Changes in this PR

  • All models have only a single exported version, named model_*(); this is WIP Reconsider R implementations of ODE models #162

    • This is a breaking change with downstream effects on e.g. training materials, cc-ing @avallecam to notify of incoming changes;
    • All ODE models' exported versions call the Rcpp internal functions which use Boost solvers; these are exposed to R as .model_*_cpp()
    • An open question is whether and how best to test the remaining R-only ODE system code
  • All ODE models accept infection parameters (and time_end) as numeric vectors; this fixes ODE model functions should accept vectors of infection parameters #166

    • Model function bodies have been changed to check input infection parameters, check that they are recyclable following Tidyverse rules, and to create a parameter table;
    • Model function bodies now use {data.table} extensively;
    • Passing scalar values to infection parameters returns a simple <data.table> rather than a nested one with a single row. This is to prevent breaking changes to any existing users. It is also probably more appropriate as a single model run is unlikely to require returning the parameters. The return is a <data.table> rather than a <data.frame> to avoid differences in return type. This fixes Return simplified data.frame for scalar inputs #177
  • All ODE models accept lists of intervention sets, and lists of <vaccination> as inputs to intervention and vaccination respectively; this fixes ODE model functions should accept list of model component sets #167

    • An intervention set is a list of <intervention>s; the new input supports lists of lists of <intervention>s
    • It is not yet possible to pass a list to population, time_dependence, or population_change --- it is an open question as to whether the latter two are necessary, but the need for multiple populations is noted in Allow population to be a list-like in epidemic models #181 but not tackled here;
    • {data.table} is used extensively to create intervention combinations within functions
    • Cross-checking inputs such as interventions has been simplified with the introduction of general functions .cross_check_*() which are used in model-specific argument checker/preparation functions; this fixes Refactor argument checker functions to cross-checking only #175
  • An updated and renamed version of the vignette on "Modelling parameter uncertainty" now shows how to pass infection parameters as vectors, and how to pass intervention sets and vaccinations as lists, this fixes Add vignette showing ODE model vectorisation #183

  • All ODE models are now tested more extensively for scalar and vector inputs, as well as error messages; this provisionally fixes Test suites for vectorised ODE model #178

  • The Vacamole model has been restructured as well to match the internal ODE structure; this provisionally fixes Restructure Vacamole model for rate interventions #143:

    • Susceptibility reduction due to vaccination (susc_reduction_vax) is now transmissibility for vaccinated individuals (transmissibility_vax);
    • Hospitalisation reduction due to vaccination (hosp_reduction_vax) is now hospitalisation for vaccinated individuals (hospitalisation_rate_vax);
    • Mortality reduction due to vaccination (mort_reduction_vax) is now hospitalisation for vaccinated individuals (mortality_rate_vax);
    • All arguments have the same default value as earlier, i.e., 80% of the value for unvaccinated individuals;
    • These parameters can now be targeted by <rate_intervention>s
  • Adds @TimTaylor and @adamkucharski as authors

  • Standardises file naming for the R code, the C++ source and header code, vignettes, and test files

Planned changes not in this PR

  • Vector inputs to the stochastic Ebola model;
  • More tests for the stochastic Ebola model;
  • More tests for S3 classes;
  • Handling remaining R-only ODE model code;
  • General package health improvements per {goodpractice} suggestions
  • See this GitHub project for more and/or linked issues.

Copy link

This pull request:

  • Adds 0 new dependencies (direct and indirect)
  • Adds 0 new system dependencies
  • Removes 1 existing dependencies (direct and indirect)
  • Removes 1 existing system dependencies

(Note that results may be inacurrate if you branched from an outdated version of the target branch.)

Copy link
Member

@adamkucharski adamkucharski left a comment

Choose a reason for hiding this comment

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

This new vector options are looking good. Have focused comments on modelling_scenarios.Rmd, particularly around how to improve visibility/explanation of new functionality for users, and giving some outputs that are close to what they'd want in practice (e.g. illustrative trajectories, tables of scenario comparison, estimation of infections averted). Also nice that single value inputs still return a data.frame rather than this getting more complex for users.

Note I had a query about the combined interventions for same parameter value in the final example – I might be missing something, but potentially a useful test to check that multiple interventions have more impact than single one for the same parameters in a deterministic model?

vignettes/modelling_scenarios.Rmd Outdated Show resolved Hide resolved
vignettes/modelling_scenarios.Rmd Outdated Show resolved Hide resolved
vignettes/modelling_scenarios.Rmd Outdated Show resolved Hide resolved
vignettes/modelling_scenarios.Rmd Show resolved Hide resolved
vignettes/modelling_scenarios.Rmd Outdated Show resolved Hide resolved
vignettes/modelling_scenarios.Rmd Show resolved Hide resolved
vignettes/modelling_scenarios.Rmd Show resolved Hide resolved
vignettes/modelling_scenarios.Rmd Outdated Show resolved Hide resolved
vignettes/modelling_scenarios.Rmd Show resolved Hide resolved
Copy link

github-actions bot commented Mar 5, 2024

This pull request:

  • Adds 0 new dependencies (direct and indirect)
  • Adds 0 new system dependencies
  • Removes 1 existing dependencies (direct and indirect)
  • Removes 1 existing system dependencies

(Note that results may be inacurrate if you branched from an outdated version of the target branch.)

1 similar comment
Copy link

github-actions bot commented Mar 5, 2024

This pull request:

  • Adds 0 new dependencies (direct and indirect)
  • Adds 0 new system dependencies
  • Removes 1 existing dependencies (direct and indirect)
  • Removes 1 existing system dependencies

(Note that results may be inacurrate if you branched from an outdated version of the target branch.)

Copy link

github-actions bot commented Mar 6, 2024

This pull request:

  • Adds 4 new dependencies (direct and indirect)
  • Adds 1 new system dependencies
  • Removes 1 existing dependencies (direct and indirect)
  • Removes 1 existing system dependencies

(Note that results may be inacurrate if you branched from an outdated version of the target branch.)

R/model_diphtheria.R Outdated Show resolved Hide resolved
# and convert to list for a data.table list column;
# also check if `intervention` is a list of interventions or a list-of-lists
# and convert to a list for a data.table list column. NULL is allowed;
is_lofints <- checkmate::test_list(
Copy link
Member

Choose a reason for hiding this comment

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

Took me a minute to figure out what the name means. Could you maybe write it in full, i.e., is_list_of_interventions?

Copy link
Member Author

Choose a reason for hiding this comment

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

Leaving the name for brevity, but have made the comments more explanatory in 51cccaf

R/model_diphtheria.R Outdated Show resolved Hide resolved
R/model_vacamole.R Outdated Show resolved Hide resolved
R/model_vacamole.R Outdated Show resolved Hide resolved
R/tools.R Outdated Show resolved Hide resolved
Copy link

This pull request:

  • Adds 4 new dependencies (direct and indirect)
  • Adds 1 new system dependencies
  • Removes 1 existing dependencies (direct and indirect)
  • Removes 1 existing system dependencies

(Note that results may be inacurrate if you branched from an outdated version of the target branch.)

Copy link

This pull request:

  • Adds 4 new dependencies (direct and indirect)
  • Adds 1 new system dependencies
  • Removes 1 existing dependencies (direct and indirect)
  • Removes 1 existing system dependencies

(Note that results may be inacurrate if you branched from an outdated version of the target branch.)

@pratikunterwegs
Copy link
Member Author

Thanks both @jamesmbaazam and @adamkucharski for your reviews. @CarmenTamayo, you mentioned you might have some feedback as well, would you like to add it? Otherwise I think this PR is ready to merge.

@CarmenTamayo
Copy link
Contributor

CarmenTamayo commented Mar 12, 2024

Thanks both @jamesmbaazam and @adamkucharski for your reviews. @CarmenTamayo, you mentioned you might have some feedback as well, would you like to add it? Otherwise I think this PR is ready to merge.

Hi @pratikunterwegs I provided some feedback as well, specifically for the new vignette for modelling parameter uncertainty, can you see my comments? I did this last week -> I hadn't submitted the comments but they're available now- apologies for this

Copy link
Contributor

@CarmenTamayo CarmenTamayo left a comment

Choose a reason for hiding this comment

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

Sorry Pratik, I had pending comments but hadn't actually submitted them

vignettes/modelling_scenarios.Rmd Show resolved Hide resolved
vignettes/modelling_scenarios.Rmd Outdated Show resolved Hide resolved
vignettes/modelling_scenarios.Rmd Outdated Show resolved Hide resolved
vignettes/modelling_scenarios.Rmd Outdated Show resolved Hide resolved
vignettes/modelling_scenarios.Rmd Outdated Show resolved Hide resolved
vignettes/modelling_scenarios.Rmd Outdated Show resolved Hide resolved
vignettes/modelling_scenarios.Rmd Outdated Show resolved Hide resolved
vignettes/modelling_scenarios.Rmd Outdated Show resolved Hide resolved
:::

::: {.alert .alert-info}
### Combinations of intervention and vaccination scenarios
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe including an example of this combination? even if it is just example code, also for users to visualise how to make the list with vaccination and interventions and specifying when there is no intervention/vaccination for the different scenarios
Otherwise it's a bit dry to read without anything to put into practice

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks - how important would you say this is? I ask as this vignette is already quite long, and James has suggested it should be split - would it be more useful to show this in a future vignette instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Opened #194 to address this.

**Note that** 'no intervention' and 'no vaccination' scenarios are not automatically created, and must be passed explicitly as `NULL` in the respective lists.

While the number of intervention and vaccination combinations are not expected to be very many, the addition of parameter uncertainty for each scenario (next section) may rapidly multiply the number of times the model is run.
**Users are advised** to be mindful of the number of scenario combinations they create.
Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned above, the visualisation of combination of intervention & vaccination first without uncertainty and then with uncertainty would be useful for users to see how the complexity builds up

Copy link
Member Author

Choose a reason for hiding this comment

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

Same as above.

@pratikunterwegs
Copy link
Member Author

Sorry Pratik, I had pending comments but hadn't actually submitted them

Thanks! I remembered you'd asked me questions about some parts of this PR so I knew there must be comments somewhere!

Copy link

This pull request:

  • Adds 4 new dependencies (direct and indirect)
  • Adds 1 new system dependencies
  • Removes 1 existing dependencies (direct and indirect)
  • Removes 1 existing system dependencies

(Note that results may be inacurrate if you branched from an outdated version of the target branch.)

Co-authored-by: CarmenTamayo <lshct11@lshtm.ac.uk>
Copy link

This pull request:

  • Adds 4 new dependencies (direct and indirect)
  • Adds 1 new system dependencies
  • Removes 1 existing dependencies (direct and indirect)
  • Removes 1 existing system dependencies

(Note that results may be inacurrate if you branched from an outdated version of the target branch.)

@pratikunterwegs pratikunterwegs changed the title New API for ODE C++ models Vector inputs for ODE C++ models Mar 13, 2024
@pratikunterwegs pratikunterwegs changed the title Vector inputs for ODE C++ models Vector inputs for ODE models Mar 13, 2024
@pratikunterwegs
Copy link
Member Author

Thanks all for your reviews - merging this now so we can make smaller PRs in future for related issues.

@pratikunterwegs pratikunterwegs merged commit e301303 into main Mar 14, 2024
11 checks passed
@pratikunterwegs pratikunterwegs deleted the feature/ode-model-api branch March 14, 2024 10:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment