-
Notifications
You must be signed in to change notification settings - Fork 4
Multinomial distribution #258
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #258 +/- ##
==========================================
+ Coverage 82.01% 84.28% +2.26%
==========================================
Files 41 41
Lines 3409 3397 -12
==========================================
+ Hits 2796 2863 +67
+ Misses 613 534 -79 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| for n in 2:6 | ||
| for trials in 2:10 | ||
| @testset let d = Multinomial(trials, normalize!(rand(rng, n), 1)) | ||
| test_exponentialfamily_interface(d; option_assume_no_allocations = false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR is good. Only thing could you add some ad-hoc tests for the methods that we skipped to test in our generic test?
|
@Nimrais could you add a test for mean parametrization Fisher information? |
|
ok np |
|
LGTM!! My only comment would be to use |
Together with @esther-van-pelt . There are a couple of issues with the Multinomial distribution:
(1) The Fisher information, which I took from the
wipfolder implementation, returns non-invertible matrices. This is similar to what happens inCategorical, which makes sense because their functional form is really similar.(2) logpartition is not differentiable for uniform Multinomial distribution. This is because of an optimization in
logsumexpwhich renders it not differentiable byForwardDiff.(3)
prodinvolves a sampling procedure, I did not want to take a dependency onStableRNGsso I used the builtinMersenneTwisterto seed the random number generator.