-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Add tests for distributions moments #5087
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
Conversation
Codecov Report
@@ Coverage Diff @@
## main #5087 +/- ##
==========================================
- Coverage 77.81% 77.78% -0.04%
==========================================
Files 87 88 +1
Lines 14008 14115 +107
==========================================
+ Hits 10900 10979 +79
- Misses 3108 3136 +28
|
Definitely
None yet. We should probably test that it respects the RV size, while also working with broadcasted params. |
d76c132
to
2be8cdc
Compare
2be8cdc
to
1583962
Compare
assert_moment_is_expected(model, expected) | ||
|
||
|
||
@pytest.mark.skip(reason="aeppl interval transform fails when both edges are None") |
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.
1583962
to
e098857
Compare
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 can't approve it, even though @ricardoV94 did 98 % of the changes.
e098857
to
490ac22
Compare
Fixes some pre-existing moments Adds moments for HalfNormal and TruncatedNormal distributions Adds helper rv_size_is_none function
490ac22
to
7ddc9d4
Compare
Do we still need more moments? |
Yes, see #5078 |
@@ -828,6 +850,12 @@ def dist(cls, sigma=None, tau=None, sd=None, *args, **kwargs): | |||
|
|||
return super().dist([0.0, sigma], **kwargs) | |||
|
|||
def get_moment(rv, size, loc, sigma): | |||
moment = loc + sigma |
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 have a doubt here.
Current implementation: loc + sigma
Wikipedia version: loc + \sqrt{\frac{2}{\pi}}sigma
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 you are right, we should probably be using that. Do you want to open a PR to fix it?
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.
Sure, happily :)
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.
A bit more involved, we should probably also use the truncated normal mean as the moment. We might even be able to simplify the switch statement, if the normal logcdf behaves well with +- infinity values: https://en.wikipedia.org/wiki/Truncated_normal_distribution
Just checking the CI already.
Do we want to systematically test the moments?
Any ideas how to do that without lots of copy-paste?
Related to #5078