Skip to content
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

Generalise handling priors for parameters set to constant #306

Closed
athowes opened this issue Sep 11, 2024 · 3 comments · Fixed by #379
Closed

Generalise handling priors for parameters set to constant #306

athowes opened this issue Sep 11, 2024 · 3 comments · Fixed by #379
Assignees
Labels
high Required for next release

Comments

@athowes
Copy link
Collaborator

athowes commented Sep 11, 2024

#282 (comment) (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
Copy link
Collaborator Author

athowes commented Sep 11, 2024

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
Copy link
Contributor

seabbs commented Sep 11, 2024

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

@athowes athowes added the high Required for next release label Sep 18, 2024
@athowes athowes self-assigned this Oct 10, 2024
@athowes
Copy link
Collaborator Author

athowes commented Oct 10, 2024

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.

seabbs pushed a commit that referenced this issue Oct 15, 2024
…erwriting (#379)

* Add abort/warn option to .replace_prior then use it in epidist_prior, and delete NULL case in code

* Fix bug (forgot to update name)

* Update tests

* Use code linking in epidist()

* Add argument documentation

* Redocument

* Add testing that the parameter set to a constant is indeed a constant

* Switch to "do nothing, or warn" prior set-up

* Better warning message and bug fix

* Expect a warning not an error

* Alter args of .replace_prior to make more external sense

* Keep old_prior as first argument
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
high Required for next release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants