-
Notifications
You must be signed in to change notification settings - Fork 21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add initial support for pathfinder #464
Conversation
This is how benchmark results would change (along with a 95% confidence interval in relative change) if e75618b is merged into main:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One typo, one philosophical. How do we detect the version of CmdStanR installed such that we can alert the user that pathfinder (and eventually laplace) isn't available? Or do we let CmdStan do it for us?
…tests for enw_sample)
5b351da
to
6873623
Compare
This is how benchmark results would change (along with a 95% confidence interval in relative change) if 96444ce is merged into main:
|
This is how benchmark results would change (along with a 95% confidence interval in relative change) if dc6c75e is merged into main:
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #464 +/- ##
==========================================
+ Coverage 97.45% 97.46% +0.01%
==========================================
Files 15 15
Lines 2160 2172 +12
==========================================
+ Hits 2105 2117 +12
Misses 55 55 ☔ View full report in Codecov by Sentry. |
This is how benchmark results would change (along with a 95% confidence interval in relative change) if 0fa2690 is merged into main:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good. I think the method detection is the only think I would recommend. Maybe a cli::cli_abort
rather than cli::warn
?
This is how benchmark results would change (along with a 95% confidence interval in relative change) if e19e5f8 is merged into main:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM--can't wait to play with this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! @seabbs, how much did you already play around with it? I just tested it on your example but with a daily random walk model and generation time, it wasn't too bad in terms of expectation, only the uncertainty quantification got worse (intervals very narrow).
When I tested pathfinder a while ago for EpiSewer
, I wasn't impressed. It didn't seem to like the renewal model at all... Trends were often considerably off, and there was basically no uncertainty. Interestingly, this got worse when running more paths, results were best when running just one path with a limited number of iterations...
Would be great to get a better understanding of pathfinder's behaviour on time series models and possible tweaks, as this is obviously very useful.
Yes, I also only did some limited exploration and found similar. I concluded that its likely useful for prototyping but I wouldn't trust it (on the examples I tested) for anything where I wanted to use the output. @athowes is interested in potentially doing a more in depth look. I think your point about potential tweaks is a good one as it might be something simple that improves performance.
I didn't see this and that is interesting. Suggests instability based on initialisation? |
Description
This PR adds initial support for Pathfinder. It appears to work fairly well for some models and less well for others (generally based on complexity). Definitely more to do here but I think having this functionality in now is useful.
Some likely contributors who may be interested to review @adrian-lison, @medewitt, @sbfnk.
Something this does not add is pathfinder initialisation for NUTs which I am waiting on support in
cmdstanr
for.Try it out with:
@athowes we should check
brms
to see ifPathfinder
is supported as for the same reasons as its useful here it might be handy to surface inepidist
.Checklist