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

107 estimation of bayes factors for models including random effects requires brmssave pars #110

Conversation

GidonFrischkorn
Copy link
Collaborator

Summary

  • add function save_pars_bmm2brms to the package.
  • add test to evaluate that the function works as intended

I wanted to avoid that there is a conflict if users have loaded both brms and bmm and therefore named the function differently. Generally, we might want to clarify that we recommend that users load both brms and bmm so that they can access the post-processing of the fit-objects that is supported by brms.

Tests

[x] Confirm that all tests passed
[x] Confirm that devtools::check() produces no errors

Release notes

@GidonFrischkorn GidonFrischkorn added the PR - minor Pull-request should update minor version label Feb 21, 2024
@GidonFrischkorn GidonFrischkorn added this to the 1.0.0 milestone Feb 21, 2024
Copy link
Owner

@venpopov venpopov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for doing this. I see a couple of problems around the current implementation. If people didn't use save_pars= argument and attempt bridge sampling they will get the error:

stop2(
  "Bridgesampling failed. Perhaps you did not set ",
  "'save_pars = save_pars(all = TRUE)' when fitting your model? ",)

which then doesn't math the function name in bmm, so they will still have to load brms. We could add internal checks and rewrite this, but why? Also, if save_pars changes arguments in the future, we'll have to change our function too, which we might not knot until someone reports an error.

A much simpler way is like this:

#' @inherit brms::save_pars title params return
#' @description Thin wrapper around [brms::save_pars()]. When calling
#'   [fit_model] with additional information to save parameters you can use this
#'   function to pass information about saving parameter draws to `brms` without
#'   having to load `brms`. Alternatively, you can also load `brms` and call
#'   `save_pars`. For details see ?brms::save_pars.
#' @export
save_pars <- brms::save_pars

I implemented this and pushed the changes

@GidonFrischkorn
Copy link
Collaborator Author

Yes, this looks way nicer. I tried to import the save_pars function, but this only works for internal development not when loading the package regularly. Thanks for adapting this.

@venpopov venpopov merged commit a89723c into develop Feb 21, 2024
3 checks passed
@venpopov venpopov deleted the 107-estimation-of-bayes-factors-for-models-including-random-effects-requires-brmssave_pars branch February 21, 2024 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR - minor Pull-request should update minor version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Estimation of Bayes Factors for models including random effects requires brms::save_pars
2 participants