math/rand: tests are too weak due to misuse of statsResults.maxError

[josharian]

The math/rand tests use the function nearEqual to compare floats:

func nearEqual(a, b, closeEnough, maxError float64) bool {
	absDiff := math.Abs(a - b)
	if absDiff < closeEnough { // Necessary when one value is zero and one value is close to zero.
		return true
	}
	return absDiff/max(math.Abs(a), math.Abs(b)) < maxError
}

A bit of thought suggests that maxError should always be pretty small, if this function is ever to return false. However, maxError is usually scaled everywhere it is used (e.g. stddev * 0.08), and in practice it frequently ends up >> 2.

As a result, many of the math/rand tests have no teeth. An extreme example of this is TestReadUniformity, which requires (among other things) that the mean of just 2 randomly selected bytes must be very near 255.0/2, a test which should almost always fail, but which currently succeeds, due to a large value of maxError. (Related: CL 51310.)

Replacing maxError with a constant 0.08 causes most math/rand tests to pass, except TestReadUniformity when called with small n (in which case it should fail).

However, I don't have enough domain expertise to know whether this is reasonable or whether an alternative bound would be better. Advice requested.

golang.org/s/owners lists no one for math/rand, so I'll guess and cc: @rsc @aclements @robpike

[gopherbot]

Change https://golang.org/cl/51891 mentions this issue: math/rand: add Shuffle

[josharian]

This is even worse than I thought. The existing code reads:

func (this *statsResults) checkSimilarDistribution(expected *statsResults) error {
	if !nearEqual(this.mean, expected.mean, expected.closeEnough, expected.maxError) {
		s := fmt.Sprintf("mean %v != %v (allowed error %v, %v)", this.mean, expected.mean, expected.closeEnough, expected.maxError)
		fmt.Println(s)
		return errors.New(s)
	}
	if !nearEqual(this.stddev, expected.stddev, 0, expected.maxError) {
		s := fmt.Sprintf("stddev %v != %v (allowed error %v, %v)", this.stddev, expected.stddev, expected.closeEnough, expected.maxError)
		fmt.Println(s)
		return errors.New(s)
	}
	return nil
}

Note that when checking stddev, closeEnough is set to 0 in the call to nearEqual. This means that all the work is being done by expected.maxError, which is generally too large.

Also, there should probably be separate values for closeEnough and maxError for mean and stddev, given that they are generally of very different scales.

[gopherbot]

Change https://golang.org/cl/55972 mentions this issue: math/rand: make Perm match Shuffle