epinowcast / epidist

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

`epidist_prior` accepting a family or an `epidist_family` #298

Closed athowes closed 2 weeks ago

athowes commented 1 month ago

In working on PR #297 I notice that epidist_prior currently takes a family as input (i.e. a default brms family):

epidist_prior <- function(data, family, formula, prior) {
  epidist_validate(data)
  family <- brms:::validate_family(family)
  # ...
}

Arguably it should take an epidist_family as input instead (i.e. a custom brms family). Perhaps this would be conflating in some way the "model priors" and the "family priors". That is, by family priors are we talking about standard family priors or custom family priors?

If we did take the custom family prior then it would seem odd to dispatch on lognormal -- it would be dispatching on latent_lognormal naturally. And then we are sort of losing the shared family aspect between models. (Of course we could regex out the lognormal part here but then I guess for other custom families it wouldn't be obvious how to get the lognormal part out.)

Perhaps there is some argument again here for carrying around the lognormal family within the latent_lognormal family so that we can just keep using the custom family once we've made it.

(Some background for this is that you could argue it's ugly to have to call family <- brms:::validate_family(family) multiple times. It's already called in the epidist_family so why have to call it again in epidist_prior as I have done in PR #297.)

seabbs commented 1 month ago

Perhaps there is some argument again here for carrying around the lognormal family within the latent_lognormal family so that we can just keep using the custom family once we've made it.

Yes I think we should do this

athowes commented 3 weeks ago

Adding comments from duplicate issue #198:

https://github.com/epinowcast/epidist/issues/198#issue-2431915565

Here it's a challenge whether things are family as input or epidist_family as input (likewise formula or epidist_formula)

_Originally posted by @athowes in https://github.com/epinowcast/epidist/pull/183#discussion_r1689463807_

Within the code, I am sometimes confused whether something is say a family (brmsfamily object) or the result of a call to epidist_family.

https://github.com/epinowcast/epidist/issues/198#issuecomment-2252446958

Additionally, whether functions should take the family or epidist_family version e.g.

Should this instead be a brmsfamily object? Could still extract out the dpars

_Originally posted by @athowes in https://github.com/epinowcast/epidist/pull/183#discussion_r1689469410_

https://github.com/epinowcast/epidist/issues/198#issuecomment-2293106016

Came up here: https://github.com/epinowcast/epidist/pull/242#discussion_r1717141121. Sam suggests one option here is to put the formula (or family) into the epidist_ version.

athowes commented 2 weeks ago

Notes on implementation here.

So we can see that in the epidist call it's just the prior which is taking family seemingly out of turn when it should be taking epidist_family.

epidist_family <- epidist_family(data, family)
epidist_formula <- epidist_formula(
   data = data, family = epidist_family, formula = formula
)
epidist_prior <- epidist_prior(
   data = data, family = family, formula = epidist_formula, prior
)
epidist_stancode <- epidist_stancode(
  data = data, family = epidist_family, formula = epidist_formula
)
fit <- fn(
  formula = epidist_formula, family = epidist_family, prior = epidist_prior,
  stanvars = epidist_stancode, backend = backend, data = data, ...
)
class(fit) <- c(class(fit), "epidist_fit")

Two options here:

  1. Put epidist_family$family
  2. Is there a way to make epidist_prior use the output of epidist_family?

I recon the thing which makes most sense could be 2.

I think one thing that could matter is whether we want to be dispatching on custom families (like latent_lognormal) or on regular families with epidist_family_prior.

Of course at the moment we can just extract out the regular family name..

Ok another note. The epidist_family code already adds family as a class! So we have code duplication here right.

  epidist_validate(data)
  family <- brms:::validate_family(family)
  class(family) <- c(family$family, class(family))
  family <- .add_dpar_info(family)
  custom_family <- epidist_family_model(data, family, ...)
  class(custom_family) <- c(family$family, class(custom_family))
  custom_family <- epidist_family_reparam(custom_family)