-
Notifications
You must be signed in to change notification settings - Fork 2
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
Conversation
e3bcefe
to
da425a1
Compare
5c50d69
to
7b6cbb1
Compare
This pull request:
(Note that results may be inacurrate if you branched from an outdated version of the target branch.) |
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 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?
This pull request:
(Note that results may be inacurrate if you branched from an outdated version of the target branch.) |
1 similar comment
This pull request:
(Note that results may be inacurrate if you branched from an outdated version of the target branch.) |
This pull request:
(Note that results may be inacurrate if you branched from an outdated version of the target branch.) |
# 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( |
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.
Took me a minute to figure out what the name means. Could you maybe write it in full, i.e., is_list_of_interventions
?
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.
Leaving the name for brevity, but have made the comments more explanatory in 51cccaf
c2e02ea
to
8c6e13f
Compare
This pull request:
(Note that results may be inacurrate if you branched from an outdated version of the target branch.) |
This pull request:
(Note that results may be inacurrate if you branched from an outdated version of the target branch.) |
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 |
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.
Sorry Pratik, I had pending comments but hadn't actually submitted them
::: | ||
|
||
::: {.alert .alert-info} | ||
### Combinations of intervention and vaccination scenarios |
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.
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
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.
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?
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.
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. |
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.
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
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.
Same as above.
Thanks! I remembered you'd asked me questions about some parts of this PR so I knew there must be comments somewhere! |
This pull request:
(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>
This pull request:
(Note that results may be inacurrate if you branched from an outdated version of the target branch.) |
Thanks all for your reviews - merging this now so we can make smaller PRs in future for related issues. |
This is a substantial user-facing and internal update to {epidemics}. Please treat any review as a full-package review.
Context
Changes in this PR
All models have only a single exported version, named
model_*()
; this is WIP Reconsider R implementations of ODE models #162.model_*_cpp()
All ODE models accept infection parameters (and
time_end
) as numeric vectors; this fixes ODE model functions should accept vectors of infection parameters #166<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 #177All ODE models accept lists of intervention sets, and lists of
<vaccination>
as inputs tointervention
andvaccination
respectively; this fixes ODE model functions should accept list of model component sets #167<intervention>
s; the new input supports lists of lists of<intervention>
spopulation
,time_dependence
, orpopulation_change
--- it is an open question as to whether the latter two are necessary, but the need for multiple populations is noted in Allowpopulation
to be a list-like in epidemic models #181 but not tackled here;.cross_check_*()
which are used in model-specific argument checker/preparation functions; this fixes Refactor argument checker functions to cross-checking only #175An 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:
susc_reduction_vax
) is now transmissibility for vaccinated individuals (transmissibility_vax
);hosp_reduction_vax
) is now hospitalisation for vaccinated individuals (hospitalisation_rate_vax
);mort_reduction_vax
) is now hospitalisation for vaccinated individuals (mortality_rate_vax
);<rate_intervention>
sAdds @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