Hi there,

I am just on the course of writing a matrix-base plugin to replace our hand-crafted library and stumbled over the following issue. It is a condensed example illustrating my problem. Basically the last call does for some reason not work under VS.net (Version 9.0.30729.1 SP, Eigen SVN Rev. 947494). Actually, it gets stuck in an endless loop.

Maybe the return-type of getRow(...) is wrong? I tried a few but was not able to find the correct one....

Any help would be appreciated.

Regards,
Hauke

Code: Select all

#include

template
const Eigen::Transpose >
getRow(const Eigen::MatrixBase& m, int row, int col = 0, int col_count = std::numeric_limits::max())
{
   typedef Eigen::Matrix RowType;
   const int ncols = std::min(m.cols()-col, col_count);
   RowType row_type(m.block(row, col, 1, ncols));
   return row_type.transpose();
}

template
Eigen::Matrix
getRowUgly(const Eigen::MatrixBase& m, int row, int col = 0, int col_count = std::numeric_limits::max())
{
   typedef Eigen::Matrix RowType;
   const int ncols = std::min(m.cols()-col, col_count);
   RowType row_type(m.block(row, col, 1, ncols));
   return row_type.transpose();
}

int main(int argc, char* argb[])
{
   typedef double Scalar;
   typedef Eigen::Matrix Matrix3x3;

   Matrix3x3 m; m << 1,2,3,4,5,6,7,8,9;

   // ok, this one works but it's ugly...
   std::cout << getRowUgly(m, 0, 0, 3) << std::endl;

   // does not work
   std::cout << getRow(m, 0, 0, 3) << std::endl;
}



hi,

the problem is not an infinite loop but the fact you return a temporary object by reference (this is because Transpose store its argument by reference). This could be solved by using a NestByValue wrapper, but then you would have two copy of the data.

So, first of all, do you really need such a getRow function ? You can achieve the same, and without any temporary, via:

Code: Select all

getRow(m,r)      m.row(r)
getRow(m,r,c)    m.row(r).end(m.cols()-c)
getRow(m,r,c,col_count)  m.row(r).segment(c,col_count)

Now if you really want to copy your row into a temporary (to enforce sequential and aligned storage) you can add .eval():

Code: Select all

m.row(r).segment(c,col_count).eval()

and finally, if you really need an extra getRow function, then, for efficiency purpose, I suggest to have the following 3 overloads:

Code: Select all

template
Eigen::Matrix::ColsAtCompileTime, 1>
getRow(const Eigen::MatrixBase& m, int row)
{
  return Eigen::Block::ColsAtCompileTime>
         (m.derived(),row,0,1,m.cols()).transpose();
}

template
Eigen::Matrix
getRow(const Eigen::MatrixBase& m, int row, int col)
{
  return m.block(row, col, 1, m.cols()-col).transpose();
}

template
Eigen::Matrix
getRow(const Eigen::MatrixBase& m, int row, int col, int col_count)
{
  return m.block(row, col, 1, col_count).transpose();
}


Hi again,

first, thanks for your quick answer.

ggael wrote:the problem is not an infinite loop but the fact you return a temporary object by reference (this is because Transpose store its argument by reference).

Just to get it right - i.e. in my plugin-code

Code: Select all

this->block(row, col, 1, ncols).transpose();

the first part before the transpose would create a temporary and transpose would return an object containing this temporary by reference? I assumed the operations (block extraction and transposition) would be nested - containing a reference to *this - my fault.

ggael wrote:So, first of all, do you really need such a getRow function ?

Since I am trying to replace our hand-crafted matrix and vector libraries, I need to stick to the interface and thus I am extremely happy about your EIGEN_MATRIXBASE_PLUGIN define....

ggael wrote:You can achieve the same, and without any temporary, via:

Code: Select all

getRow(m,r)      m.row(r)
getRow(m,r,c)    m.row(r).end(m.cols()-c)
getRow(m,r,c,col_count)  m.row(r).segment(c,col_count)

Right, but I cannot assign [font=Courier]m.row(_r_)[/font] to a mx1 vector - unfortunately all our vectors used to be column-vectors and we need to stick to it and thus I need the transposition. Well, alternatively I could write a constructor that allows to assign column-vectors and row-vectors.

Finally for the corresponding getCol, do I need return type specializations as well, or is the following fine? Btw, this is the actual code injected into my MatrixBase...

Code: Select all

const typename BlockReturnType::Type
getCol(int col) const
{
   return this->block(0, col, this->rows(), 1);
}

const typename BlockReturnType::Type
getCol(int col, int row) const
{
   return this->block(row, col, this->rows()-row, 1);
}

const typename BlockReturnType::Type
getCol(int col, int row, int row_count) const
{
   const int nrows = std::min(this->rows()-row, row_count);
   return this->block(row, col, nrows, 1);
}


- Hauke

Hauke wrote:Hi again,

first, thanks for your quick answer.

ggael wrote:the problem is not an infinite loop but the fact you return a temporary object by reference (this is because Transpose store its argument by reference).

Just to get it right - i.e. in my plugin-code

Code: Select all

this->block(row, col, 1, ncols).transpose();

the first part before the transpose would create a temporary and transpose would return an object containing this temporary by reference? I assumed the operations (block extraction and transposition) would be nested - containing a reference to *this - my fault.

no no, Eigen always returns expressions. So when you write:

Code: Select all

m.block(whatever).transpose()

no temporary is created. You can even do:

Code: Select all

m.block(whatever).transpose() = ;

The problem of your initial getRow() function is that you explicitly created temporary objects via the Matrix class.

Also, note that you can assign a row vector to column vector without explicit transposition. What is forbidden, however, is, for instance, to add a column vector to a row vector.

So, if you want getRow returns a column vector without creating a temporary, you should return something like:

Code: Select all

Transpose > >

and the body would look like:

Code: Select all

return this->block().nestByValue().transpose();

The nest-by-value thing is needed because the Block object have to be stored by value by the Transpose object.

Thank you ggael, the nestByValue() did the trick.

I introduced the bug with the temporary when I created the stripped code for the initial post - in the end I was actually looking for nestByValue().

Great - thanks again.

- Hauke