epinowcast / epidist

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

Generalise handling priors for parameters set to constant #306

Closed athowes closed 3 weeks ago

athowes commented 1 month ago

https://github.com/epinowcast/epidist/pull/282#discussion_r1753960842 (and comment thread):

replace_prior currently errors when you pass it a prior which there is no matching prior for it to overwrite. One possibility would be just to make it less strict and just to have a warning when it's given a prior which doesn't exist. But this should only happen for the user passing in priors, of course for all our code we shouldn't have routine warnings happening.

To remove anything which is fixed it would be like removing anything which is fixed from each of:

model <- epidist_model_prior(data, formula)
family <-  epidist_family_prior(family, formula)

I can imagine the code for this being like:

model <- epidist_model_prior(data, formula)
family <-  epidist_family_prior(family, formula)
for(dpar in names(formula$pfix)) { ... }

Which I could be happy to move to. It's probably more complicated now but I agree in future it'll be less complicated. It's also mildly unappealing to have e.g. epidist_model_prior return the wrong prior (all else equal then it being lower down the stack is better).

One plan here:

  1. Change replace_prior to do a warning rather than error when there is no prior to replace
  2. Add utility function to remove all parameters set to a constant
  3. Put use of that utility function into epidist_prior and remove the lower level parts about removing priors on constants

The flaws in this plan:

  1. If we change replace_prior to just do a warning then we don't need the utility function to remove priors on constant parameters since default_prior already won't have put a prior on a constant in there, so there will be nothing to replace, hence we just end up with a warning
  2. I don't think users should have to look at warnings for things we are doing internally, the warnings should just come up if they place a prior that doesn't exist
athowes commented 1 month ago

Possible solution:

  1. Have a version of replace_prior which doesn't care if you try to replace things that don't exist and use that for internal replacements.
  2. Also have a version which gives warnings and use that for user replacements.

Downside: maybe it's harder for us to be debugging if any replacements are going wrong.

seabbs commented 1 month ago

just have an optional arg that you set when its time to replace user priors I think

athowes commented 4 weeks ago

As a part of this issue, we might like to also alter the structure of our priors so that epidist_family_prior.lognormal doesn't need as much general code in it.