Extra tests #331
Extra tests #331
Conversation
src/distributions/binomial.rs
Outdated
|
|
||
| let variance = | ||
| results.iter().map(|x| (x - mean) * (x - mean)).sum::<f64>() | ||
| / (results.len() - 1) as f64; |
There was a problem hiding this comment.
I believe the formula for variance is ((x-mean)^2)/(n-1).
There was a problem hiding this comment.
Where did you get that from? It should be over n; you are simply averaging each sample's variance. See https://en.wikipedia.org/wiki/Variance#Discrete_random_variable
Also, it is arguable that you should be using the expected mean instead of the actual mean here. That might improve the tolerance you can use in the test?
There was a problem hiding this comment.
Ah, there is a difference between taking the variance of all results, and of only a sample of them. I should know these things better...
src/distributions/binomial.rs
Outdated
| @@ -129,17 +129,32 @@ mod test { | |||
| use distributions::Distribution; | |||
| use super::Binomial; | |||
|
|
|||
| fn test_binomial_mean_and_variance(n: u64, p: f64) { | |||
| let binomial = Binomial::new(n, p); | |||
| let mut rng = ::test::rng(123); | |||
There was a problem hiding this comment.
Using a fixed RNG is a good idea, but better to pass either the RNG or the seed into the function so not all calls use the same sequence
src/distributions/other.rs
Outdated
| // We can pick from 62 characters. This is so close to a power of 2, 64, | ||
| // that we can do better than `Range`. Use a simple bitshift and | ||
| // rejection sampling. We do not use a bitmask, because for small RNGs | ||
| // the most significant bits are usually of higher quality. | ||
| loop { | ||
| let var = rng.next_u32() >> 26; |
There was a problem hiding this comment.
Maybe write (32 - 6) so people don't get the wrong idea about where 26 comes from?
| .map(|()| rng.sample(Alphanumeric)).take(5).collect(); | ||
| assert_eq!(word.len(), 5); | ||
| .map(|()| rng.gen::<char>()).take(1000).collect(); | ||
| assert!(word.len() != 0); |
There was a problem hiding this comment.
There's not much point keeping this now. There's already a doc-test on Rng doing the same thing.
There was a problem hiding this comment.
I can't find that doc-test to be honest.
There was a problem hiding this comment.
$ rg Alphanumeric src/lib.rs
309:/// use rand::distributions::{Alphanumeric, Range};
318:/// .map(|()| rng.sample(Alphanumeric))
...
There was a problem hiding this comment.
I see the difference 😄. I was looking for a doctest for .gen::char(). I would like to keep the test for .sample(Alphanumeric), because it checks whether the characters are actually alphanumeric.
There was a problem hiding this comment.
Yes, the new one you added does, but the one above only makes a String, which seems redundant.
There was a problem hiding this comment.
Yes, the one in lib.rs is redundant as a test. But it is still useful as documentation, isn't it?
There was a problem hiding this comment.
Yes, the one in lib.rs is redundant with test_chars which is pointless now you added test_alphanumeric. I'm saying delete test_chars.
There was a problem hiding this comment.
But test_chars is testing the Uniform distributions for chars, not the Alphanumeric one like test_alphanumeric and the doc-test?
There was a problem hiding this comment.
Oh, you changed it. I guess I should read not just rely on memory. 👍
| match JitterRng::new() { | ||
| Ok(ref mut rng) => { | ||
| // false positives are possible, but extremely unlikely | ||
| assert!(rng.next_u32() | rng.next_u32() != 0); |
There was a problem hiding this comment.
Testing that two single next_u32 samples are each not zero (to test the "half used" code) is probably about the only useful test to do here. Don't worry about 1 failure in 2^31 test runs; CI fails far far more often than that due to infrastructure / incompatible updates.
The other != 0 tests seem pointless to me.
There was a problem hiding this comment.
I mean assert!(rng.next_u32() != 0) twice to check both are not 0. Edit: sorry, XOR does the same thing 👍
|
Ready to merge? |
|
You didn't reply to my other comment, about the JitterRng tests. |
|
O, I removed all except the one just above your comment. |
I have been playing around with
kcovto measure code coverage. I realize that good testing is much more than code coverage, but it showed some places that could use some easy better tests.charUniformandAlphanumericdistributions.Range::new_inclusiveandRange::sample_single.JitterRng(to catch integer overflow etc.)Finally there was one change I was missing. In dhardy#71 (comment) you made
usizesupport for 16-bit better.B.t.w. our code coverage is now 92.9%. The remaining percentages are not really testable, they are the error handling in
OsRng,JitterRng,EntropyRng,ReseedingRng, and inerror.rs.