Eigen <-> StridedArrayView integration #74
Conversation
Codecov Report
@@ Coverage Diff @@
## master #74 +/- ##
===========================================
+ Coverage 75.13% 89.47% +14.33%
===========================================
Files 21 16 -5
Lines 905 570 -335
===========================================
- Hits 680 510 -170
+ Misses 225 60 -165
Continue to review full report at Codecov.
|
| CORRADE_COMPARE(view1[i][j], view2[i][j]); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This could use an additional test case that
- uses a
flipped(),broadcasted()andtransposed()array on input to test all cases of a positive, zero and negative stride - converts to Eigen, verifies a few random picks (
a[2][3] == b[2][3]) - converts back
and then verifies that data(), size() and stride() are the same as on input. In particular I'm not sure if Eigen even supports zero strides (I hope it supports negative strides at least).
There was a problem hiding this comment.
So I researched this a bit, and it is possible to incorporate negative strides by mapping those to Eigen::Reverse expressions I think.
Unfortunately I dont think it is possible to cast broadcasted ArrayViews, since Eigen handels braodcasting differently. E.g. to add a vector to a matrix one turns the matrix into a special kind of broadcast expression but the vector is not changed in any way
Eigen::MatrixXf m;
Eigen::VectorXf v;
m.rowwise() += v;
So if you agree I would just assert on zero strides.
There was a problem hiding this comment.
Ok actually the Reverse expression are not well suited since everything needs to be known at compile time. I found that negative strides are actually supported out of the box since relatively recently : https://eigen.tuxfamily.org/bz/show_bug.cgi?id=747 so that should still work and maybe zero strides will also just work, I will look into that.
There was a problem hiding this comment.
Looking at the commit log, this seems to be done and fully supported, right? :)
| @brief Convert Corrades StridedArrayView2D to Eigens Dynamic Matrix Type. | ||
| */ | ||
| template<class T> inline Eigen::Map<Eigen::Matrix<T, Eigen::Dynamic, Eigen::Dynamic, Eigen::RowMajor>, Eigen::Unaligned, Eigen::Stride<Eigen::Dynamic, Eigen::Dynamic>> arrayCast(const Containers::StridedArrayView2D<T>& from) { | ||
| using MatrixT = Eigen::Matrix<T, Eigen::Dynamic, Eigen::Dynamic, Eigen::RowMajor>; |
There was a problem hiding this comment.
Could be the RowMajor / ColMajor configurable via an optional second template parameter, maybe? Not sure what should be the default tho.
There was a problem hiding this comment.
I think having it RowMajor is consistent with how it is used in Magnum (i.e.. view2D[0] is usually used as a row, on the other hand the default in Eigen is ColumnMajor) and one can always transpose the resulting Eigen expression to get a ColumnMajor view on the data if I am not mistaken?
There was a problem hiding this comment.
I investigated and tried to support both RowMajor / ColMajor via a template parameter only to realize it doesn't really have any effect on a Map if you explicitly specify Stride as well: https://eigen.tuxfamily.org/dox/group__TutorialMapClass.html#TutorialMapTypes A Map can be then copied to both row- and col-major Matrix so unless I'm missing something it doesn't matter at all what Matrix type is used inside the Map.
So I removed the RowMajor, flipped order of Stride arguments and it resulted in various types being significantly shorter: 9379c64
Co-authored-by: Vladimír Vondruš <mosra@centrum.cz>
avoid auto and add const Co-authored-by: Vladimír Vondruš <mosra@centrum.cz>
use CORRADE_COMPARE_AS for StridedArrayView1D Co-authored-by: Vladimír Vondruš <mosra@centrum.cz>
Janos95
changed the title
|
Finally merged as e8eceb3, thank you! As I mentioned in the comment above, 9379c64 removes the |
This patch provides an arrayCast method converting between Eigen expressions and Corrade's StridedArrayView class. Not the most beautiful code in the world but should mostly work ;).