New user self-registration is disabled due to spam. Please email eigen-core-team @ lists.tuxfamily.org if you need an account.
Before reporting a bug, please make sure that your Eigen version is up-to-date!
Bug 205 - eigen2/eigen2_adjoint_5 test fails (eigen2support)
Summary: eigen2/eigen2_adjoint_5 test fails (eigen2support)
Status: RESOLVED FIXED
Alias: None
Product: Eigen
Classification: Unclassified
Component: Core - general (show other bugs)
Version: unspecified
Hardware: x86 - 64-bit Linux
: --- Unknown
Assignee: Nobody
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 3.0
  Show dependency treegraph
 
Reported: 2011-02-28 15:21 UTC by Benoit Jacob
Modified: 2011-03-02 23:03 UTC (History)
3 users (show)



Attachments

Description Benoit Jacob 2011-02-28 15:21:09 UTC
Test adjoint(MatrixXi(8, 12)) failed in "/home/bjacob/eigen/test/eigen2/eigen2_adjoint.cpp" (71)
    test_ei_isApprox(ei_abs(v1.eigen2_dot(v1)), v1.squaredNorm())

seems to fail here for all seeds.

note this is is a eigen2support test. You need to enable it with

  cmake -DEIGEN_TEST_EIGEN2=ON
Comment 1 Jitse Niesen 2011-03-01 10:31:28 UTC
It's quite easy to see what the problem is, but I don't know what the best way is to resolve it. The test is 

   test_ei_isApprox(ei_abs(v1.eigen2_dot(v1)), v1.squaredNorm())

where v1 is a random vector of integers. Now that we have extended the range of random integers returned by VectorXi::Random(), this dot product and the squared norm overflows, ending up as a negative number. Thus, the lhs is positive (because of the abs) and the rhs is negative.

To resolve this failure, we may either replace the ei_abs with ei_real (as is done in the Eigen3 test which is why we didn't notice before). However, it's probably better to make the entries in v1 small enough that there is no overflow when computing its squared norm. Any opinions? 

Another issue is: Should Eigen in v2 compatibility mode use the new or the old convention for Random()? The tests are run with EIGEN2_SUPPORT_STAGE10_FULL_EIGEN2_API so I think there is an argument for restricting random integers between -10 and 10 (or whatever the old convention was).
Comment 2 Benoit Jacob 2011-03-01 16:12:45 UTC
Thanks for the analysis.

(In reply to comment #1)
> It's quite easy to see what the problem is, but I don't know what the best way
> is to resolve it. The test is 
> 
>    test_ei_isApprox(ei_abs(v1.eigen2_dot(v1)), v1.squaredNorm())
> 
> where v1 is a random vector of integers. Now that we have extended the range of
> random integers returned by VectorXi::Random(), this dot product and the
> squared norm overflows, ending up as a negative number. Thus, the lhs is
> positive (because of the abs) and the rhs is negative.
> 
> To resolve this failure, we may either replace the ei_abs with ei_real (as is
> done in the Eigen3 test which is why we didn't notice before).

Yes, this is the right fix. Please go ahead and apply it.

> However, it's
> probably better to make the entries in v1 small enough that there is no
> overflow when computing its squared norm. Any opinions?

No, I really prefer keeping arbitrary random integers and using ei_real, because of the following reason. One must shed the prejudice that integer types represent mathematical integers as in 'Z'. Instead, they represent integers-modulo-2^N as in Z/(2^N)Z. In the ring Z/(2^N)Z, the notion of absolute value doesn't make any sense, but the notion of dot product and squared norm in modules over Z/(2^N)Z can still be defined in the usual way and obviously x.dot(x)==x.squaredNorm().

> Another issue is: Should Eigen in v2 compatibility mode use the new or the old
> convention for Random()? The tests are run with
> EIGEN2_SUPPORT_STAGE10_FULL_EIGEN2_API so I think there is an argument for
> restricting random integers between -10 and 10 (or whatever the old convention
> was).

Since tests happen to pass (I had fixed them except for the present one, apparently) with generalized random numbers, is there any value in restricting them? Even if Eigen2's random() returned integers between -10 and 10, users may use the whole integer range already with Eigen2, so it's better to h
ave our tests cover that (given that it's already working anyway).
Comment 3 Jitse Niesen 2011-03-02 23:03:41 UTC
(In reply to comment #2)
> > To resolve this failure, we may either replace the ei_abs with ei_real (as is
> > done in the Eigen3 test which is why we didn't notice before).
> 
> Yes, this is the right fix. Please go ahead and apply it.

Applied as changeset 2b5e76499896.

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