-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Add nu parametrization to beta distribution #6344
Add nu parametrization to beta distribution #6344
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #6344 +/- ##
==========================================
- Coverage 94.72% 86.04% -8.69%
==========================================
Files 148 148
Lines 27487 27818 +331
==========================================
- Hits 26038 23936 -2102
- Misses 1449 3882 +2433
|
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.
Thanks for opening the PR. I left a couple of suggestion below.
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.
Looks good. Just not sure about the "sample size" part in the docstrings.
pymc/distributions/continuous.py
Outdated
@@ -1059,10 +1059,11 @@ class Beta(UnitContinuous): | |||
Support :math:`x \in (0, 1)` | |||
Mean :math:`\dfrac{\alpha}{\alpha + \beta}` | |||
Variance :math:`\dfrac{\alpha \beta}{(\alpha+\beta)^2(\alpha+\beta+1)}` | |||
Sample size :math:`\alpha + \beta` |
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.
I am not sure about this one. It's not really a property of the distribution, but an interpretation of the parameters, assuming a specific model
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.
You can add this as a comment in the docstring, but I agree that this is not a property per se.
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.
I was hesitating as well, I will remove the reference to "sample size" without the full explanation. Should I add:
'nu : tensor_like of float, optional
Alternative "sample size" of a Beta distribution for the Haldane prior probability Beta(0,0) (nu
> 0) .
in the parameter section?
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.
I think the parameter description is good enough as it is now.
We can perhaps add a link to the relevant Wikipedia section instead of mentioning the Haldane prior?
https://en.m.wikipedia.org/wiki/Beta_distribution#Mean_and_sample_size
Pre-commit is failing, otherwise the PR looks good to me |
* Add (mu, nu) parametrization * add test beta logp * Add relationship between alpha, beta and nu * change test structure * remove white space accidently added in docstring * remove sample size from docstring --------- Co-authored-by: Michael Osthege <michael.osthege@outlook.com>
What is this PR about?
...
This PR addresses closes #5023 and closes #6295 and incorporates the mu, nu parametrization to the Beta distribution as described here https://en.wikipedia.org/wiki/Beta_distribution as well as fixing the error on the sigma bounds in the docstring.
As for the test, I implemented one, on the logp, and it passed. I assumed that if logp was the same across parametrization, cdf and others would follow. Let me know if this is sufficient.
Checklist
Major / Breaking Changes
Bugfixes / New features
Beta
distribution can now be parametrized bynu
.Docs / Maintenance