-
-
Notifications
You must be signed in to change notification settings - Fork 189
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
Expose keep_mu option to users to make mu explicit in code and summary #1610
Conversation
- also with stan_log_lik_student_time
- otherwise it was incosistent with same code in stan_predictor, causing a problem with eigenMsar
- it attempts to check if priors will become valid when dpar=mu is added. - add test for repair_prior - this now ensures that stancode and standate can regenerate code and data from old models with keep_mu = TRUE - regenerate documentation - handle case where mu is predicted explicitely
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #1610 +/- ##
==========================================
+ Coverage 82.06% 82.08% +0.01%
==========================================
Files 70 70
Lines 19886 19928 +42
==========================================
+ Hits 16320 16358 +38
- Misses 3566 3570 +4 ☔ View full report in Codecov by Sentry. |
Thank you a lot! The PR is a bit overwhelming though. I wish you would have discussed with me first before doing this major work. The challenge I have now is that it is hard for me to tell whether the approach you choose is a good approach to maintain in the future. Or, to put it differently, perhaps it would make sense to abandon the "mu requirement" altogether in some more general sense (not clear on the implications yet). I any case, I will need some time to look into your PR and determine whether this is a good way forward. |
I completely agree, I should have opened an issue to discuss this. I thought it would be a much smaller change but then got absorbed by making it work and by the time I realized I should have open a discussion I was almost done. So I decided to post the PR and use that as a discussion. It's on me, so I don't want to put any pressure on you - if you decide this is not a good approach that's totally fine! |
Closing this as we've found a way to circumvent the issue in our own package, and as we discussed this would be up for a more general restructuring of how mu is treated in brms 3.0 #1660 |
Summary
This PR exposes the option keep_mu in check_prefix() to the user-facing functions brm(), stancode(), standata(), validate_formula(), and default_prior(). The default is keep_mu = FALSE, in which case everything is the same as before. When keep_mu = TRUE, the mu parameter is labeled explicitely in the stancode, standata and the resulting summary object. Thus, mu is treated like any other parameter. I hope you like the idea - the behavior is completely optional but ti will be useful for our team.
Example
Details
This is a follow up on this issue. It does not allow arbitrary parameters to be the main, but it makes the mu parameter transparent, and handled much more like other parameters, which is a small step towards that. It is completely optional behavior, and I have tested it extensively to make sure that when the option is false (as by default), the current brms behavior is not affected.
I thought this will be a quick feature so I tried to implement directly and see if you like it. It turned out to be trickier than I thought and took much longer than I expected, but on the plus side I now I understand much better how brms works internally. I saw that check_prefix has an option
keep_mu
, but a lot of changes were necessary to make sure that nothing breaks when this option is exposed and set to TRUE.The basic implementation is:
validate_formula
. Within validate_formula, a new attribute of formula$formula called "keep_mu" is created. This attribute gets also automatically copied to bterms$mu$formulastan_center_X()
function instan_predictors.R
, a new utility functionstan_keep_mu()
can be used to flexibly determine when this attribute affects behaviorcheck_prefix
andcombine_prefix()
, which now have a default argument keep_mu = NULL (FALSE previously). If the keep_mu argument is NULL, they use stan_keep_mu() to determine whether to keep the mu prefix or not. This was necessary because otherwise "_mu" is sometimes added to the wrong variables such as the responseY
.Tests
Additional features and changes
prior()
doesn't understanddpar = "mu"
#1368). To keep backwards compatibility, this is supressed in validate_prior if keep_mu = FALSE (the default)options(brms.keep_mu = TRUE)
Below is an example code file with the new options:
Full example code of different functionality
Replicating https://paul-buerkner.github.io/brms/articles/brms_distreg.html#zero-inflated-models with keep_mu = TRUE
prior no longer removes the mu parameter:
but this will be removed internally if keep_mu = FALSE (default) for backwards compatibility
and kept if keep_mu = TRUE. keep_mu can be set globally via options as well
stancode and standata also have the keep_mu argument
Differences in the two codes:
stancode and standata can regenerate the stan code and data from older fits with keep_mu = TRUE