Bug 748 - array_5 test fails for seed 1392781168
array_5 test fails for seed 1392781168
Status: RESOLVED FIXED
Product: Eigen
Classification: Unclassified
Component: General
3.2
All All
: Normal Unknown
Assigned To: Nobody
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2014-02-20 20:00 UTC by Benoit Steiner
Modified: 2014-02-24 15:15 UTC (History)
4 users (show)



Attachments

Description 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<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)
Comment 1 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).
Comment 2 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.
Comment 3 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.
Comment 4 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?
Comment 5 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.
Comment 6 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.