New user self-registration is currently disabled. Please email eigen-core-team @ lists.tuxfamily.org if you need an account.
Bug 748 - array_5 test fails for seed 1392781168
 Summary: array_5 test fails for seed 1392781168
 Status: RESOLVED FIXED Eigen Unclassified General 3.2 All All Normal Unknown Nobody Show dependency tree / graph

 Reported: 2014-02-20 20:00 UTC by Benoit Steiner 2014-02-24 15:15 UTC (History) 4 users (show) chtz gael.guennebaud jacob.benoit.1 jitseniesen

Attachments

 Benoit Steiner 2014-02-20 20:00:38 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(1,320), internal::random(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(1,320), internal::random(1,320))) - array - Seed: 1392781168 Aborted (core dumped)``` Christoph Hertzberg 2014-02-21 12:46:38 UTC ```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).``` Benoit Steiner 2014-02-21 17:47:06 UTC `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.` Christoph Hertzberg 2014-02-21 18:03:38 UTC ```(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.``` Jitse Niesen 2014-02-23 11:14:31 UTC ```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?``` Christoph Hertzberg 2014-02-24 10:37:09 UTC ```(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.``` Jitse Niesen 2014-02-24 15:15:27 UTC ```(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.```

 Note You need to log in before you can comment on or make changes to this bug.