Bug 314 - Infinite recursion when doing exp(matrix)
Infinite recursion when doing exp(matrix)
Status: RESOLVED FIXED
Product: Eigen
Classification: Unclassified
Component: Core - general
3.0
All All
: --- Unknown
Assigned To: Nobody
:
: 508 523 (view as bug list)
Depends on:
Blocks: 3.2
  Show dependency treegraph
 
Reported: 2011-07-06 11:40 UTC by Gael Guennebaud
Modified: 2013-06-12 23:44 UTC (History)
5 users (show)



Attachments
proof of concept patch removing most of the internal::* standard math functions (169.78 KB, patch)
2012-10-09 13:51 UTC, Gael Guennebaud
no flags Details | Diff
Move math funcs from internal to math (169.16 KB, application/octet-stream)
2013-06-10 23:43 UTC, Gael Guennebaud
no flags Details

Description Gael Guennebaud 2011-07-06 11:40:44 UTC
Here is a simple example to reproduce:

#include <Eigen/Core>
#include <iostream>
int main() {
  Eigen::MatrixXf a(4,4), b;
  b = exp(a);
}

The problem is in the file MathFunctions.h, lines 469-488 where exp calls exp_default_impl::exp which itself calls exp...

The fix is probably to add specializations for MatrixBase objects.
Comment 1 Jitse Niesen 2011-09-11 10:42:07 UTC
I had a look but there is too much metaprogramming in MathFunctions for me :(

I can understand how the infinite recursion happens. However, if you replace Matrix by SparseMatrix then the compiler gives a fairly clean error message. I can't see why that case should be different.

In addition, I don't see what all that metaprogramming is used for, even after reading the comment at the top of MathFunctions. Why can't we use overloading? That is, define a function 

namespace Eigen
{
  template<typename Derived>					
  inline const CwiseUnaryOp<internal::scalar_sin_op<typename Derived::Scalar>, const Derived> 
  sin(const ArrayBase<Derived>& x) {	
    return x.derived();
  }
} 

If the user writes an unqualified function call, e.g. sin(array) or sin(array1+array2), then Koenig lookup will find this. If we also want to give the user the possibility to do std::sin(array) - I wouldn't know why but we seem to have made that decision - then we can put a similar function in the std namespace.
Comment 2 Gael Guennebaud 2012-10-09 13:49:26 UTC
*** Bug 508 has been marked as a duplicate of this bug. ***
Comment 3 Gael Guennebaud 2012-10-09 13:51:36 UTC
Created attachment 301 [details]
proof of concept patch removing most of the internal::* standard math functions
Comment 4 Gael Guennebaud 2012-10-09 13:56:42 UTC
As discussed on the malling list, the best solution seems to be indeed to remove this meta-programming kung fu that brings more troubles than advantages. Still need to address imag, real, and some others before pushing.

This patch also removes the overloads of the standard math functions defined in the std namespace for Eigen::ArrayBase<> objects: indeed, they leads to ambiguous calls when someone do, for instance:

template<typename T> void foo(const T& t)
{
  using std::sin;
  sin(t);
}

and T is an ArrayBase<> object.
Comment 5 Gael Guennebaud 2012-10-24 10:26:46 UTC
*** Bug 523 has been marked as a duplicate of this bug. ***
Comment 6 Gael Guennebaud 2012-11-06 15:29:33 UTC
https://bitbucket.org/eigen/eigen/changeset/ee70f8f54114/
changeset:   ee70f8f54114
user:        ggael
date:        2012-11-06 15:25:50
summary:     Fix bug 314:
- remove most of the metaprogramming kung fu in MathFunctions.h (only keep functions that differs from the std)
- remove the overloads for array expression that were in the std namespace
affected #:  88 files


The real, imag, and conj overloads are still there because the std versions only work on complex. We should probably rename them, or move them to another namespace to prevent from infinite recursion for them too.
Comment 7 Gael Guennebaud 2013-03-20 18:39:19 UTC
Another option for real, imag, and conj is to simply implement the missing overloads for int, float, double, etc. ourself in the internal namespace, thus really geting rid of all this metaprogramming kung fu.
Comment 8 Gael Guennebaud 2013-04-19 11:58:40 UTC
Let me first recall that for real, imag and conj, the current behavior is a bit different than with the other ones:

Matrix2cf a, b, c;
b = real(a);
c = conj(a);

does compile and run fine, but b and c are simply equal to a. 

So after playing a bit at fixing this issue,  I think that the best solution would be to rename these functions to, e.g., genReal, genImag, genConj where "gen" stands for generic. Better names welcome!

The reason is that our variants are not really compatible with the std library because we support calling them on real scalar values. Adding our own overloads for real types (int, float, double, etc.) would indeed fix the issue, but that will also require users to support them on their custom real types, that is not nice and will break existing code.
Comment 9 Gael Guennebaud 2013-06-10 23:43:48 UTC
Created attachment 343 [details]
Move math funcs from internal to math

Proposed patch moving the remaining math funcs (imag, real, conj, abs2, norm1, hypot, pow) from the internal namespace to math namespace.

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