-
Notifications
You must be signed in to change notification settings - Fork 0
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
Support for stochastic models #9
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 38 38
Lines 1575 1597 +22
=========================================
+ Hits 1575 1597 +22 ☔ View full report in Codecov by Sentry. |
9f47375
to
17835a7
Compare
98ed591
to
e180d64
Compare
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.
OK - I think I understand what the PR is adding in terms of the stochastic support - but I haven't grasped the final comment about observers etc - so perhaps missing a bit of bigger picture here... but other than that...!
## in order to get trajectories out etc, but that's not super | ||
## obvious really; that means we're going to need some sort of | ||
## special sauce for the observer function really. | ||
}) |
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.
I have not understood this comment well yet!
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.
ah, it's out of date now after #10
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.
(and now it's gone)
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.
would like to definitely chat through this and that sir model on call!
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.
Looks good! just some minor comments
i = hint), | ||
arg = "model", call = call) | ||
} | ||
list(set = model$set_rng_state) |
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.
just for clarification, why is this returned as list(set = model$set_rng_state)
instead of returning model$set_rng_state
?
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.
Because I anticipate that later we'll need get = model$get_rng_state
too for cases where we have a stochastic model that we want to be able to restart
if (isTRUE(model$properties$is_stochastic)) { | ||
model$model$set_rng_state(rng) | ||
} |
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.
will this be applied to different samplers? if so perhaps we should put this in mcstate_run_chain
right after sampler$initialise
so that this is a general functionality instead of tied to this particular initialisation function
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.
None of the other samplers can cope with stochastic models; they will be throwing errors!
## in order to get trajectories out etc, but that's not super | ||
## obvious really; that means we're going to need some sort of | ||
## special sauce for the observer function really. | ||
}) |
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.
would like to definitely chat through this and that sir model on call!
Note that this PR pulls in
dust
for now, to use in the tests. That is probably not what we really want because dust is going to depend on mcstate for its rng calls! Once we work out what we need, I'll hand-write a version of this model that does the job, but this setup should be ok for a while (probably until we know enough to write dust2, really).I've left a little placeholder in around getting rng state from the model; that is going to interact with restartable mcmc in ways that are going to require some care I think.
There's no actual testing in the stochastic model + random walk; however, this constitutes "particle MCMC" which is cool. There are a couple of small features we will likely add there later based the covid work: