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

Remove infection class #140

Merged
merged 29 commits into from
Nov 21, 2023
Merged

Remove infection class #140

merged 29 commits into from
Nov 21, 2023

Conversation

pratikunterwegs
Copy link
Member

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 Add examples to model functions #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 PDF documentation rendering fails on r-universe #139,
  4. Pulls prob_discrete_erlang() internal.

@pratikunterwegs
Copy link
Member Author

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
Copy link
Member Author

Hi @bahadzie, just an update on this PR - I've hard reset this branch to bb92bc5, 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
Copy link
Member Author

pratikunterwegs commented Nov 21, 2023

I'm forcing the branch to bb92bc5 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants