Fix a bug in the uniform continuous distribution

[mp4096]

Hi and thanks for the library!

Problem

I think there's a bug in the implementation of the continuous uniform distribution. According to the documentation, it should return values in the range [min, max], i.e. min ≤ random value ≤ max.

Unfortunately, the current implementation generates values in the range [min, max + 1), i.e. min ≤ random value < max + 1.

I think it might be a copy-paste bug 'inherited' from the discrete uniform distribution, where you really need to add 1 to the upper bound.

Solution

This PR is a partial fix for this bug: It changes the range of the continuous uniform distribution to [min, max), i.e. min ≤ random value < max. I don't have a quick fix to include the upper bound, but I think it's important to at least fix the + 1.0 issue. As of 0.7.0, you cannot really sample values between 0 and 1 without resorting to wild workarounds.

References

• MathNET implementation
• discrete uniform distribution in statrs
• rand::Rng trait, which says that gen_range() does not include the upper bound

[vks]

Could you please add a test verifying it works now?

[mp4096]

Sure, but I'm not sure how can you test this in a deterministic way. However, I can just sample a lot of values and see whether they are all in the range [min, max).

[vks]

Yes, I was thinking of something like this:

diff --git a/src/distribution/uniform.rs b/src/distribution/uniform.rs
index 11b03da..b839849 100644
--- a/src/distribution/uniform.rs
+++ b/src/distribution/uniform.rs
@@ -464,4 +464,17 @@ mod test {
         test::check_continuous_distribution(&try_create(0.0, 10.0), 0.0, 10.0);
         test::check_continuous_distribution(&try_create(-2.0, 15.0), -2.0, 15.0);
     }
+
+    #[test]
+    fn test_sample() {
+        use super::Sample;
+
+        let mut u = Uniform::new(0., 1.).unwrap();
+        let mut r = ::rand::StdRng::new().unwrap();
+        for _ in 0..100000 {
+            let x = u.sample(&mut r);
+            assert!(0. <= x);
+            assert!(x < 1.);
+        }
+    }
 }

[vks]

If you want it to be deterministic, you can seed the RNG.

[mp4096]

Wow, we've written the tests within a minute ✋ 😎

[mp4096]

Seeding the PRNG is surely a good idea! I'll take a look at it tomorrow.

[boxtown]

If we could re-name this to something like test_sample_in_range to be a little more explicit that would be awesome

[mp4096]

Yep, fixed it!

[boxtown]

This is an excellent catch. If you could address the comment I'll merge asap. Also if you could update the CHANGELOG.md under v0.8.0 to reflect what you did

[mp4096]

I struggled a bit with the wording for changelog, but I hope one can understand what I mean.