documenting get_moment #5244
documenting get_moment #5244
Conversation
| @@ -202,6 +212,10 @@ blah = pm.Blah.dist([0, 0], [1, 2]) | |||
| blah.eval() | |||
| # array([0.62778803, 1.95165513]) | |||
|
|
|||
| # Test the get_moment | |||
| pm.distributions.distribution.get_moment(blah).eval() | |||
There was a problem hiding this comment.
| pm.distributions.distribution.get_moment(blah).eval() | |
| from pm.distributions.distribution import get_moment | |
| get_moment(blah).eval() |
| from pymc.distributions import Blah | ||
| from pymc.initial_point import make_initial_point_fn | ||
|
|
||
| def assert_moment_is_expected(model, expected): |
There was a problem hiding this comment.
We shouldn't copy the body of assert_moment_is_expected, just mention it exists, what it does and and how it should be used (which you show below)
There was a problem hiding this comment.
Thanks for the review! That makes sense if eventually the function assert_moment_is_expected changes in the test file we don't have to track the changes and update the documentation. I will work on these changes a commit them soon.
revision_001 after the first commit.
|
I changed and committed a new file based on your suggestions. Again, thanks for reviewing! |
| @@ -202,6 +213,10 @@ blah = pm.Blah.dist([0, 0], [1, 2]) | |||
| blah.eval() | |||
| # array([0.62778803, 1.95165513]) | |||
|
|
|||
| # Test the get_moment | |||
| get_moment(blah).eval() | |||
| # array([0.5, 1. ]) | |||
There was a problem hiding this comment.
These would be array([0., 0.]) with the pseudo code above no?
There was a problem hiding this comment.
This is what I'm getting from get_moment() for blah = pm.Uniform.dist([0, 0], [1, 2])
I did some tests and I think this is the right output. Considering that the first argument is location and second is scale (or lower and upper), then first interval is [0, 1] and second interval is [0, 2]. With the formula mean = 0.5 * [a, b], then we have [0.5, 1.0]. I compared with scipy implementation and got the same result. Let me now if I am mistaken here :)
import pymc as pm
from pymc.distributions.distribution import get_moment
from scipy.stats import uniform
# PyMC implementation
blah = pm.Uniform.dist([0, 0], [1, 2])
get_moment(blah).eval()
>> array([0.5, 1. ])
# spicy implementation
uniform.mean([0, 0], [1, 2])
>> array([0.5, 1. ])I see that there is an error in pre-commit / pre-commit (pull_request) that is related with Trim Trailing Whitespace. Is this something I did wrong? Let me know how I can fix it.
Thank you,
There was a problem hiding this comment.
Your comments & code here are correct. I think Ricardo referred to the example implementation above.
The test case further down also assumes that Blah == Normal, so maybe make this example also with a normal get_moment?
There was a problem hiding this comment.
Thanks for the clarification! Yes, I agree it makes sense to keep it uniform by changing this example from Uniform to Normal. I will commit the changes below.
Codecov Report
@@ Coverage Diff @@
## main #5244 +/- ##
==========================================
+ Coverage 78.94% 79.14% +0.19%
==========================================
Files 88 88
Lines 14237 14306 +69
==========================================
+ Hits 11240 11323 +83
+ Misses 2997 2983 -14
|
|
I think I was able to solve the error in My sublime text wasn't automatically trimming trailing whitespace on save.
|
docs/source/contributing/developer_guide_implementing_distribution.md
Outdated
Show resolved
Hide resolved
docs/source/contributing/developer_guide_implementing_distribution.md
Outdated
Show resolved
Hide resolved
docs/source/contributing/developer_guide_implementing_distribution.md
Outdated
Show resolved
Hide resolved
| @@ -202,6 +213,10 @@ blah = pm.Blah.dist([0, 0], [1, 2]) | |||
| blah.eval() | |||
| # array([0.62778803, 1.95165513]) | |||
|
|
|||
| # Test the get_moment | |||
| get_moment(blah).eval() | |||
| # array([0.5, 1. ]) | |||
There was a problem hiding this comment.
Your comments & code here are correct. I think Ricardo referred to the example implementation above.
The test case further down also assumes that Blah == Normal, so maybe make this example also with a normal get_moment?
docs/source/contributing/developer_guide_implementing_distribution.md
Outdated
Show resolved
Hide resolved
Co-authored-by: Michael Osthege <[email protected]>
|
@michaelosthege Thank you very much for the comments and suggestions! I have accepted them all and added a commit that changes the examples we have discussed from Thank you, |
|
Thank you @ricardoV94 @michaelosthege for your reviews and guidance. It was fun working on this PR, the first one we never forget :) |
This PR updates the file
developer_guide_implementing_distribution.mddocumenting theget_moment()method based on the issue #5210@ricardoV94 I included your instructions from #5078 in the docs where I think they fit better in. Let me know what you think or if I missed anything.
Closes #5210