Binomial and Poisson Moment #5150
Conversation
farhanreynaldo
force-pushed
the
binomial-poisson-moment
branch
from
7dc703a to
cd9f9d1
Compare
Codecov Report
@@ Coverage Diff @@
## main #5150 +/- ##
==========================================
+ Coverage 77.86% 77.92% +0.06%
==========================================
Files 88 88
Lines 14165 14109 -56
==========================================
- Hits 11029 10995 -34
+ Misses 3136 3114 -22
|
|
@farhanreynaldo Need to resolve conflicts. |
This is my first time having a conflict. Are the following steps correct? and then accept both changes and push the resolved conflicts? |
|
I think during rebasing you just git add, and then you force push. |
pymc/distributions/discrete.py
Outdated
| @@ -570,6 +576,11 @@ def dist(cls, mu, *args, **kwargs): | |||
| # mode = intX(at.floor(mu)) | |||
There was a problem hiding this comment.
Seems like we have a leftover from before. Do you mind removing the commented out code?
| # mode = intX(at.floor(mu)) |
There was a problem hiding this comment.
Also note that we used the floor of mu, we should do the same in the new get_moment
There was a problem hiding this comment.
Is the IntX necessary or should I just include the at.floor?
pymc/distributions/discrete.py
Outdated
| @@ -117,6 +117,12 @@ def dist(cls, n, p, *args, **kwargs): | |||
| # mode = at.cast(tround(n * p), self.dtype) | |||
There was a problem hiding this comment.
| # mode = at.cast(tround(n * p), self.dtype) |
Same here, we should remove the old commented out code, and use at.round in the new get_moment
There was a problem hiding this comment.
Is the at.cast also necessary here?
There was a problem hiding this comment.
Not anymore. That's done automatically by the function that calls get_moment
There was a problem hiding this comment.
Left a couple of suggestions @farhanreynaldo. Thanks for the PR!
| (np.arange(1, 5), (2, 4), np.full((2, 4), np.arange(1, 5))), | ||
| ], | ||
| ) | ||
| def test_poisson_moment(mu, size, expected): |
There was a problem hiding this comment.
Some of the test combinations should include non integer values for mu to check the flooring is working as expected
| (10, np.arange(1, 6) / 10, (2, 5), np.full((2, 5), np.arange(1, 6))), | ||
| ], | ||
| ) | ||
| def test_binomial_moment(n, p, size, expected): |
There was a problem hiding this comment.
Some of the test combinations should have a n*p that is different when rounded and not rounded to make sure it is working as expected
Add moments and tests for the below distributions as part of #5078: