Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 35 additions & 0 deletions R/checking.R
Original file line number Diff line number Diff line change
Expand Up @@ -48,3 +48,38 @@ check_dist_func <- function(func,

TRUE
}

#' Cross-check `*_opts()` lists to run the \pkg{ringbp} model
#'
#' @details Currently the only cross-checking is between
#' `onset_to_self_isolation` from [delay_opts()] and
#' `symptomatic_self_isolate` from [event_prob_opts()].
#'
#' @inheritParams outbreak_step
#'
#' @return `TRUE` if all the checks pass or an error or warning is thrown if
#' the simulation options are incompatible.
#' @keywords internal
cross_check_opts <- function(delays, event_probs) {
if (is.null(delays$onset_to_self_isolation) &&
event_probs$symptomatic_self_isolate > 0) {
stop(
"A non-zero `symptomatic_self_isolate` has been specified in ",
"`event_prob_opts()`, but `onset_to_self_isolation` is `NULL`.\n",
"Please specify an `onset_to_self_isolation` function for a proportion ",
"of symptomatic infections to self-isolate.",
call. = FALSE)
}
if (is.function(delays$onset_to_self_isolation) &&
event_probs$symptomatic_self_isolate == 0) {
warning(
"An `onset_to_self_isolation` delay has been specified in ",
"`delay_opts()`, but the `symptomatic_self_isolate` in ",
"`event_prob_opts()` is zero.\nIgnoring `onset_to_self_isolation`.\n",
"Please specify a non-zero value to `symptomatic_self_isolate` for a ",
"proportion of symptomatic infections to self-isolate.",
call. = FALSE
)
}
TRUE
}
1 change: 1 addition & 0 deletions R/globals.R
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ utils::globalVariables(c(
"asymptomatic", # <outbreak_step>
"onset", # <outbreak_step>
"exposure", # <outbreak_step>
"self_isolate", # <outbreak_step>
"infector_asymptomatic", # <outbreak_step>
"traced", # <outbreak_step>
"test_positive", # <outbreak_step>
Expand Down
41 changes: 37 additions & 4 deletions R/opts.R
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,19 @@ offspring_opts <- function(community, isolated, asymptomatic = community) {
#' presymptomatic transmission, depending on the `incubation_period`
#' distribution and `presymptomatic_transmission` (in [event_prob_opts()]).
#'
#' @param onset_to_self_isolation a `function`: a random number generating
#' `function` that samples from the onset-to-self-isolation distribution,
#' the `function` accepts a single `integer` argument specifying the length
#' of the `function` output.
#'
#' 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.
Comment on lines +81 to +87
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the same suggestion as the comment above, just in a different document. See above for reasoning on not accepting the recommended change.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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()`.

#'
#' @return A `list` with class `<ringbp_delay_opts>`.
#' @export
#'
Expand All @@ -83,11 +96,18 @@ offspring_opts <- function(community, isolated, asymptomatic = community) {
#' )
delay_opts <- function(incubation_period,
onset_to_isolation,
latent_period = 0) {
latent_period = 0,
onset_to_self_isolation = NULL) {

check_dist_func(incubation_period, dist_name = "incubation_period")
check_dist_func(onset_to_isolation, dist_name = "onset_to_isolation")
checkmate::assert_number(latent_period, lower = 0, finite = TRUE)
if (!is.null(onset_to_self_isolation)) {
check_dist_func(
onset_to_self_isolation,
dist_name = "onset_to_self_isolation"
)
}

if (latent_period > 0) {
warning(
Expand All @@ -103,7 +123,8 @@ delay_opts <- function(incubation_period,
opts <- list(
incubation_period = incubation_period,
onset_to_isolation = onset_to_isolation,
latent_period = latent_period
latent_period = latent_period,
onset_to_self_isolation = onset_to_self_isolation
)

class(opts) <- "ringbp_delay_opts"
Expand All @@ -119,6 +140,15 @@ delay_opts <- function(incubation_period,
#' @param symptomatic_traced a `numeric` scalar probability (between 0
#' and 1 inclusive): proportion of infectious contacts ascertained by contact
#' tracing
#' @param symptomatic_self_isolate a `numeric` scalar probability (between 0
#' and 1 inclusive): proportion of cases that self-isolate when they become
#' symptomatic. These individuals do not get tested and do not require a
#' positive test result to enter isolation. Default is 0 (i.e. no infectious
#' individuals self-isolate).
#'
#' If `symptomatic_self_isolate` is non-zero a random number generating
#' `function` needs to be specified in the `onset_to_self_isolation` argument
#' in the [delay_opts()] function, otherwise the [scenario_sim()] will error.
#'
#' @return A `list` with class `<ringbp_event_prob_opts>`.
#' @export
Expand All @@ -131,11 +161,13 @@ delay_opts <- function(incubation_period,
#' )
event_prob_opts <- function(asymptomatic,
presymptomatic_transmission,
symptomatic_traced) {
symptomatic_traced,
symptomatic_self_isolate = 0) {

checkmate::assert_number(asymptomatic, lower = 0, upper = 1)
checkmate::assert_number(presymptomatic_transmission, lower = 0, upper = 1)
checkmate::assert_number(symptomatic_traced, lower = 0, upper = 1)
checkmate::assert_number(symptomatic_self_isolate, lower = 0, upper = 1)

# calculate alpha parameter from presymptomatic_transmission
alpha <- presymptomatic_transmission_to_alpha(
Expand All @@ -146,7 +178,8 @@ event_prob_opts <- function(asymptomatic,
asymptomatic = asymptomatic,
presymptomatic_transmission = presymptomatic_transmission,
alpha = alpha,
symptomatic_traced = symptomatic_traced
symptomatic_traced = symptomatic_traced,
symptomatic_self_isolate = symptomatic_self_isolate
)

class(opts) <- "ringbp_event_prob_opts"
Expand Down
1 change: 1 addition & 0 deletions R/outbreak_model.R
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ outbreak_model <- function(initial_cases,
checkmate::assert_class(event_probs, "ringbp_event_prob_opts")
checkmate::assert_class(interventions, "ringbp_intervention_opts")
checkmate::assert_class(sim, "ringbp_sim_opts")
cross_check_opts(delays, event_probs)

# Initial setup
case_data <- outbreak_setup(
Expand Down
2 changes: 2 additions & 0 deletions R/outbreak_setup.R
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#' * `$traced`: `logical`
#' * `$onset`: `numeric`
#' * `$new_cases`: `integer`
#' * `$self_isolate`: `logical`
#' * `$isolated_time`: `numeric`
#' * `$sampled`: `logical`
#' @autoglobal
Expand Down Expand Up @@ -53,6 +54,7 @@ outbreak_setup <- function(initial_cases, delays, event_probs) {
traced = FALSE,
onset = delays$incubation_period(initial_cases),
new_cases = NA_integer_,
self_isolate = FALSE,
isolated_time = Inf,
sampled = FALSE
)
Expand Down
65 changes: 41 additions & 24 deletions R/outbreak_step.R
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,17 @@
#' and `asymptomatic`
#' @param delays a `list` with class `<ringbp_delay_opts>`: the
#' delay distribution `function`s for the \pkg{ringbp} model, returned
#' by [delay_opts()]. Contains two elements: `incubation_period` and
#' `onset_to_isolation`
#' by [delay_opts()]. Contains 4 elements: `incubation_period`,
#' `onset_to_isolation`, `latent_period` and `onset_to_self_isolation`
#' @param event_probs a `list` with class `<ringbp_event_prob_opts>`: the
#' event probabilities for the \pkg{ringbp} model, returned by
#' [event_prob_opts()]. Contains three elements: `asymptomatic`,
#' `presymptomatic_transmission` and `symptomatic_traced`
#' [event_prob_opts()]. Contains 5 elements: `asymptomatic`,
#' `presymptomatic_transmission`, `alpha`, `symptomatic_traced` and
#' `symptomatic_self_isolate`
#' @param interventions a `list` with class `<ringbp_intervention_opts>`:
#' the intervention settings for the \pkg{ringbp} model, returned by
#' [intervention_opts()]. Contains one element: `quarantine`
#' [intervention_opts()]. Contains 2 elements: `quarantine` and
#' `test_sensitivity`
#'
#' @importFrom data.table data.table rbindlist fcase fifelse copy
#' @importFrom stats runif rnbinom rbinom
Expand Down Expand Up @@ -122,41 +124,56 @@ outbreak_step <- function(case_data,
sampled = FALSE,
# assign negative test result (FALSE) as placeholder;
# will draw test result for symptomatic cases below
test_positive = FALSE
test_positive = FALSE,
# assign no isolation as placeholder; symptomatic, test-positive,
# traced or quarantined isolation time sampled below
isolated_time = Inf
)][,
# 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
]

# draw a sample for missing and test result
prob_samples[
infector_asymptomatic == FALSE,
traced := runif(.N) < event_probs$symptomatic_traced
][
asymptomatic == FALSE,
asymptomatic == FALSE & self_isolate == FALSE,
test_positive := runif(.N) <= interventions$test_sensitivity
]

prob_samples[, isolated_time := {
ref_time <- onset + delays$onset_to_isolation(.N)
fcase(
# asymptomatic: never isolated
asymptomatic, Inf,
# symptomatic, false-negative test: never isolated
!test_positive, Inf,
# symptomatic, test-positive, not traced: isolated at symptom onset + delay
!traced, ref_time,
# symptomatic, test-positive, traced, quarantine:
# earliest of infector / infectee isolation time
rep(interventions$quarantine, .N), pmin(ref_time, infector_isolation_time),
# symptomatic, test-positive, traced, no quarantine:
# earliest of symptom onset + delay / infector isolation time
default = pmin(ref_time, pmax(onset, infector_isolation_time))
)
}]
# symptomatic, self-isolate, not tested:
# isolated at symptom onset + self-isolation delay
prob_samples[
self_isolate == TRUE,
isolated_time := onset + delays$onset_to_self_isolation(.N)
]
Comment on lines +155 to +157
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


# asymptomatic and symptomatic with false-negative test:
# never isolated remain Inf
prob_samples[
!self_isolate & !asymptomatic & test_positive,
isolated_time := {
ref_time <- onset + delays$onset_to_isolation(.N)
fcase(
# symptomatic, test-positive, not traced: isolated at symptom onset + delay
!traced, ref_time,
# symptomatic, test-positive, traced, quarantine:
# earliest of infector / infectee isolation time
rep(interventions$quarantine, .N), pmin(ref_time, infector_isolation_time),
# symptomatic, test-positive, traced, no quarantine:
# earliest of symptom onset + delay / infector isolation time
default = pmin(ref_time, pmax(onset, infector_isolation_time))
)
}
]

# Chop out unneeded sample columns
prob_samples[
Expand Down
1 change: 1 addition & 0 deletions R/scenario_sim.R
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ scenario_sim <- function(n,
checkmate::assert_class(event_probs, "ringbp_event_prob_opts")
checkmate::assert_class(interventions, "ringbp_intervention_opts")
checkmate::assert_class(sim, "ringbp_sim_opts")
cross_check_opts(delays, event_probs)

# Run n number of model runs and put them all together in a big data.frame
res <- replicate(
Expand Down
33 changes: 33 additions & 0 deletions man/cross_check_opts.Rd

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

20 changes: 19 additions & 1 deletion man/delay_opts.Rd

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

17 changes: 16 additions & 1 deletion man/event_prob_opts.Rd

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 7 additions & 5 deletions man/outbreak_model.Rd

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading