Fix date-dimension mismatch in forecast_infections#1324
Conversation
Move summary() call before args modification to ensure dates are calculated from the original fit dimensions rather than extended dimensions. This fixes the error when extending R trajectory beyond the original observation period. Co-Authored-By: Sebastian Funk <sebastian.funk@lshtm.ac.uk>
|
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 PR addresses a date-dimension mismatch bug in Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
NEWS.md (1)
5-5: Start the entry with the action verb "Fixed" per repository conventions.The coding guidelines require NEWS.md entries to start with action verbs ('Added', 'Fixed', 'Updated', 'Changed'). Consider rephrasing for consistency with other entries in the file.
📝 Suggested rewording
-- A bug was fixed in `forecast_infections()` where the summary call to extract dates was using modified args instead of the original fit dimensions, causing a date-dimension mismatch when extending the R trajectory beyond the original observation period. +- Fixed a bug in `forecast_infections()` where the summary call to extract dates was using modified args instead of the original fit dimensions, causing a date-dimension mismatch when extending the R trajectory beyond the original observation period.As per coding guidelines: "Start with action verbs: 'Added', 'Fixed', 'Updated', 'Changed'"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@NEWS.md` at line 5, Update the NEWS.md entry to begin with the action verb "Fixed" to match repository conventions: rephrase the line so it starts "Fixed" and then describes the bug in forecast_infections(), e.g. "Fixed a bug in forecast_infections() where the summary call to extract dates used modified args instead of the original fit dimensions, causing a date-dimension mismatch when extending the R trajectory beyond the original observation period." Ensure the entry references forecast_infections() and retains the same technical detail.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@NEWS.md`:
- Line 5: Update the NEWS.md entry to begin with the action verb "Fixed" to
match repository conventions: rephrase the line so it starts "Fixed" and then
describes the bug in forecast_infections(), e.g. "Fixed a bug in
forecast_infections() where the summary call to extract dates used modified args
instead of the original fit dimensions, causing a date-dimension mismatch when
extending the R trajectory beyond the original observation period." Ensure the
entry references forecast_infections() and retains the same technical detail.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 62d2d82d-161b-4d93-aa66-d242ae0ebe8b
📒 Files selected for processing (2)
NEWS.mdR/simulate_infections.R
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
seabbs
left a comment
There was a problem hiding this comment.
There needs to be evidence that this works. I suggest adding a unit test that would fail on main and now passes that replicates the usage in the synthetic example case. It would be nice to run the synthetic generator here as well (or show evidence if this was done)
- Fix NEWS.md to start with action verb "Fixed" - Add unit test for extended R trajectory scenario Co-Authored-By: Sebastian Funk <sebastian.funk@lshtm.ac.uk>
|
Thanks for the review! I've addressed the feedback:
Running the synthetic validation locally now to provide evidence. |
|
Synthetic validation evidence: Ran the core rt.R scenario locally with the fix: This confirms the fix works for the exact scenario that was failing (140-element R trajectory on a 100-day fit). |
|
Triggered the synthetic validation workflow on this branch: https://github.com/epiforecasts/EpiNow2/actions/runs/22853331016 This should pass now (previous runs were failing with the date-dimension mismatch bug this PR fixes). |
Description
This PR closes #1318.
Fixed a bug in
forecast_infections()where callingsummary()after modifyingestimates$argscaused a date-dimension mismatch when extending the R trajectory beyond the original observation period. The fix moves thesummary()call to before the args modification block.Initial submission checklist
devtools::test()anddevtools::check()).devtools::document()).lintr::lint_package()).After the initial Pull Request
Summary by CodeRabbit