Bugzilla – Bug 748

array_5 test fails for seed 1392781168

Last modified: 2014-02-24 15:15:27 UTC

I compiled the test with gcc4.8 on Ubuntu with SSE enabled: ./array_5 s1392781168 Initializing random number generator with seed 1392781168 Repeating each test 10 times Test array_real(ArrayXXf(internal::random<int>(1,320), internal::random<int>(1,320))) failed in /usr/local/google/home/bsteiner/EigenLatest/eigen/test/array.cpp (199) test_isApprox(m1.abs().log(), log(abs(m1))) Stack: - array_real(ArrayXXf(internal::random<int>(1,320), internal::random<int>(1,320))) - array - Seed: 1392781168 Aborted (core dumped)

It appears that sometimes m1 contains a 0.0 and our isApprox can't compare infinities. We could change isApprox in MathFunctions.h, replacing return abs(x - y) <= (min)(abs(x), abs(y)) * prec; by return x == y || abs(x - y) <= (min)(abs(x), abs(y)) * prec; Or we could avoid zeros in this test (and test zeros separately).

I usually use abs(x - y) <= (abs(x) + abs(y)) * prec * 0.5 to compare floating point numbers. Compared to the existing isApprox() code, this effectively adds about 0.5*prec^2 to the tolerance though.

(In reply to comment #2) > I usually use abs(x - y) <= (abs(x) + abs(y)) * prec * 0.5 to compare floating > point numbers. Compared to the existing isApprox() code, this effectively adds > about 0.5*prec^2 to the tolerance though. This will not work either if you want to compare inf with inf, since (inf-inf) is NaN and (NaN <= x) is false (independently of x). I think comparing for equality is the simplest way to include infinities in the isApprox test. An alternative would be something like: Scalar tol = min(abs(x), abs(y))*prec; // or (abs(x)+abs(y))*prec/2 ... return x+tol >= y && y+tol >= x; Another alternative: return ! (abs(x-y) > tolerance); // Bad if x or y is NaN ... I think another flaw with our isApprox is that comparing almost certainly fails if both operands are very close to zero. But to solve that the tolerance must somehow be derived from the source operands not the result elements.

It does make some sense for isApprox to consider infinities as approximately equal since they are considered equal. However, I still have some doubt about this; isApprox is documented to do |x - y| <= prec * min(|x|, |y|) which does kind of imply that infinities are not approximately equal, so changing this arguably is a change in behaviour. In some cases, having an infinity might hide an error and we do not want to compare them equal. How about making sure that we do not take a logarithm of zero by changing the test to something like: VERIFY_IS_APPROX((m1.abs() + small).log() , log(abs(m1)+small)); where small is a small positive number like dummy_precision?

(In reply to comment #4) > isApprox is documented to do |x - y| <= prec * min(|x|, |y|) which does > kind of imply that infinities are not approximately equal, I agree, if it is documented that way, we should not change it. > How about making sure that we do not take a logarithm of zero Yes, that was basically my alternative suggestion.

(In reply to comment #5) > > How about making sure that we do not take a logarithm of zero > > Yes, that was basically my alternative suggestion. Indeed you did. I must have skipped that. I committed the change, which should fix the issue: changeset: ee16e4827333 parent: 3a9de8d7cca2 date: Mon Feb 24 14:10:17 2014 +0000 summary: Fix Bug 748 - array_5 test fails for seed 1392781168. changeset: 5e654c5a0460 branch: 3.2 parent: 95b52681a2b4 date: Mon Feb 24 14:10:17 2014 +0000 summary: Fix Bug 748 - array_5 test fails for seed 1392781168.