Implement the MatrixView class #734

GiulioRomualdi · 2020-09-09T16:26:18Z

This PR has to be the updated version of #730. Differently from #730 the storage ordering is now a regular attribute of the class, rather than a template parameter. The class now behaves like an std::span, i.e. when the copy assignment operator is called the pointer, and not what it's contained, is copied.

I've also implemented make_matrix_view to simplify the creation of a MatrixView. The usual toEigen() functions have been implemented to convert the MatrixView<> to an EigenMap, in this case, the StorageOrder is handled using the Eigen::Stride (thanks @S-Dafarra for suggesting this).

The PR is correlated to UnitTests.

This is the first step to achieve #716

@traversaro @prashanthr05 @S-Dafarra

This is a preliminary working implementation of kindyn with MatrixView.

…e with make_matrix_view method

src/core/include/iDynTree/Core/EigenHelpers.h

src/core/include/iDynTree/Core/MatrixView.h

src/core/tests/MatrixViewUnitTest.cpp

…lisions

…w class

…w object is passed to a toEigen() method

traversaro

Super minor comment, other then that it is great. The stride trick is far from being intuitive, so if you have some link to explain it would be great to add it in a comment in the code, thanks!
Other then that, can you update the CHANGELOG?

src/core/include/iDynTree/Core/EigenHelpers.h

S-Dafarra · 2020-09-10T07:40:11Z

Let me drop here an idea. It would be nice to add (not necessarily in this PR) a block call. I think that this would be possible by playing with the stride (which should become a class parameter as well). We can discuss this F2F, or I can also try to implement it if you want.

GiulioRomualdi · 2020-09-10T08:16:29Z

Let me drop here an idea. It would be nice to add (not necessarily in this PR) a block call. I think that this would be possible by playing with the stride (which should become a class parameter as well). We can discuss this F2F, or I can also try to implement it if you want.

For the time being we can use toEigen().block(), but yes we can discuss 😄

Co-authored-by: Silvio Traversaro <pegua1@gmail.com>

S-Dafarra · 2020-09-10T08:19:45Z

For the time being we can use toEigen().block(), but yes we can discuss

Yes, but that is not returning a MatrixView. Also, notice that the pointer returned by data() of block is const.

…n iDynTree::MatrixView

prashanthr05 · 2020-09-10T08:38:33Z

src/core/include/iDynTree/Core/EigenHelpers.h

+
+    // This is a trick required to see a ColMajor matrix as a RowMajor matrix.
+    const int innerStride = (mat.storageOrder() == StorageOrder::ColMajor) ? mat.rows() : 1;
+    const int outerStride = (mat.storageOrder() == StorageOrder::ColMajor) ? 1 : mat.cols();


I read here that it is not always safe to use ternary operators with Eigen expressions.

Ah, my bad, here mat is a MatrixView type!

I think that it is not safe when an Eigen type is on the left side. In this case should be fine as only int are involved.

Ah okay, understood.

Here the operator is called on int so it shouldn't be a problem, please correct me if I'm mistaken.

I think that it is not safe when an Eigen type is on the left side. In this case should be fine as only int are involved.

Ops I missed the message

GiulioRomualdi · 2020-09-10T08:41:33Z

@traversaro

Super minor comment, other then that it is great. The stride trick is far from being intuitive, so if you have some link to explain it would be great to add it in a comment in the code, thanks!

Done here: 7bcf16f

Other then that, can you update the CHANGELOG?

Here: f07abf2

traversaro · 2020-09-10T09:38:26Z

Thanks! Can I merge with squash?

GiulioRomualdi · 2020-09-10T09:53:30Z

Thanks! Can I merge with squash?

Yes sure

GiulioRomualdi added 9 commits September 9, 2020 17:58

Implement MatrixView class

28306d7

Add MatrixView to the set of the headers in the core CMakeLists file

abd7f9d

Implement MatrixDynSize assignement operator for MatrixView objects

de39dbd

Add typedef in MatrixDynSize and MatrixFixSize to make them compatibl…

17ae047

…e with make_matrix_view method

Extend the MatrixDynSize test with the MatrixView class

e4b9ace

Implement the toEigen method for the MatrixView class

9038b94

Extend the EigenHelpersUnitTest with tests related to MatrixView

2813702

Implement unit tests for MatrixView class

abc9e97

Enable the compilation of the MatrixViewUnitTest

44694c9

GiulioRomualdi requested review from traversaro and S-Dafarra September 9, 2020 16:26

GiulioRomualdi self-assigned this Sep 9, 2020

GiulioRomualdi mentioned this pull request Sep 9, 2020

[OUTDATED] Implement the MatrixView class #730

Closed

S-Dafarra reviewed Sep 9, 2020

View reviewed changes

GiulioRomualdi added 4 commits September 9, 2020 18:56

Fix typos in EigenHelpers.h

c173e11

Introduce the MatrixViewInternal namespace to avoid possible name col…

d766d55

…lisions

Use std::ptrdiff_t instead of unsigned int as index type in MatrixVie…

67f2589

…w class

Add a test for checking the possible errors when a ColMajor MatrixVie…

c4fe388

…w object is passed to a toEigen() method

GiulioRomualdi force-pushed the feature/add_matrix_view branch from ba6bc38 to c4fe388 Compare September 9, 2020 17:45

S-Dafarra approved these changes Sep 9, 2020

View reviewed changes

traversaro approved these changes Sep 10, 2020

View reviewed changes

src/core/include/iDynTree/Core/EigenHelpers.h Outdated Show resolved Hide resolved

Fix style in EigenHelpers.h file

ab3af17

Co-authored-by: Silvio Traversaro <pegua1@gmail.com>

Add a comment describing the trick used to build an Eigen::Map from a…

7bcf16f

…n iDynTree::MatrixView

prashanthr05 reviewed Sep 10, 2020

View reviewed changes

Update CHANGELOG.md

f07abf2

Merge branch 'devel' into feature/add_matrix_view

5ebd210

traversaro merged commit 5b2b487 into robotology:devel Sep 10, 2020

GiulioRomualdi deleted the feature/add_matrix_view branch September 10, 2020 10:21

GiulioRomualdi mentioned this pull request Sep 16, 2020

Decided how to handle different storing order in the repo ami-iit/bipedal-locomotion-framework#100

Closed

traversaro mentioned this pull request Dec 16, 2021

Implement externally owned vector and matrices #430

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement the MatrixView class #734

Implement the MatrixView class #734

GiulioRomualdi commented Sep 9, 2020 •

edited

Loading

traversaro left a comment

S-Dafarra commented Sep 10, 2020

GiulioRomualdi commented Sep 10, 2020

S-Dafarra commented Sep 10, 2020

prashanthr05 Sep 10, 2020 •

edited

Loading

prashanthr05 Sep 10, 2020 •

edited

Loading

S-Dafarra Sep 10, 2020

prashanthr05 Sep 10, 2020

GiulioRomualdi Sep 10, 2020 •

edited

Loading

GiulioRomualdi Sep 10, 2020

GiulioRomualdi commented Sep 10, 2020 •

edited

Loading

traversaro commented Sep 10, 2020

GiulioRomualdi commented Sep 10, 2020

Implement the MatrixView class #734

Implement the MatrixView class #734

Conversation

GiulioRomualdi commented Sep 9, 2020 • edited Loading

traversaro left a comment

Choose a reason for hiding this comment

S-Dafarra commented Sep 10, 2020

GiulioRomualdi commented Sep 10, 2020

S-Dafarra commented Sep 10, 2020

prashanthr05 Sep 10, 2020 • edited Loading

Choose a reason for hiding this comment

prashanthr05 Sep 10, 2020 • edited Loading

Choose a reason for hiding this comment

S-Dafarra Sep 10, 2020

Choose a reason for hiding this comment

prashanthr05 Sep 10, 2020

Choose a reason for hiding this comment

GiulioRomualdi Sep 10, 2020 • edited Loading

Choose a reason for hiding this comment

GiulioRomualdi Sep 10, 2020

Choose a reason for hiding this comment

GiulioRomualdi commented Sep 10, 2020 • edited Loading

traversaro commented Sep 10, 2020

GiulioRomualdi commented Sep 10, 2020

GiulioRomualdi commented Sep 9, 2020 •

edited

Loading

prashanthr05 Sep 10, 2020 •

edited

Loading

prashanthr05 Sep 10, 2020 •

edited

Loading

GiulioRomualdi Sep 10, 2020 •

edited

Loading

GiulioRomualdi commented Sep 10, 2020 •

edited

Loading