Improve interface around model specification #10
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #10 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 38 39 +1
Lines 1456 1616 +160
==========================================
+ Hits 1456 1616 +160 ☔ View full report in Codecov by Sentry. |
| ##' @param model A list or environment with elements as described in | ||
| ##' Details. |
There was a problem hiding this comment.
where is "Details"? is this a roxygen2 thing that will reference the text above?
There was a problem hiding this comment.
Everything after the first linebreak is Details in the help rendering (or should be)
R/model.R
Outdated
| validate_model_gradient <- function(model, properties, call) { | ||
| if (isFALSE(properties$has_gradient)) { | ||
| return(NULL) | ||
| } | ||
| gradient <- model$gradient | ||
| if (isTRUE(properties$has_gradient) && !is.function(gradient)) { | ||
| cli::cli_abort( | ||
| paste("Did not find a function 'gradient' within your model, but", | ||
| "your properties say that it should do"), | ||
| arg = "model", call = call) | ||
| } | ||
| if (!is.null(gradient) && !is.function(gradient)) { | ||
| cli::cli_abort( | ||
| "Expected 'model$gradient' to be a function if non-NULL", | ||
| arg = "model", call = call) | ||
| } | ||
| gradient | ||
| } | ||
|
|
||
|
|
||
| validate_model_direct_sample <- function(model, properties, call) { | ||
| if (isFALSE(properties$has_direct_sample)) { | ||
| return(NULL) | ||
| } | ||
| direct_sample <- model$direct_sample | ||
| if (isTRUE(properties$has_direct_sample) && !is.function(direct_sample)) { | ||
| cli::cli_abort( | ||
| paste("Did not find a function 'direct_sample' within your model, but", | ||
| "your properties say that it should do"), | ||
| arg = "model", call = call) | ||
| } | ||
| if (!is.null(direct_sample) && !is.function(direct_sample)) { | ||
| cli::cli_abort( | ||
| "Expected 'model$direct_sample' to be a function if non-NULL", | ||
| arg = "model", call = call) | ||
| } | ||
| direct_sample | ||
| } |
There was a problem hiding this comment.
Could this repeated logic be implemented once and take expected property name etc as argument?
There was a problem hiding this comment.
yeah, I wondered about that. Will do
Co-authored-by: Emma Russell <44669576+EmmaLRussell@users.noreply.github.com>
To support having an observer on a stochastic model that can save things like trajectories, and to support adding lots of weird things to support nested models, I think we need a slightly more flexible approach to specifying models.
The motivation for this is a little hard to see, but starts to appear when considering models that run a time series as part of the likelihood calculation. For these, we're going to want to record trajectories over time (either deterministic or some sampled stochastic trajectories). We're likely also to want to store things like the final state so that onward simulation can be continued.
The
mcstate_modelinterface does not need to know about any of this - and we don't want to have an ever increasing set of things to add there as people add new functionality to their models. Instead I'm imagining that we'll have some sort of "observer" that will be added to the call tomcstate_samplethat we'll run at each step and which will receive the mcstate model to prod at it however it wants. So the observer and the model need to agree on the interface but the sampler does not really need to know about this detail at all.Two ways we could do this are to allow
mcstate_modelto accept...with additional properties, or we can compose the finalmcstate_modelout of whatever we're given; I've gone with the latter.I was not totally sure where to putparameters(and then as a resultdomain) so for now I've put them both in the model and in the call tomcstate_modelbut I'm open to removing one of these and requiring that we have these in one place or the other. I'd wondered too about allowingmodelto be a density function (in which case we just domodel <- list(density = density)to allow a really simple interface here; this is much of where allowingparameters/domainthrough has come from. For stochastic models (#9, currently draft) having this format is useful because it allows a model to provide functions that can support a stochastic model and then also gives us something to use to say "but actually this one is deterministic", which is what we do when creating deterministic models from stochastic onesEdit: @M-Kusumgar and I had a chat about this and we're gone for something slightly different here that ends up being a little easier to think about I think. There's now an
mcstate_model_propertiesfunction which can be passed in tomcstate_modelwhich let's you assert what your model can and cannot do. We then look at the model and pull out all the functions that we need into a list but we also keep the original model which allows us to have the user do whatever they want at each mcmc step (in a later PR)