| Re: [eigen] On the implementation of STL iterator for Eigen::Matrix |
[ Thread Index |
Date Index
| More lists.tuxfamily.org/eigen Archives
]
On 2018-10-02 15:35, Patrik Huber wrote:
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.
Overall, I'd say the main problem is to have appropriate documentation
then. When introducing new members `allRows()`, I don't see why
A.allRows().norm(); A.allRows().reverseInPlace(); // etc
should not work as well. And to be honest, the meaning of
A.colwise().replicate(2) I find a bit arbitrary, too (without checking
the documentation). It could also mean:
[A.col(0), A.col(0), A.col(1), A.col(1), ...]
At the moment, the single purpose of allRows()/allCols() would be using
it with .begin() and .end(), even though they have very similar
functionality to rowwise()/colwise().
But if adding new functions is consensus, I'm totally ok with that.
for(auto x : A.reshaped()) {...}
I also don't find this too clear. Does it reshape col-wise or row-wise? [...]
Yes, that is one topic we are currently debating about ...
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?
Usually, Eigen will never produce copies until you explicitly assign
expressions to matrices (Some exceptions: Using .eval(), or products
which either could alias or involve more than two factors)
A.reshaped() will actually be writable as long as A is writable.
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?
Yes, and without the const this should also be writable (did not try it yet)
Hope that's somehow helpful feedback.
Thanks for the feedback!
Christoph
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
--
Dr.-Ing. Christoph Hertzberg
Besuchsadresse der Nebengeschäftsstelle:
DFKI GmbH
Robotics Innovation Center
Robert-Hooke-Straße 5
28359 Bremen, Germany
Postadresse der Hauptgeschäftsstelle Standort Bremen:
DFKI GmbH
Robotics Innovation Center
Robert-Hooke-Straße 1
28359 Bremen, Germany
Tel.: +49 421 178 45-4021
Zentrale: +49 421 178 45-0
E-Mail: christoph.hertzberg@xxxxxxx
Weitere Informationen: http://www.dfki.de/robotik
-----------------------------------------------------------------------
Deutsches Forschungszentrum fuer Kuenstliche Intelligenz GmbH
Firmensitz: Trippstadter Straße 122, D-67663 Kaiserslautern
Geschaeftsfuehrung: Prof. Dr. Dr. h.c. mult. Wolfgang Wahlster
(Vorsitzender) Dr. Walter Olthoff
Vorsitzender des Aufsichtsrats: Prof. Dr. h.c. Hans A. Aukes
Amtsgericht Kaiserslautern, HRB 2313
Sitz der Gesellschaft: Kaiserslautern (HRB 2313)
USt-Id.Nr.: DE 148646973
Steuernummer: 19/672/50006
-----------------------------------------------------------------------