-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Add back some missing moments #5147
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 back some missing moments #5147
Conversation
Codecov Report
@@ Coverage Diff @@
## main #5147 +/- ##
==========================================
+ Coverage 77.79% 77.80% +0.01%
==========================================
Files 88 88
Lines 14121 14145 +24
==========================================
+ Hits 10985 11006 +21
- Misses 3136 3139 +3
|
pymc/distributions/continuous.py
Outdated
@@ -1399,6 +1405,12 @@ def dist(cls, lam, *args, **kwargs): | |||
# Aesara exponential op is parametrized in terms of mu (1/lam) | |||
return super().dist([at.inv(lam)], **kwargs) | |||
|
|||
def get_moment(rv, size, lam): |
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.
We are converting lam to mu in the dist
class method, so get_moment
should expect mu
already and you don't need to invert it (the same happens with the logcdf below).
That's why your tests are failing for the exponential.
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.
Ah that makes sense, thanks! I'll make that change.
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 to me. Waiting to see if the tests pass
Thanks once again @michaeloriordan! |
Add moments and tests for the below distributions as part of #5078