Prospective refactor #77
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…order to match func signature
…um_initial_cases and prop_asym
…ument default in scenario_sim, outbreak_model and outbreak_step
… set argument default in scenario_sim, outbreak_model and outbreak_step
…eplace hardcoded incubation period with incubation_period arg
…incubation_period args
sbfnk
reviewed
Oct 7, 2024
There was a problem hiding this comment.
Thanks @joshwlambert I this these are sensible suggestions - left some minor comments on style/linting.
Comment on lines
+42
to
+43
| #' @param outbreak_df_week data.table weekly cases produced by the outbreak | ||
| #' model |
There was a problem hiding this comment.
Suggested change
| #' @param outbreak_df_week data.table weekly cases produced by the outbreak | |
| #' model | |
| #' @param outbreak_df_week data.table weekly cases produced by the outbreak | |
| #' model |
Should have 2 indents throughout here for readability as in e.g. https://github.com/r-lib/roxygen2/blob/9652d15221109917d46768e836eaf55e33c21633/R/roxygenize.R#L14
| #' disp_com = 0.16, | ||
| #' disp_subclin = 0.16, | ||
| #' k = 0, | ||
| #' onset_to_isolation = function(x) rweibull(n = x, shape = 1.651524, scale = 4.287786), |
There was a problem hiding this comment.
Suggested change
| #' onset_to_isolation = function(x) rweibull(n = x, shape = 1.651524, scale = 4.287786), | |
| #' onset_to_isolation = \(x) rweibull(n = x, shape = 1.651524, scale = 4.287786), |
we already have this notation elsewhere
Comment on lines
+66
to
+80
| outbreak_model <- function(num_initial_cases, | ||
| prop_ascertain, | ||
| cap_max_days, | ||
| cap_cases, | ||
| r0isolated, | ||
| r0community, | ||
| r0subclin = r0community, | ||
| disp_iso, | ||
| disp_com, | ||
| disp_subclin = disp_com, | ||
| k, | ||
| onset_to_isolation, | ||
| incubation_period, | ||
| prop_asym, | ||
| quarantine) { |
There was a problem hiding this comment.
Suggested change
| outbreak_model <- function(num_initial_cases, | |
| prop_ascertain, | |
| cap_max_days, | |
| cap_cases, | |
| r0isolated, | |
| r0community, | |
| r0subclin = r0community, | |
| disp_iso, | |
| disp_com, | |
| disp_subclin = disp_com, | |
| k, | |
| onset_to_isolation, | |
| incubation_period, | |
| prop_asym, | |
| quarantine) { | |
| outbreak_model <- function( | |
| num_initial_cases, | |
| prop_ascertain, | |
| cap_max_days, | |
| cap_cases, | |
| r0isolated, | |
| r0community, | |
| r0subclin = r0community, | |
| disp_iso, | |
| disp_com, | |
| disp_subclin = disp_com, | |
| k, | |
| onset_to_isolation, | |
| incubation_period, | |
| prop_asym, | |
| quarantine | |
| ) { |
to be consistent with the style in other parts of the package
|
Thanks for the suggestions @sbfnk. Are you happy for me to work through the {ringbp} issues and make some of the changes included in this PR along the way? |
|
Yes, that would be great! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request makes a series of changes to the {ringbp} to hopefully improve the package.
This PR is not for merging. It is purely to demonstrate the types of changes that could be made in {ringbp}.
If this PR contains changes that we want to include in {ringbp} then I can make specific feature branches and tag the relevant issues. In this PR I have not related the changes to any existing issues in the {ringbp} GitHub repository as this was a more general exploration of how the package can be improved.
This is not a full refactor, but a start of to give a flavour of the improvements, without sinking too much time if we decide this is not the best direction of development for the package. Some changes include:
dist_spec()function