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

Create native Dirichlet multinomial family #1729

Merged
merged 10 commits into from
Feb 3, 2025

Conversation

tom-peatman
Copy link
Contributor

Following discussion in #1728, I've had a go at creating a native Dirichlet multinomial family.

I've attempted to update the documentation of brmsfamily, and added some tests too.

I'm not confident that the posterior_epred.R function is correct, and probably should not have included it.

Hopefully this is helpful - please let me know if you need anything more from me. And thanks again @paul-buerkner for all your help with this!

@paul-buerkner paul-buerkner added this to the brms 2.23.0 milestone Jan 28, 2025
@paul-buerkner
Copy link
Owner

Thank you! This PR looks good overall, I think.

Why are not confident about posterior_epred? You should be able to test it by comparing its output against the known(?) mean of the dirichlet_mulinomial distribution. I currently don't have time to check this detail myself.

@tom-peatman
Copy link
Contributor Author

Thanks @paul-buerkner for taking a look at the PR - much appreciated.

I've now checked posterior_epred output for some simple models fitted to simulated data, and the output of posterior_epred is consistent with the distribution means. My understanding is that posterior_epred for the dirichlet_multinomial distribution should be the same as for the multinomial distribution (as overdispersion shouldn't influence the expected mean), so I believe that it is correct.

I also had a quick scan of the checks above in case the failed checks were related to the dirichlet_multinomial family - I don't think they are.

Copy link
Owner

@paul-buerkner paul-buerkner left a comment

Choose a reason for hiding this comment

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

I have now checked the PR once again in a bit more detail and found a few more minor things to adjust before the PR is ready to be merged.

remove (unused) cats and alpha variables from log_lik_dirichlet_multinomial, and call posterior_epred_multinomial within posterior_epred_dirichlet_multinomial with explanatory comment
@tom-peatman
Copy link
Contributor Author

Thanks @paul-buerkner for reviewing my pull request, and for your helpful comments and suggestions. I've made some minor updates in the latest commit, which hopefully address your questions.

@paul-buerkner
Copy link
Owner

Thanks! Can you add the initial transformations and check also do the density function as discussed above? Then the PR should be ready for merging.

@tom-peatman
Copy link
Contributor Author

I'll implement the checks in the density function as requested above.

Though I'm not sure what you mean by initial transformations?

Do you also want me to add a specific help page for the dirichlet multinomial describing the parameterisation of the distribution (similar to ?BetaBinomial)? I just realised that I didn't do this first time around

add checks for dimensions of eta, and improve documentation
@paul-buerkner
Copy link
Owner

I meant

if (is.null(dim(eta))) {
    eta <- matrix(eta, nrow = 1)
  }

but you already added that. I think it is fine if we keep the density internal for now. Just as we have it with dmultinomial. I will check the PR once again and then merge.

@tom-peatman
Copy link
Contributor Author

OK great - thanks again for all your help with this!

@paul-buerkner
Copy link
Owner

Will merge now. Thank you for your contribution to brms! I highly appreciate it!

@paul-buerkner paul-buerkner merged commit 2720749 into paul-buerkner:master Feb 3, 2025
0 of 5 checks passed
@tom-peatman tom-peatman deleted the dirichlet-multinomial branch February 3, 2025 12:15
@tom-peatman
Copy link
Contributor Author

Thanks @paul-buerkner, it was a pleasure to contribute and the least I can do as a very grateful user of brms!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants