Skip to content

Improve weekly aggregation in outbreak_model()#197

Merged
sbfnk merged 4 commits intoepiforecasts:mainfrom
joshwlambert:outbreak_model_if_cond
Feb 11, 2026
Merged

Improve weekly aggregation in outbreak_model()#197
sbfnk merged 4 commits intoepiforecasts:mainfrom
joshwlambert:outbreak_model_if_cond

Conversation

@joshwlambert
Copy link
Copy Markdown
Collaborator

@joshwlambert joshwlambert commented Jan 21, 2026

This PR addresses #168 but goes beyond fixing the typo in the if statement and rewrites a section of outbreak_model() to more efficiently create the empty weeks in the output data.table.

The complete outbreak data, with missing weeks is now created using a non-equi join. The need for the original if statement is removed.

This update also changes the type of the columns, from double to integer, resulting in a smaller memory footprint for the outbreak data.

Test expectations are updated to reflect the change in type for the week, weekly_cases and cumulative columns returned from outbreak_model() (and therefore also scenario_sim()).

Summary by CodeRabbit

Release Notes

  • Chores
    • Updated internal data type handling for weekly case calculations to ensure consistent integer representation across output columns.
    • Refined weekly case computation logic for improved data processing consistency.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Jan 21, 2026

Walkthrough

This PR refactors the weekly_cases calculation in the outbreak model to pre-initialise a zero-filled container for all weeks and overlay actual outbreak data, whilst updating numeric columns to integer types throughout the codebase and tests.

Changes

Cohort / File(s) Summary
Global variable declaration
R/globals.R
Added "i.weekly_cases" to the global variables list recognised by utils::globalVariables() for the outbreak_model group.
Core logic refactoring
R/outbreak_model.R
Refactored weekly_cases construction to pre-initialise a zero-filled container for weeks 0 to max_week, then join actual outbreak counts from weekly_cases_outbreak. Removed manual missing_weeks calculation and rbindlist step.
Test snapshots and expectations
tests/testthat/_snaps/scenario_sim.md, tests/testthat/test-outbreak_model.R, tests/testthat/test-scenario_sim.R
Updated type annotations and test expectations from numeric to integer for sim, week, weekly_cases, and cumulative columns; replaced numeric literals with integer literals (1L, 0L, 25L, etc.) in test assertions.

Possibly related PRs

  • Improve package test suite #160: Updates tests and snapshots asserting weekly_cases structure and types, directly aligned with the main PR's changes to weekly_cases construction and global variable additions.

Suggested labels

testing

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarises the main change—improving weekly aggregation in outbreak_model() by replacing a faulty conditional with a non-equi join approach.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
R/outbreak_model.R (1)

10-12: Documentation does not reflect the new integer column types.

The @return documentation states that $week, $weekly_cases, and $cumulative are numeric, but the implementation now produces integer columns. Consider updating the documentation to match.

📝 Suggested documentation update
 #' `@return` `data.table` of cases by week, cumulative cases, and the effective
 #' reproduction number of the outbreak. `data.table` columns are:
-#' * `$week`: `numeric`
-#' * `$weekly_cases`: `numeric`
-#' * `$cumulative`: `numeric`
+#' * `$week`: `integer`
+#' * `$weekly_cases`: `integer`
+#' * `$cumulative`: `integer`
 #' * `$effective_r0`: `numeric`
 #' * `$cases_per_gen`: `list`
🧹 Nitpick comments (1)
R/outbreak_model.R (1)

108-116: Potential type inconsistency: floor() returns double, but week column is integer.

The floor(onset / 7) expression on line 108 returns a double, whilst the week column in weekly_cases (line 114) is created as integer via 0:max_week. Although the data.table join will still function correctly, this creates a subtle type mismatch between weekly_cases_outbreak$week (double) and weekly_cases$week (integer).

Consider wrapping in as.integer() for explicit type consistency:

♻️ Suggested refinement
-  weekly_cases_outbreak <- case_data[, week := floor(onset / 7)
+  weekly_cases_outbreak <- case_data[, week := as.integer(floor(onset / 7))
                            ][, list(weekly_cases = .N), by = week
                              ]

@sbfnk sbfnk added this pull request to the merge queue Feb 11, 2026
Merged via the queue into epiforecasts:main with commit 51dd7d2 Feb 11, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants