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

Hi Gael,

It's fantastic to see this being added to Eigen, thanks a lot for all the work.

>> 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 [...]

My first thought was that this would probably "take rows from A, one at a time" and then loop over the columns of each row, but somehow that didn't make too much sense. After thinking about it for a bit, I can also definitely read it as iterating over the rows of A and I think that makes more sense actually. But after reading the PR and bug 231 (and after my own confusion), I definitely see that there's ambiguity.

I also didn't find allRows() or rowSet() more clear at first. Looking at it for a bit longer and thinking about it, I find allRows() quite clear though.

In all the cases I'd probably have to check the documentation to make sure that the code I am writing is really doing what I want to do. 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.

>> 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.

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?

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?

Hope that's somehow helpful feedback.

Cheers,

Patrik

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