Add zero-inflated Beta-binomial distribution #1311
Conversation
…ull request, will use branch next time
|
Thank you! I will review this PR in the next couple of days :-) |
There was a problem hiding this comment.
This PR appears to be of excellent quality and I have just a few minor comments.
In addition, could you update NEWS and bump the brms version in DESCRIPTION by 0.0.1?
I would like to add you as a contributor. If you don't mind, can you tell me your real name for this purpose?
DESCRIPTION
Outdated
| @@ -46,6 +46,7 @@ Suggests: | |||
| emmeans (>= 1.4.2), | |||
| cmdstanr (>= 0.4.0), | |||
| projpred (>= 2.0.0), | |||
| processx, | |||
There was a problem hiding this comment.
Why and where do we need processx in this PR?
There was a problem hiding this comment.
Sorry, this should be in something separate, if needed at all? It was just throwing a warning in devtools::check as it get’s called on R/backends.R#L633 but wasn’t in the list of suggested packages
There was a problem hiding this comment.
You are right. This had been added recently. I will add it separately to master.
NAMESPACE
Outdated
| @@ -607,6 +610,8 @@ importFrom(bridgesampling,bayes_factor) | |||
| importFrom(bridgesampling,bridge_sampler) | |||
| importFrom(bridgesampling,post_prob) | |||
| importFrom(coda,as.mcmc) | |||
| importFrom(extraDistr,dbbinom) | |||
There was a problem hiding this comment.
Since you seem to access these function via :: to we need to import them?
There was a problem hiding this comment.
What’s the best approach? If we call via :: but the user doesn’t have extraDistr installed they’ll get an error like: Error in loadNamespace(x) : there is no package called ‘extraDistr’ when using loo. But then if we importFrom it means users always have to install extraDistr when installing brms which seems over the top. I’ll remove the importFrom’s and add it to suggests? Or would it be better to try code these density/mass functions in directly in a separate PR to remove the dependency altogether?
There was a problem hiding this comment.
Having it in Suggests is the most sensible thing to do I think. Perhaps for the functions that require a package unders Suggests could be starting with a brms:::require_package(...) call. I will just add it manually to your PR.
|
Brilliant, thanks for reviewing and all the tips/advice! Have made those changes (6abc29a) with just a couple follow up questions/comments above. Full name: Hayden Rabel Thanks again 👍 |
|
Merging now. Thank you very much again for your contribution to brms! |
|
We are now in the intersting position that we have a zero-inflated beta-binomial family but not a regular beta-binomial family. Of course, the latter is already available via a custom family and featured in the custom family vignette but I think it may be time to make beta-binomial built-in as well. Do you perhaps, by any chance, have the time to implement this as well? No worries if not of course, it just happend to cross my mind. |
|
Sweet, thanks! Yep, I'll have a go at the regular beta-binomial early next week. Do you think it'd be worth updating that vignette (which was reallly helpful by the way) to a different custom family currently being requested? Like the generalised Poisson (#739) / Pareto (#1113) distribution, or lognormal with natural scale params (#1027)? Or just make small text edits to reflect that the beta-binomial is [will be] in |
|
Thank you! With regard to the vignette, both is fine I think. Changing the example in the vignette of course has the advantage of getting another family available for users. Perhaps the lognormal with mean-SD parameterization could be interesting, but I don't want to cause you even more work. So whatever you feel like doing for this, would be fine by me. |
Hey hope this zero-inflated Beta-binomial distribution implementation looks okay for #1243.
Mostly followed https://cran.r-project.org/web/packages/brms/vignettes/brms_customfamilies.html and the other families/code already in brms.
Here's a rough example using it with the data from Example 13.1 of McElreath's Statistical Rethinking: https://hdrab127.github.io/brms-zibb-example/brms_zeroinflated_betabinomial.html#model-4-zero-inflated-beta-binomial.
Happy to make any changes or add further tests/docs if it helps. Keen to learn more brms and contribute where possible as might be using a bit this year.
Sorry about the last four commits, please ignore as they don't actually do anything. Hopefully you can squash them in to one on your end, or I can try. Will squash and use branches next time!
Thanks,
Hayden