Keep calm and carry on #56
Conversation
See GitHub issue statrs-dev#41. Don't panic when calculating a pdf, cdf or pmf whenever the input is a value that the distribution cannot attain. The pdf, pmf and cdf are still defined at those inputs and we should return the correct values. This commit also adds a few tests to each distribution to test that pdf(-inf)=0, pdf(inf)=0, cdf(-inf)=0 and cdf(inf)=1. It also uses simple numerical integration to test for continuous distributions that the integral of the pdf is approximately equal to the cdf, and for discrete distributions that the sum of the pmf is equal to the cdf.
There was a problem hiding this comment.
This is a great PR. Thank for you for doing this. One thing I'd like is if you could run rustfmt in order to ensure consistent formatting. I've just been using the default configuration so that should be fine. Also, if you're up to it, adding tests for the upper bounds (i.e. x == f64::INFINITY) would be greatly appreciated but if not, I'll just add an issue for it to be done at a later date. Again, thanks for doing this. Also if you could update the CHANGELOG.md with the changes that you've made under a new v0.8.0 section that would be awesome. Something similar to what you have in the PR description would work perfectly.
src/distribution/binomial.rs
Outdated
| /// # Formula | ||
| /// | ||
| /// ```ignore | ||
| /// (n choose k) * p^k * (1 - p)^(n - k) | ||
| /// ``` | ||
| fn pmf(&self, x: u64) -> f64 { | ||
| assert!(x <= self.n, | ||
| format!("{}", StatsError::ArgLte("x", 1.0))); | ||
| if x > self.n { |
There was a problem hiding this comment.
can we put this into the match statement? same with the ln_pmf method below
There was a problem hiding this comment.
No, the opposite should happen: the match should go away. Matching on floating-point values is deprecated. It's currently only a warning but it will become a hard error in the future.
| 0.0 | ||
| } else if self.freedom > 160.0 { | ||
| self.ln_pdf(x) | ||
| self.ln_pdf(x).exp() |
There was a problem hiding this comment.
good catch, could we possibly add a test case for this?
| @@ -566,14 +564,14 @@ mod test { | |||
| test_almost(10.0, 10.0, 1.0251532120868705806216092933926141802686541811003037e-30, 1e-44, |x| x.pdf(10.0)); | |||
| test_almost(10.0, 1.0, 0.0000010137771196302974029859010421116095333052555418644397, 1e-20, |x| x.pdf(1.0)); | |||
| test_almost(10.0, 1.0, 0.12511003572113329898476497894772544708420990097708601, 1e-15, |x| x.pdf(10.0)); | |||
| test_is_nan(10.0, f64::INFINITY, |x| x.pdf(1.0)); | |||
| test_is_nan(10.0, f64::INFINITY, |x| x.pdf(f64::INFINITY)); | |||
| test_is_nan(10.0, f64::INFINITY, |x| x.pdf(1.0)); // is this really the behavior we want? | |||
There was a problem hiding this comment.
Honestly not sure what the desired behavior is in this case. We can leave it for now and open another issue for discussion. See #57
Used rustfmt to fix the fact that I used tabs instead of spaces in my previous commit. Rustfmt also changed quite a few other things, some reasonable, some unreasonable. I undid the unreasonable changes manually, but left the ones that seemed reasonable.
These tests are already done for each distribution in I also updated the changelog. Rustfmt clearly hasn't been run in a while, because when I ran it, a lot of things were changed that I haven't touched. And some of the things rustfmt did (especially in src/function/factorial.rs and src/function/stable.rs) look very strange, so I undid them. It seems rustfmt with default arguments does some weird things within {} brackets. Perhaps it would be good to use some non-default rustfmt parameters and add those to a rustfmt.toml file to fix this. |
|
Cool, looks good. I'll merge this in and open up issues for removing matches on floats and for creating a rustfmt.toml |
See issue #41.
Don't panic when calculating a pdf, cdf or pmf whenever the input is a value that the distribution cannot attain. The pdf, pmf and cdf are still defined at those inputs and we should return the correct values.
This commit also adds a few tests to each distribution to test that pdf(-∞)=0, pdf(∞)=0, cdf(-∞)=0 and cdf(∞)=1. It also uses simple numerical integration to test for continuous distributions that the integral of the pdf is approximately equal to the cdf, and for discrete distributions that the sum of the pmf is equal to the cdf.