search

epiverse-trace / epidemics

A library of published compartmental epidemic models, and classes to represent demographic structure, non-pharmaceutical interventions, and vaccination regimes, to compose epidemic scenarios.
https://epiverse-trace.github.io/epidemics/
Other
8 stars 2 forks source link

Remove infection class #140

Closed pratikunterwegs closed 9 months ago

pratikunterwegs commented 10 months ago

This PR removes the <infection> class, and moves infection related parameters to the models' function calls as arguments. All arguments have sensible default values rather than being missing. This fixes #133 and #114.

Additionally this PR:

  1. Adds basic examples to function documentation, fixing #104,
  2. Removes the C++ implementation of the ebola model as it has fallen substantially behind the R version and does not add a speed boost,
  3. Removes unicode characters in the documentation of the ebola model to fix #139,
  4. Pulls prob_discrete_erlang() internal.
pratikunterwegs commented 10 months ago

Hi @bahadzie please do not push directly to PRs opened by others. It's better to leave a reivew, and let the PR author make the changes. That allows the PR author to consider how best to resolve the review issues, to be aware of the changes made, and allows others to come in and comment as well.

pratikunterwegs commented 10 months ago

Hi @bahadzie, just an update on this PR - I've hard reset this branch to https://github.com/epiverse-trace/epidemics/commit/bb92bc562eb0a64053856e8abe32314db683f9a1, thanks for spotting that. cc-ing @Bisaloo as this could be a good time to add to the PR/code review/ section in the Blueprints.

I have moved your other changes to the branch dev/accessors if you would like to continue work on them, after rebasing on main. I broadly agree that we should use [[ accessors where possible internally, but ideally that would be a separate issue and PR that I'm happy to accept. I would suggest that some autogenerated code, such as knitr::opts_chunk$set() should be left as they are, and code that we expect users might want to run interactively, such as the vignettes, can also be left unchanged.

I would also suggest to not commit changes to the snapshot tests without being sure why the checks are failing. In this case, my best guess is that it was probably because you were running an older version of {testthat}.

pratikunterwegs commented 9 months ago

I'm forcing the branch to https://github.com/epiverse-trace/epidemics/commit/bb92bc562eb0a64053856e8abe32314db683f9a1 again as I did last week, to remove the extra commits relating to accessors. This PR will be merged, unfortunately without review, once checks pass.

The work on accessors can be found on the dev/accessors branch. Please note that some of those commits also make changes to the snapshot test files - please don't change those files before careful consideration and ideally, a review.