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

Update epidist_prior to return all priors including defaults for additional parameters #108

Closed
Tracked by #234
athowes opened this issue Jun 16, 2024 · 4 comments
Closed
Tracked by #234
Labels
enhancement New feature or request medium Nice to have for next release

Comments

@athowes
Copy link
Collaborator

athowes commented Jun 16, 2024

In PR #95, the version of epidist_prior that we provide is as follows:

epidist_priorA <- function() {
  prior1 <- brms::prior("normal(2, 0.5)", class = "Intercept")
  prior2 <- brms::prior("normal(0.5, 0.5)", class = "Intercept", dpar = "sigma")
  return(prior1 + prior2)
}

Depending on how possible it is to use brms::get_prior in a suitable way, I in this issue I propose that we move to:

epidist_priorB <- function() {
  defaults <- brms::get_prior()
  # check here that defaults actually contains the two intercepts?
  defaults[1] <- "normal(2, 0.5)"
  defaults[2] <- "normal(0.5, 0.5)"
}

Briefly:

  • I think that epidist_prior returning all priors used in the model at once is more "natural"
  • Allows us to suggest that users call epidist_prior to see all priors used in the model such they may then edit one object
    • This simplifies the workflow suggested in current roxygen2 documentation

(I give my reasons for this preference in the comments for #95, but I think summed up here.)

I'm open to disagreement about this, and other suggestions to improve the epidist prior interface.

@athowes athowes added the enhancement New feature or request label Jun 16, 2024
@seabbs
Copy link
Contributor

seabbs commented Jun 17, 2024

I think a sensible thing to do is check for issues around getting all priors from a brms model and if there isn't one opening one (i.e fixing this at the brms level)

@athowes
Copy link
Collaborator Author

athowes commented Jun 17, 2024

I think a sensible thing to do is check for issues around getting all priors from a brms model and if there isn't one opening one (i.e fixing this at the brms level)

Based on this I am unsure that we are on the same page. I suggest that we clear this up during next meeting over voice.

@athowes
Copy link
Collaborator Author

athowes commented Jun 17, 2024

  • Sam A writes:

what if you have a custom wrapper around get_prior?
that merges custom priors into the defaults?
and then pass it to fn?

get_all_priors <- function(...,) {
  defaults <- get_prior(...)
  actual_priors <- defaults - custom_prior #pseudo code
  return(actual_priors)
}
  • Agree that best option would be replacement for fn that gets all the priors
  • Katie notes it's nice to see priors before running model and priors be comprehensive

@athowes athowes changed the title Update epidist_prior to return all priors (including defaults for additional parameters) Update epidist_prior to return all priors including defaults for additional parameters Jun 24, 2024
@athowes athowes added the medium Nice to have for next release label Aug 9, 2024
@athowes
Copy link
Collaborator Author

athowes commented Aug 25, 2024

This is complete as of #242.

@athowes athowes closed this as completed Aug 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request medium Nice to have for next release
Projects
None yet
Development

No branches or pull requests

2 participants