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 1018 - blueNorm handles complex infinities wrong on some systems
Summary: blueNorm handles complex infinities wrong on some systems
Status: CONFIRMED
Alias: None
Product: Eigen
Classification: Unclassified
Component: Core - general (show other bugs)
Version: 3.3 (current stable)
Hardware: All All
: Normal Wrong Result
Assignee: Nobody
URL:
Whiteboard:
Keywords:
Depends on: 1019
Blocks:
  Show dependency treegraph
 
Reported: 2015-05-20 12:53 UTC by Christoph Hertzberg
Modified: 2015-06-05 13:03 UTC (History)
3 users (show)



Attachments

Description Christoph Hertzberg 2015-05-20 12:53:53 UTC
This results in the stable_norm_5 unit test to fail on several systems (mostly clang, but I experienced it on g++4.9-cygwin, as well)

The problem is that sometimes abs for complex is implemented similar to this:

  template<typename _Tp>
    inline _Tp
    __complex_abs(const complex<_Tp>& __z)
    {
      _Tp __x = __z.real();
      _Tp __y = __z.imag();
      const _Tp __s = std::max(abs(__x), abs(__y));
      if (__s == _Tp())  // well ...
        return __s;
      __x /= __s; 
      __y /= __s;
      return __s * sqrt(__x * __x + __y * __y);
    }

This causes a division of Inf/Inf which results in NaN instead of Inf (which is a bug not caused by us). Nevertheless, even for working abs implementations, we could (actually for all *Norm implementations) increase stability and improve performance by computing the norm of a real-valued vector of twice the size (at least for Euclidean norm).

Related to that: Some of our decompositions (e.g. eigenvalue/svd) normalize the input matrix by dividing by .cwiseAbs().maxCoeff(). IMO it would make sense here to take use the max of (real().cwiseAbs(), imag().cwiseAbs()) here as well (no, need to compute any complex abs)
Comment 1 Gael Guennebaud 2015-06-05 13:03:56 UTC
Agree with both proposals.

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