Re: [eigen] vector-cwise operator +=/-=

[Gael Guennebaud <gael.guennebaud@xxxxxxxxx>]

ok these aliasing checks are now fixed (and they are even better with much less code :) )

gael

On Wed, Dec 16, 2009 at 10:51 AM, Gael Guennebaud <gael.guennebaud@xxxxxxxxx> wrote:

good catch ! explanations:

this is because in debug mode we (Eigen) detect that v3 appears on both sides of the assignment and since it is transposed on the right hand side we have an aliasing issue (because transpose is a null op).. E.g.:

v3 = v3.transpose() + X;

this cannot work without evaluating the rhs (or v3.transpose()) into a temporary.

so far so good.

The problem now is that in your example we actually take the transpose of v3 on both sides:

v3.transpose() = v3.transpose() + X;

that is fine regarding aliasing issue. So yes we should update these aliasing checks to handle such a case. I'll add a test so that we don't forget!

gael

On Wed, Dec 16, 2009 at 10:24 AM, Hauke Heibel <hauke.heibel@xxxxxxxxxxxxxx> wrote:

Geez, its complaining about 'aliasing' in lazyAssign (line 309, Transpose.h).

On Wed, Dec 16, 2009 at 10:17 AM, Hauke Heibel <hauke.heibel@xxxxxxxxxxxxxx> wrote:

And another thing - I know that it's again not the nicest use case but this code yields a run-time error complaining about slicing...

v3.transpose() += v2.transpose() * m;

Can we create a compile time error here? Or should we also try to make it work?

- Hauke

On Wed, Dec 16, 2009 at 10:14 AM, Hauke Heibel <hauke.heibel@xxxxxxxxxxxxxx> wrote:

So the transposition is really not (!) taking place - it's optimized away? Is there a general rule of thumb to infer, when and if transposition is a nullary op?

Regarding the convenience I would say 'yes, it's more convenient'. In particular in presence of my previous question - it is (*cough* at least for me) not intuitively clear to the user that the multiplication as proposed by you is equally efficient.

Finally, in the patch I am guarding against vectors of different size. I can not guarantee, that I am overseeing a use case where this might fail. I can run the unit tests against the modifications and true, I see the need for the ei_assert...

- Hauke

On Wed, Dec 16, 2009 at 10:05 AM, Gael Guennebaud <gael.guennebaud@xxxxxxxxx> wrote:

indeed we already allow col_vector = row_vector, but I'm not sure we should allow coeff-wise binary operators to work between column and row vectors... is it always safe ? is it really more convenient ?

About the last question, e.g., your example is better written like this:

v3 += m.transpose() * v2;

gael

On Wed, Dec 16, 2009 at 9:39 AM, Hauke Heibel <hauke.heibel@xxxxxxxxxxxxxx> wrote:

Hi guys,

I think it would be cool, if the following code worked:

#include <Eigen/Core>
#include <Eigen/Array>

using namespace Eigen;

void main ()
{
  Matrix<double,3,1> v3;
  Matrix<double,2,1> v2;
  Matrix<double,2,3> m;

  m << 1,2,3,4,5,6;
  v2 << 7,8;
  v3 << 4,5,6;

  v3 += v2.transpose() * m; // expected 43, 59, 57
  std::cout << v3 << std::endl;
}

The simple assignment

v3 = v2.transpose() * m;

is already working and if I am not totally wrong, the only thing which keeps this current example from working is the ctor of CwiseBinaryOp. In particular the lines

EIGEN_STATIC_ASSERT_SAME_MATRIX_SIZE(Lhs, Rhs)
ei_assert(lhs.rows() == rhs.rows() && lhs.cols() == rhs.cols());

(these are redundant, right?). I think, if we simply make this check less restrictive in the sense that we allow the assignment of vectors and their transposed counterparts (having the same size...) we would be done. Any objections or comments against such a change?

- Hauke