Add self-isolation to simulation model#213
Add self-isolation to simulation model#213joshwlambert wants to merge 11 commits intoepiforecasts:mainfrom
Conversation
…ampling logic, relates epiforecasts#212
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis pull request adds a self-isolation feature to the ringbp model by introducing new delay and event probability options ( Changes
Possibly related issues
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
R/outbreak_model.R (1)
59-59: Consider the redundant validation when called fromscenario_sim.When
outbreak_modelis invoked viascenario_sim, thecross_check_optsvalidation runs once inscenario_sim(line 66) and then again here on every iteration of thereplicateloop. Fornsimulations, this results inn + 1validations instead of just one.This is acceptable if you want
outbreak_modelto be safe for standalone usage, but if performance becomes a concern with largen, you could consider an internal flag or a separate unchecked entry point.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/outbreak_model.R` at line 59, The cross_check_opts validation in outbreak_model is being redundantly called when scenario_sim already validated inputs; add a way to skip repeated checks by introducing a validation flag (e.g., validate = TRUE) on outbreak_model (or create an internal helper outbreak_model_internal used by scenario_sim) and use that flag inside outbreak_model to bypass cross_check_opts(delays, event_probs) when validate is FALSE; update scenario_sim to call outbreak_model with validate = FALSE (or call the internal unchecked helper) so validation runs only once while keeping outbreak_model safe for standalone use.tests/testthat/test-outbreak_setup.R (1)
49-50: Consider adding an explicit assertion forself_isolatevalues.The test verifies the column type but doesn't explicitly check that initial cases have
self_isolate = FALSE. For consistency with other assertions (e.g., line 57 checksunique(res$sampled)), consider adding:expect_identical(unique(res$self_isolate), FALSE)This would explicitly verify the documented behaviour that initial cases do not self-isolate.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/testthat/test-outbreak_setup.R` around lines 49 - 50, Add an explicit assertion that all initial cases have self_isolate == FALSE by adding a statement like expect_identical(unique(res$self_isolate), FALSE) near the other column checks in the test (the variable under test is res and the column is self_isolate); this complements the existing type check for self_isolate and ensures the documented behavior that initial cases do not self-isolate is enforced.R/outbreak_step.R (1)
127-131: Initialiseself_isolatefor all rows to avoid NA-state logic.Right now, only symptomatic rows receive values, so others carry
NA. Initialising toFALSEmakes this field consistently boolean and reduces NA-sensitive downstream behaviour.♻️ Proposed refactor
test_positive = FALSE, # assign no isolation as placeholder; symptomatic, test-positive, # traced or quarantined isolation time sampled below - isolated_time = Inf + isolated_time = Inf, + self_isolate = FALSE )][, # draws a sample to see if this person is asymptomatic asymptomatic := runif(.N) < event_probs$asymptomatic ][, # onset of new case is exposure + incubation period sample onset := exposure + delays$incubation_period(.N) ][ # draw a sample to see if each person self-isolates if symptomatic asymptomatic == FALSE, self_isolate := runif(.N) < event_probs$symptomatic_self_isolate ]Also applies to: 138-141
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@R/outbreak_step.R` around lines 127 - 131, The column self_isolate is only set for symptomatic rows causing NAs elsewhere; initialize self_isolate = FALSE for all rows in the same data.table creation/assignment where test_positive and isolated_time are set (look for the block setting test_positive = FALSE and isolated_time = Inf in outbreak_step.R) so the field is always boolean; update the initial assignment to include self_isolate = FALSE (and mirror the same change in the corresponding block at lines referenced 138-141) rather than only assigning it for symptomatic rows.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@man/delay_opts.Rd`:
- Around line 45-48: Update the documentation sentence to indicate that the
cross-check for incompatible self-isolation settings applies to both entry
points: change the phrase that currently says only "scenario_sim()" will error
to mention that both scenario_sim() and outbreak_model() perform the same
validation; explicitly state that if onset_to_self_isolation is NULL while
symptomatic_self_isolate is non-zero, both scenario_sim() and outbreak_model()
will error, and that if onset_to_self_isolation is provided but
symptomatic_self_isolate is zero a warning is thrown and onset_to_self_isolation
is ignored. Include the parameter names onset_to_self_isolation and
symptomatic_self_isolate in the updated text for clarity.
In `@R/opts.R`:
- Around line 81-87: Update the documentation text for onset_to_self_isolation
to state that the cross-check (if onset_to_self_isolation is specified but
symptomatic_self_isolate is zero a warning is thrown; if
symptomatic_self_isolate is non-zero and onset_to_self_isolation is NULL an
error is raised) is enforced by both scenario_sim() and outbreak_model(), not
just scenario_sim(); modify the paragraph referencing
onset_to_self_isolation/symptomatic_self_isolate in opts.R (and the similar
wording at the other occurrence around lines 149–151) to call out both entry
points and keep the existing conditional behavior and function names
(onset_to_self_isolation, symptomatic_self_isolate, event_prob_opts(),
scenario_sim(), outbreak_model()) so users know the validation happens at both
API entrypoints.
In `@R/outbreak_step.R`:
- Around line 155-157: The call to delays$onset_to_self_isolation(.N) inside
outbreak_step() assumes the sampler option is enabled; add an explicit guard
that checks the relevant option/flag (e.g., self_isolate or a config flag used
by delays) before invoking delays$onset_to_self_isolation to avoid runtime
errors when outbreak_step() is called directly. Locate the block using
self_isolate and onset + delays$onset_to_self_isolation(.N) and: verify the
sampler exists/is enabled on the delays object (or check the global/config
option that controls self-isolation sampling) and only call
delays$onset_to_self_isolation(.N) when that check passes; otherwise set
isolated_time to NA or a safe default and/or log/raise a clear error. Ensure
references to self_isolate, delays, onset_to_self_isolation, and outbreak_step
are used so the new guard targets the exact code paths.
---
Nitpick comments:
In `@R/outbreak_model.R`:
- Line 59: The cross_check_opts validation in outbreak_model is being
redundantly called when scenario_sim already validated inputs; add a way to skip
repeated checks by introducing a validation flag (e.g., validate = TRUE) on
outbreak_model (or create an internal helper outbreak_model_internal used by
scenario_sim) and use that flag inside outbreak_model to bypass
cross_check_opts(delays, event_probs) when validate is FALSE; update
scenario_sim to call outbreak_model with validate = FALSE (or call the internal
unchecked helper) so validation runs only once while keeping outbreak_model safe
for standalone use.
In `@R/outbreak_step.R`:
- Around line 127-131: The column self_isolate is only set for symptomatic rows
causing NAs elsewhere; initialize self_isolate = FALSE for all rows in the same
data.table creation/assignment where test_positive and isolated_time are set
(look for the block setting test_positive = FALSE and isolated_time = Inf in
outbreak_step.R) so the field is always boolean; update the initial assignment
to include self_isolate = FALSE (and mirror the same change in the corresponding
block at lines referenced 138-141) rather than only assigning it for symptomatic
rows.
In `@tests/testthat/test-outbreak_setup.R`:
- Around line 49-50: Add an explicit assertion that all initial cases have
self_isolate == FALSE by adding a statement like
expect_identical(unique(res$self_isolate), FALSE) near the other column checks
in the test (the variable under test is res and the column is self_isolate);
this complements the existing type check for self_isolate and ensures the
documented behavior that initial cases do not self-isolate is enforced.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 56dce6d3-e348-4d29-b066-4a1a5289bf5e
📒 Files selected for processing (19)
R/checking.RR/globals.RR/opts.RR/outbreak_model.RR/outbreak_setup.RR/outbreak_step.RR/scenario_sim.Rman/cross_check_opts.Rdman/delay_opts.Rdman/event_prob_opts.Rdman/outbreak_model.Rdman/outbreak_setup.Rdman/outbreak_step.Rdman/scenario_sim.Rdtests/testthat/_snaps/outbreak_setup.mdtests/testthat/_snaps/scenario_sim.mdtests/testthat/test-checking.Rtests/testthat/test-opts.Rtests/testthat/test-outbreak_setup.R
| If \code{onset_to_self_isolation} is specified but \code{symptomatic_self_isolate} | ||
| is zero, a warning will be thrown and the \code{onset_to_self_isolation} | ||
| will be ignored; if \code{symptomatic_self_isolate} is non-zero and | ||
| \code{onset_to_self_isolation} is \code{NULL}, \code{\link[=scenario_sim]{scenario_sim()}} will error.} |
There was a problem hiding this comment.
Docs understate where incompatible self-isolation configs fail.
Line 48 mentions only scenario_sim(), but the same cross-check is also run for outbreak_model(). Please broaden this wording so users of both entry points get accurate guidance.
📝 Suggested documentation update
- \code{onset_to_self_isolation} is \code{NULL}, \code{\link[=scenario_sim]{scenario_sim()}} will error.}
+ \code{onset_to_self_isolation} is \code{NULL}, model runs that apply option
+ cross-checking (e.g. \code{\link[=scenario_sim]{scenario_sim()}} and
+ \code{\link[=outbreak_model]{outbreak_model()}}) will error.}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| If \code{onset_to_self_isolation} is specified but \code{symptomatic_self_isolate} | |
| is zero, a warning will be thrown and the \code{onset_to_self_isolation} | |
| will be ignored; if \code{symptomatic_self_isolate} is non-zero and | |
| \code{onset_to_self_isolation} is \code{NULL}, \code{\link[=scenario_sim]{scenario_sim()}} will error.} | |
| If \code{onset_to_self_isolation} is specified but \code{symptomatic_self_isolate} | |
| is zero, a warning will be thrown and the \code{onset_to_self_isolation} | |
| will be ignored; if \code{symptomatic_self_isolate} is non-zero and | |
| \code{onset_to_self_isolation} is \code{NULL}, model runs that apply option | |
| cross-checking (e.g. \code{\link[=scenario_sim]{scenario_sim()}} and | |
| \code{\link[=outbreak_model]{outbreak_model()}}) will error.} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@man/delay_opts.Rd` around lines 45 - 48, Update the documentation sentence to
indicate that the cross-check for incompatible self-isolation settings applies
to both entry points: change the phrase that currently says only
"scenario_sim()" will error to mention that both scenario_sim() and
outbreak_model() perform the same validation; explicitly state that if
onset_to_self_isolation is NULL while symptomatic_self_isolate is non-zero, both
scenario_sim() and outbreak_model() will error, and that if
onset_to_self_isolation is provided but symptomatic_self_isolate is zero a
warning is thrown and onset_to_self_isolation is ignored. Include the parameter
names onset_to_self_isolation and symptomatic_self_isolate in the updated text
for clarity.
There was a problem hiding this comment.
Fair point but scenario_sim() is seen as the principle user-facing function so I'll leave as is for now. outbreak_model() may be moved from exported to internal in a future version of {ringbp}.
There was a problem hiding this comment.
@joshwlambert, understood — that's a sensible rationale. Keeping the docs focused on scenario_sim() as the primary entry point makes sense, and if outbreak_model() is eventually internalized the doc would need no further update. Thanks for the context!
(´• ω •`) ~♪
✏️ Learnings added
Learnt from: joshwlambert
Repo: epiforecasts/ringbp PR: 213
File: man/delay_opts.Rd:45-48
Timestamp: 2026-03-04T14:13:08.671Z
Learning: In the {ringbp} R package (repository epiforecasts/ringbp), `scenario_sim()` is the principal user-facing entry point. `outbreak_model()` is currently exported but may be moved to internal in a future version. Documentation for user-facing functions should primarily reference `scenario_sim()` rather than `outbreak_model()`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| #' By default `onset_to_self_isolation` is `NULL`. An onset-to-self-isolation | ||
| #' `function` only needs to be specified if a non-zero value is specified to | ||
| #' `symptomatic_self_isolate` in [event_prob_opts()] (which by default is 0). | ||
| #' If `onset_to_self_isolation` is specified but `symptomatic_self_isolate` | ||
| #' is zero, a warning will be thrown and the `onset_to_self_isolation` | ||
| #' will be ignored; if `symptomatic_self_isolate` is non-zero and | ||
| #' `onset_to_self_isolation` is `NULL`, [scenario_sim()] will error. |
There was a problem hiding this comment.
Clarify all call paths that enforce the cross-check.
The docs currently mention only scenario_sim(), but this validation is also enforced via outbreak_model(). Please document both entrypoints to avoid ambiguity for API users.
✏️ Proposed doc fix
-#' will be ignored; if `symptomatic_self_isolate` is non-zero and
-#' `onset_to_self_isolation` is `NULL`, [scenario_sim()] will error.
+#' will be ignored; if `symptomatic_self_isolate` is non-zero and
+#' `onset_to_self_isolation` is `NULL`, [scenario_sim()] and
+#' [outbreak_model()] will error.-#' in the [delay_opts()] function, otherwise the [scenario_sim()] will error.
+#' in the [delay_opts()] function, otherwise [scenario_sim()] and
+#' [outbreak_model()] will error.Also applies to: 149-151
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@R/opts.R` around lines 81 - 87, Update the documentation text for
onset_to_self_isolation to state that the cross-check (if
onset_to_self_isolation is specified but symptomatic_self_isolate is zero a
warning is thrown; if symptomatic_self_isolate is non-zero and
onset_to_self_isolation is NULL an error is raised) is enforced by both
scenario_sim() and outbreak_model(), not just scenario_sim(); modify the
paragraph referencing onset_to_self_isolation/symptomatic_self_isolate in opts.R
(and the similar wording at the other occurrence around lines 149–151) to call
out both entry points and keep the existing conditional behavior and function
names (onset_to_self_isolation, symptomatic_self_isolate, event_prob_opts(),
scenario_sim(), outbreak_model()) so users know the validation happens at both
API entrypoints.
There was a problem hiding this comment.
This is the same suggestion as the comment above, just in a different document. See above for reasoning on not accepting the recommended change.
There was a problem hiding this comment.
@joshwlambert, understood — I've noted the reasoning from the earlier comment (that outbreak_model() may be moved to internal in a future version, so documentation should favour scenario_sim() as the principal user-facing entry point). Happy to leave the wording as-is.
🧠 Learnings used
Learnt from: joshwlambert
Repo: epiforecasts/ringbp PR: 213
File: man/delay_opts.Rd:45-48
Timestamp: 2026-03-04T14:13:08.671Z
Learning: In the {ringbp} R package (repository epiforecasts/ringbp), `scenario_sim()` is the principal user-facing entry point. `outbreak_model()` is currently exported but may be moved to internal in a future version. Documentation for user-facing functions should primarily reference `scenario_sim()` rather than `outbreak_model()`.
| self_isolate == TRUE, | ||
| isolated_time := onset + delays$onset_to_self_isolation(.N) | ||
| ] |
There was a problem hiding this comment.
Guard the self-isolation sampler with an explicit option cross-check.
On Line 156, delays$onset_to_self_isolation(.N) assumes option compatibility is already enforced. Because outbreak_step() is public, direct calls can hit this path without pre-validation and fail with a non-actionable runtime error.
🛡️ Proposed fix
outbreak_step <- function(case_data,
offspring,
delays,
event_probs,
interventions) {
checkmate::assert_data_table(case_data)
checkmate::assert_class(offspring, "ringbp_offspring_opts")
checkmate::assert_class(delays, "ringbp_delay_opts")
checkmate::assert_class(event_probs, "ringbp_event_prob_opts")
checkmate::assert_class(interventions, "ringbp_intervention_opts")
+ cross_check_opts(delays = delays, event_probs = event_probs)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@R/outbreak_step.R` around lines 155 - 157, The call to
delays$onset_to_self_isolation(.N) inside outbreak_step() assumes the sampler
option is enabled; add an explicit guard that checks the relevant option/flag
(e.g., self_isolate or a config flag used by delays) before invoking
delays$onset_to_self_isolation to avoid runtime errors when outbreak_step() is
called directly. Locate the block using self_isolate and onset +
delays$onset_to_self_isolation(.N) and: verify the sampler exists/is enabled on
the delays object (or check the global/config option that controls
self-isolation sampling) and only call delays$onset_to_self_isolation(.N) when
that check passes; otherwise set isolated_time to NA or a safe default and/or
log/raise a clear error. Ensure references to self_isolate, delays,
onset_to_self_isolation, and outbreak_step are used so the new guard targets the
exact code paths.
There was a problem hiding this comment.
Fair point. I'm not going to add cross_check_opts() to outbreak_step() as this will have performance impact.
Tagging @pearsonca as we've discussed potentially moving some of the exported functions (incl. outbreak_step()) to internal and this is another contributing factor to that debate.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
the generally-easiest approach here is to rip outbreak_step into a public-with-validation outbreak_step and an internal no-validation internal_outbreak_step (or similar naming). i imagine that also means moving some of the validation into the overall public-facing simulation runner previously handled by outbreak_step.
that seem straightforward enough, if now is the time? but maybe approach as a focus PR on that, then rebase this changeset onto that.
There was a problem hiding this comment.
I would leave this for a subsequent PR specifically working on the package namespace.
From my side, the primary (and only?) reason for leaving the functions that scenario_sim() calls (outbreak_model(), outbreak_setup(), outbreak_step()) exported is to better enable spin-off projects, as was done in COVID-19. But perhaps the exported/internal status does not influence the ability of projects to extend this model, and it's more to do with the model architecture, I'm genuinely not sure.
It also relates #118.
There was a problem hiding this comment.
I'd lean towards more to do with the model structure, so agree for another PR whichever way end up pursuing.
an even easier version would be to update the documents for these exported-but-internal-ish functions to make clear that they don't do validation. caveat utor, etc.
2 participants
This PR addresses #212 by allowing the user to specify a proportion of people that self-isolate when symptomatic. These individuals do not get tested and instead isolate themselves a delay after symptom onset. The onset-to-self-isolation delay is separate from the existing onset-to-isolation delay (
onset_to_isolation) in the model, allowing the user to specify a different (likely shorter) delay to isolation.This feature has added a
symptomatic_self_isolateargument toevent_prob_opts(), which defaults to 0 for backwards compatibility; and anonset_to_self_isolationargument todelay_opts()to specify the onset-to-self-isolation delay. These two arguments interact but are specified in different*_opts()functions so require cross-checking in the simulation code.onset_to_self_isolationdefaults toNULLbecause by default no one self-isolates (and is backwards compatible).The
cross_check_opts()function has been added in this PR, and is called inscenario_sim()andoutbreak_model(). It will warn or error ifsymptomatic_self_isolateandonset_to_self_isolationare incompatible. Unit tests have been added forcross_check_opts().Initial cases in
outbreak_setup()are assumed not to self-isolate. This follows the current assumption that initial cases are missed by contact tracing.outbreak_step()now assigns a placeholder value forisolated_timewhen creating the new cases, and there is now a two-step sampling of isolation times:The
fcase()for the second isolation time sampling removes the conditions for asymptomatic or false-negative tested individuals because these areInfwhich is equal to the placeholder value.Unit test expectations for
outbreak_setup()and*_opts()function have been updated, as have the snapshots.Not related to the feature but updated in this PR: argument documentation for
delays,event_probsandinterventionshas been updated to the correct number of output elements.Summary by CodeRabbit
Release Notes
New Features
Improvements