Fix #579: re-enable approx inference vignette evaluation#591
Fix #579: re-enable approx inference vignette evaluation#591seabbs merged 5 commits intoepinowcast:mainfrom
Conversation
The vignette was disabled due to a brms path issue with CmdStan 2.37's pathfinder algorithm (paul-buerkner/brms#1831). This has been fixed in the development version of brms, so add brms to Remotes and re-enable vignette evaluation. Closes epinowcast#579 Co-authored-by: Sam Abbott <contact@samabbott.co.uk>
WalkthroughThe pull request adds a remote dependency entry to the package DESCRIPTION file, specifying Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Add NEWS.md entry for the approx inference vignette re-enablement. Created epinowcast#592 to track removing brms from Remotes once CRAN catches up. Co-authored-by: Sam Abbott <contact@samabbott.co.uk>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
DESCRIPTION (1)
73-73: If retaining the GitHub dependency, pin to a specific commit; otherwise, consider removing it.The GitHub dependency was added to access the pathfinder fix before CRAN release. However, brms 2.23.0 (released September 2025) includes pathfinder support and any related fixes as of that date. As of April 2026, this dependency may now be unnecessary.
If the dependency must remain, pin to a specific commit or tag to ensure build reproducibility:
Suggested pin format
- paul-buerkner/brms, + paul-buerkner/brms@<commit-sha>,Otherwise, verify that CRAN brms can be used directly and remove this entry. If removal is planned, ensure issue
#579tracks the cleanup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@DESCRIPTION` at line 73, The DESCRIPTION entry currently lists the GitHub dependency "paul-buerkner/brms,"; either remove this dependency if CRAN brms >= 2.23.0 (which contains pathfinder) can be used, or pin the GitHub reference to a specific commit or tag to ensure reproducible builds (replace "paul-buerkner/brms" with a commit-pinned ref); if you remove it, add a note referencing issue `#579` to track the cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@DESCRIPTION`:
- Line 73: The DESCRIPTION entry currently lists the GitHub dependency
"paul-buerkner/brms,"; either remove this dependency if CRAN brms >= 2.23.0
(which contains pathfinder) can be used, or pin the GitHub reference to a
specific commit or tag to ensure reproducible builds (replace
"paul-buerkner/brms" with a commit-pinned ref); if you remove it, add a note
referencing issue `#579` to track the cleanup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 013cebc9-2ba7-4b06-a453-449a5beb378c
📒 Files selected for processing (2)
DESCRIPTIONvignettes/approx-inference.Rmd
seabbs
left a comment
seabbs
left a comment
There was a problem hiding this comment.
Not ideal we need to rely on an unreleased version but seems like a reasonable solution for now. I have had @seabbs-bot make an issue for tracking the release of brms so we can move to using that when released.
Pre-compile the Stan model before timing comparisons so compilation cost is not included in HMC time. Use update() throughout for all inference methods. Remove outdated Pathfinder stability warning as the issue has been fixed in recent brms/cmdstanr versions. Co-authored-by: Sam Abbott <contact@samabbott.co.uk>
Head branch was pushed to by a user without write access
Use brms empty = TRUE pass-through to compile the model without fitting, rather than running a minimal fit with chains = 1, iter = 5. Co-authored-by: Sam Abbott <contact@samabbott.co.uk>
update() doesn't work on empty brmsfits, so use a minimal fit (chains = 1, iter = 5) for pre-compilation instead. Co-authored-by: Sam Abbott <contact@samabbott.co.uk>
|
@athowes I added precompilation here and that changed the timing difference by about 5 times. Also note that with the newest release of pathfinder things seem much more stable. |
2 participants
This is entirely from an agent so do not review until I have pinged for review as I will do a first pass
Summary
eval = TRUE)paul-buerkner/brmstoRemotesin DESCRIPTION to use the development version which includes the pathfinder path fix (Account for newpath__variable in CmdStan 2.37 withalgorithm="pathfinder"paul-buerkner/brms#1831)update()throughout for all inference methods (HMC, Laplace, ADVI, Pathfinder)Context
The vignette was disabled because brms had a path issue when reading Stan files with CmdStan 2.37's pathfinder algorithm. This has been fixed in the development version of brms but not yet released to CRAN (current CRAN version 2.23.0 was published 2025-09-09, fix merged 2025-10-31).
Test plan
paul-buerkner/brmsfrom Remotes once a CRAN release includes the fix (tracked in Remove paul-buerkner/brms from Remotes once CRAN release includes pathfinder fix #592)Closes #579
This was opened by a bot. Please ping @seabbs for any questions.