Reuse generation-time support grid across Rt Newton updates#1403
Reuse generation-time support grid across Rt Newton updates#1403ajh-oai wants to merge 4 commits into
Conversation
WalkthroughThe PR refactors ChangesNewton-step precomputation optimisation
🚥 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 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
Co-authored-by: Codex <noreply@openai.com>
2a16124 to
9eac792
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
inst/stan/functions/rt.stan (1)
84-100: ⚡ Quick winDocument the
zero_seriesparameter.The docstring should document all parameters of the function it precedes. Add an
@paramentry forzero_seriesto describe its purpose (the precomputed index sequence [0, 1, ..., len-1]).📝 Suggested documentation addition
* `@param` R Reproduction number * `@param` r Current estimate of the growth rate * `@param` pmf Generation time probability mass function (first index: 0) + * `@param` zero_series Precomputed index sequence (0, 1, ..., len-1) for weighted calculations * `@return` The Newton step for updating r🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@inst/stan/functions/rt.stan` around lines 84 - 100, Add a missing `@param` doc for the zero_series argument in the rt_from_R Newton-step function's docstring: describe zero_series as the precomputed index sequence (0, 1, ..., len-1) used to align pmf and lagging terms in the calculation, and note its expected type/length relative to pmf; update the function comment block that contains the other `@param` entries (R, r, pmf) to include this new `@param` for zero_series so all parameters are documented.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@inst/stan/functions/rt.stan`:
- Around line 84-100: Add a missing `@param` doc for the zero_series argument in
the rt_from_R Newton-step function's docstring: describe zero_series as the
precomputed index sequence (0, 1, ..., len-1) used to align pmf and lagging
terms in the calculation, and note its expected type/length relative to pmf;
update the function comment block that contains the other `@param` entries (R, r,
pmf) to include this new `@param` for zero_series so all parameters are
documented.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4c34eb22-cd8a-484b-9998-0c140924f8c9
📒 Files selected for processing (1)
inst/stan/functions/rt.stan
Co-authored-by: Codex <noreply@openai.com>
Co-authored-by: Codex <noreply@openai.com>
|
This is how benchmark results would change (along with a 95% confidence interval in relative change) if 187de36 is merged into main:
|
Co-authored-by: Codex <noreply@openai.com>
|
This is how benchmark results would change (along with a 95% confidence interval in relative change) if 9ae081f is merged into main:
|
Description
This PR closes #1273.
This is a focused Stan performance change in the reproduction-number to growth-rate conversion path.
R_to_r()now constructs the generation-time support grid once per call and reuses it both for the mean generation-time calculation and for each Newton update, instead of rebuildinglinspaced_vector(...)inside every iteration.The existing 3-argument
R_to_r_newton_step()entry point is retained as a thin wrapper so current test-facing usage remains unchanged. This stays separate from #1274, which is working on other Stan hot paths.Benchmarks
I benchmarked the change locally with paired CmdStan runs on the package's infection-estimation workflow (
example_confirmed[1:60], fixed generation time, standard incubation/reporting delays, 2 chains, 250 warmup + 250 sampling draws, seeds 101/202/303/404, order balanced across baseline and branch runs).infectionsStan profile blockAll four paired runs improved in the profiled
infectionsblock (-3.63%to-4.61%) and in total CmdStan runtime (-0.25%to-1.13%). The repository's Stan benchmark workflow should also post its standard benchmark comparison comment for this PR.Validation
git diff --checkAI assistance disclosure
Codex helped identify the optimization seam, benchmark the candidate changes, and prepare this PR. I reviewed the final patch and benchmark results before submission.
Initial submission checklist
devtools::test()anddevtools::check()).devtools::document()).lintr::lint_package()).After the initial Pull Request
Summary by CodeRabbit
Release Notes