Re: [eigen] On the implementation of STL iterator for Eigen::Matrix

[Gael Guennebaud <gael.guennebaud@xxxxxxxxx>]

On Tue, Oct 2, 2018 at 11:26 PM Patrik Huber <patrikhuber@xxxxxxxxx> wrote:

[...] So in that sense you could maybe say none of the solutions is really great, as an API should be as intuitive as possible. But we all know in practice that's not always possible, so I think both allRows() and rowwise() would do the job perfectly fine, and I'd say allRows() is slightly more clear in my opinion.

 

thank you for sharing your opinion.

 

>> for(auto x : A.reshaped()) {...}

I also don't find this too clear. Does it reshape col-wise or row-wise? I'd say this would depend on the storage order you choose, and since when not specified, Eigen defaults to col-major storage order, I'd say this must reshape col-wise. Anyway it's probably fine because in Matlab or NumPy, you also don't know what their reshape() functions do, unless you check the documentation or already know the storage order they use.

yes, that's definitely something you have to check in the doc, I don't see any way to make it cristal clear from the name unless you name it:

A.reshaped_column_major_to_1D_column()

;)

 

Does that line incur a copy of A btw to reshape it, or does it work like Eigen::Map and provide a view on the original data?

It's only a view, just like everywhere else in Eigen.

 

Maybe this one's a silly question: I suppose looping over the rows/cols will work with "const auto&" too, so there are no copies?

no need for a const ref:

for(auto c : A.allCols()) {}

already does the job fine because it is equivalent to:

for(MatrixXd::ColXpr c : A.allCols()) {}

and ColXpr (aka a Block<>) is already a light-weight reference to the column of A.

gael

On Tue, 2 Oct 2018 at 13:54, Gael Guennebaud <gael.guennebaud@xxxxxxxxx> wrote:

Hi list,

we're eventually implementing STL iterators to iterator over the coefficients of a vector/matrix as well as over the columns or rows of a matrix, and your inputs might be welcome to help converging to a stable API, see below.

You can watch the WIP in PR 519 [1], and find some background in bug 231 [2]. Basically, to avoid ambiguities, we decided that iterators over coefficients would be defined for 1D _expression_ only, meaning that to iterate over the elements of a 2D Matrix you'll have to explicitly reshape it as a 1D vector first. Some typical examples:

VectorXd v;

MatrixXd A;

for(auto x : v) {...}

for(auto x : A.col(j)) {...}

for(auto x : A.row(i)) {...}

for(auto x : A.reshaped()) {...}

The last line iterate in column-major order regardless of the storage order of A. So far so good. Things get more tricky now.

To iterate over rows or columns, we need to define a proxy telling so. One option is to reuse (abuse?) rowwise()/colwise():

for(auto x : A.rowwise()) { ... }

Is it obvious to everyone that this line is iterating over the rows of A and is thus a synonym for:

  for(int i=0;i<A.rows();++i) { auto x = A.row(i); ... }

??

On my side, I'm rather reading it as "iterate over all elements of A in row-major order", which is unfortunate as if I'm not alone with such an interpretation, then we need to come up with new names. Since rows()/cols() are already taken, we could think of:

for(auto x : A.allRows()) { ... }

for(auto x : A.rowSet()) { ... }

?????

 

Feel free to share your opinion on that question as well as on the other minor issues discussed in the PR.

thanks,

gael

[1] https://bitbucket.org/eigen/eigen/pull-requests/519/

[2] http://eigen.tuxfamily.org/bz/show_bug.cgi?id=231