epinowcast / epidist

Estimate epidemiological delay distributions with brms
http://epidist.epinowcast.org/
Other
13 stars 5 forks source link

Transfer functionality of `sample_model` to `epidist::epidist` #163

Closed athowes closed 4 months ago

athowes commented 4 months ago

We have the function sample_model which does various things on top of fitting a brms model.

These things are:

  1. Failure tolerant model fitting. Use purrr::safely. If there is an error keep the error.
  2. Have a diagnostics argument. Create divergent transitions metrics.
  3. Add the timing

I think that we should decide on which of these features to keep, implement them into epidist::epidist, then delete this function.

My opinion about what to keep:

  1. Someone can write their own wrapper for this. If we do have it, I'd support an option safely = FALSE which if TRUE will do this for someone. I guess our use-case is that we pass to brms, so perhaps some safety is warranted.
  2. I think this could be a reasonable thing to have, though of course it's possible to do yourself it is a little bit tricky and you can do it wrong. I think if we do have this then we should create a function epidist_diagnostics or similar to house the functionality and call it within epidist if diagnostics = TRUE

There's also a question here about how to return these objects. The brms output is already a complicated object, so in some sense feels like it would't hurt much to add more elements onto its list. I'd prefer for the output of epidist::epidist to be ammenible to standard brms methods so I wouldn't want to have like output$fit and output$diagnostics particularly.

seabbs commented 4 months ago

So this exists to make it easier to run models at scale and was part of the pipeline code we needed for the paper. I agree we should keep the output of epidist as simple/as close to brms as possible.

course it's possible to do yourself it is a little bit tricky and you can do it wrong.

I think this is likely true. I think the main kicker here is the running in a fault tolerant manner which large pipelines often need. I think we could largely get around this by adding some documentation or an example somewhere?

epidist_diagnostics

Good idea. This or something like it should take the output of epidist I think like the standard cmdstan tools do for directly fitted models.

I'd prefer for the output of epidist::epidist to be ammenible to standard brms methods so I wouldn't want to have like output$fit and output$diagnostics particularly.

I strongly agree if we can manage - we can make a diagnostic function take this as an input and then return diagnostics.

I think we agree we should

  1. Remove sample_model if we can fitting method agnostically support diagnostics in another function.
  2. Add a diagnostics function that dispatches on the output of epidist and returns diagnostics + timings. The only slightly tricky thing here is diagnostics differ between samplers and so moving this outside of epidist could make that hard to track (this is the reason that in epinowcast it is a custom wrapper around sample etc vs being multiple tools.
  3. Document an approach to deal with stochastically failing models.
athowes commented 4 months ago

Having a FAQ about how to do things that you could do e.g. purrr::safely.

athowes commented 4 months ago

If we have different opinions about diagnostics then we could implement that into epidist_diagnostics. We do have users less familiar with Bayesian methods.

athowes commented 4 months ago

Add documentation for safely and about timings somewhere.

athowes commented 4 months ago

My current thinking on this is:

athowes commented 4 months ago

Closed via #181 and #175.