refactoring Stan code for efficiency [WIP]#1274
refactoring Stan code for efficiency [WIP]#1274bob-carpenter wants to merge 3 commits intoepiforecasts:mainfrom
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
WalkthroughThe pull request modifies two Stan function files. In convolve.stan, the loop bounds logic is adjusted and allocation timing changed. In gaussian_process.stan, a new matern_indices helper function is introduced and Matern spectral density computations are refactored to use it. Changes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Tip 🧪 Unit Test Generation v2 is now available!We have significantly improved our unit test generation capabilities. To enable: Add this to your reviews:
finishing_touches:
unit_tests:
enabled: trueTry it out by using the Have feedback? Share your thoughts on our Discord thread! 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
🤖 Fix all issues with AI agents
In `@inst/stan/functions/convolve.stan`:
- Around line 74-79: The loop currently calls calc_conv_indices_xlen for all s
up to max(len, xlen), which causes end_x = s to exceed xlen when s > xlen and
leads to out-of-bounds access; modify the loop in the convolve implementation so
that for s <= xlen it uses calc_conv_indices_xlen but for s > xlen it uses
calc_conv_indices_len (or otherwise ensure end_x is capped at xlen), updating
z[s] = dot_product(...) to use the indices returned by the correct helper (refer
to calc_conv_indices_xlen, calc_conv_indices_len and the loop variable s) so no
slice of x exceeds xlen.
In `@inst/stan/functions/gaussian_process.stan`:
- Around line 69-71: The code declares real factor1 but returns factor which is
undefined; change the return to use factor1 (i.e., return factor1 ./ denom) so
the function references the declared variable factor1 instead of the
non-existent factor in gaussian_process.stan (look for the declarations of
factor1 and denom and the return statement).
- Around line 36-38: The function matern_indices currently declares parameter L
as int but callers (diagSPD_Matern12, diagSPD_Matern32, diagSPD_Matern52) pass a
real, causing a Stan type mismatch; change the signature of matern_indices to
accept real L instead of int, and ensure uses inside (e.g., factor = pi() / (2 *
L) and linspaced_vector(M, factor, factor * M)) remain consistent with real
arithmetic so the function compiles and matches its callers.
🧹 Nitpick comments (1)
inst/stan/functions/gaussian_process.stan (1)
86-89: Unused variableindices.The
indicesvector on line 86 is computed but never used — the function now relies onmatern_indices(M, L)instead. This is dead code that should be removed.♻️ Proposed fix to remove dead code
vector diagSPD_Matern52(real alpha, real rho, real L, int M) { - vector[M] indices = linspaced_vector(M, 1, M); real factor = 16 * pow(sqrt(5) / rho, 5); vector[M] denom = 3 * pow(5 / square(rho) + matern_indices(M, L), 3); return alpha * sqrt(factor ./ denom); }
|
Hi @bob-carpenter, Thank you for this, and yes we are very keen to have some help with efficiency tuning. Sorry its a bit annoying to work with. Stan error: This seems like an odd one. I would imagine that the problem here is due to a mismatch between stan versions in Linting: That isn't ideal. We have the In terms of testing, our CI has correctness checks and benchmarking (it will post as a comment if not failing!) so it should be enough to bounce off, I would hope. When I do local development I use our "cmdstanr" backend for ease (https://epiforecasts.io/EpiNow2/reference/stan_sampling_opts.html?q=cmdstan#arg-backend) and then use devtools::test() as you are doing. |
2 participants
Work in Progress: do not merge
I am creating this intermediate PR to see if the EpiNow2 devs want this kind of PR and to ask what kind of testing you want before proceeding to refactor the rest of the code base for efficiency.
Description
This PR closes #1273
Initial submission checklist
devtools::test()anddevtools::check()).devtools::document()).lintr::lint_package()).I'm not an R developer. I installed
devtools::test(), but it's failing with an error I don't understand that looks like it's Stan related as it mentions a Stan signature. I made sure that all of my Stan changes compile in Stan, so I'm not sure where it's coming from or how to track this down. I'll append the error dump below.I don't know what you're expecting from the linter run. The linter dumps out dozens of pages of lint errors when I run it in R on files I haven't touched. Is there a way to see if the current PR introduced new errors?
After the initial Pull Request
ERROR I'M GETTING WITH
devtools::test()Summary by CodeRabbit
New Features
Refactor
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.