Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Decide on the guidelines for the type of vectors to be used when defining an interface #95

Closed
S-Dafarra opened this issue Sep 4, 2020 · 8 comments
Assignees

Comments

@S-Dafarra
Copy link
Member

This issue is part of a broader discussion initiated with #94

In particular, in case we have to develop an interface which requires a vector as input, we have to decide what to use. We have several possibilities:

  • eigen vectors, more specifically Eigen::Ref
  • GenericContainer::Vector
  • templates
  • ...

In this issue, we can share thoughts and find an agreement.

@S-Dafarra
Copy link
Member Author

S-Dafarra commented Sep 4, 2020

Right now in the code, we have two approaches:

  1. only Eigen in the header, see https://github.com/dic-iit/bipedal-locomotion-framework/blob/master/src/Planners/include/BipedalLocomotion/Planners/QuinticSpline.h
  2. the use of GenericVector plus templates. The corresponding usage results in something like https://github.com/dic-iit/bipedal-locomotion-framework/blob/master/src/ParametersHandler/tests/ParametersHandlerTest.cpp#L72-L77

For the first solution we have:

  • Pros
    • the interface is directly usable with Eigen and Eigen-compatible objects (I am thinking about manif and cppAD)
    • the input objects can be used straight away for math operations without using toEigen
  • Cons
    • Resizing an Eigen::Ref can cause issues, so it should be used only when the input size is known beforehand
    • Requires using maps when the interface has to be used with std, iDynTree or yarp vectors.

For the second solution, we have:

  • Pros
    • It can be used with any type of vector usable with Span
    • Input vectors can be resized, provided that the original vector was resizable.
  • Cons
    • It requires defining an additional templated method, or to convert the input vector to GenericContainer::Vector manually.
    • Math operations require toEigen.

If I have to decide when to use one with respect to the other, I guess that I would distinguish the following two cases:

  • Math interface
  • I/O interface

For the first, I would go for Eigen objects, hence leaving the street open to future usages of automatic differentiation tools. I am not expecting the same need on libraries which take care of reading/writing outputs from/to other "systems". I call these, I/O interfaces, which include libraries to read parameters or to interface with the robot. in this case, I would go for GenericContainer::Vector since it allows dealing with different types of vectors of unknown dimensions natively.

@GiulioRomualdi @prashanthr05 what do you think?

@prashanthr05
Copy link
Collaborator

If I have to decide when to use one with respect to the other, I guess that I would distinguish the following two cases:

  - Math interface
  - I/O interface

For the first, I would go for Eigen objects, hence leaving the street open to future usages of automatic differentiation tools. I am not expecting the same need on libraries which take care of reading/writing outputs from/to other "systems". I call these, I/O interfaces, which include libraries to read parameters or to interface with the robot. in this case, I would go for GenericContainer::Vector since it allows dealing with different types of vectors of unknown dimensions natively.

I am totally in line with this.

I have a question regarding the Eigen::Ref though.

Resizing an Eigen::Ref can cause issues, so it should be used only when the input size is known beforehand

Is there any significant distinction between Eigen::Ref<Eigen::VectorXd> and Eigen::VectorXd&. Is it ok to resize a Eigen::VectorXd& type argument passed to a function?

@S-Dafarra
Copy link
Member Author

S-Dafarra commented Sep 4, 2020

Is there any significant distinction between Eigen::Ref<Eigen::VectorXd> and Eigen::VectorXd&. Is it ok to resize a Eigen::VectorXd& type argument passed to a function?

See https://eigen.tuxfamily.org/dox/TopicFunctionTakingEigenTypes.html. More specifically, if you set as input parameter an Eigen::VectorXd& you cannot use that method with an Eigen::map for example, at least not without allocating an Eigen::VectorXd.

@GiulioRomualdi
Copy link
Member

I would like to keep the interface Eigen only as much as possibile, and avoiding to create our custom types.
In the parameter handler, the GenericVector was required to handle different kinds of vectors (i.e. vectors of std::string).

My personal idea is:

  1. In theory blf should contain yarp/iDynTree only in some small part of the code (communication with the robot, kinDynComputation, etc)
  2. Avoiding the toEigen() here and there is plus of eigen
  3. The nice thing about the Ref is the fact that you can do.
    void foo(Eigen::Ref<Eigen::MatrixXd> m); 
    
    int main()
    {
        Eigen::MatrixXd m(100,100);
        foo(m.block(10,10,23,41));
    } 
    similarly, it can be done with the vectors.
  4. Resizing an Eigen::Ref can be generally done, if the resize is not possible an error is thrown at runtime (in debug).
  5. I would vote to keep the GenericVector in:
    • the parameter handler
    • Robot interface only

For point 5, I would add some template capability (I don't know how and what) to avoid to do this

foo(std::string, GenericContainer::Vector, GenericContainerVectorResizeMode)

It would be nice to have

foo(std::string, GenericContainer::Vector)

and then the GenericContainer::Vector is built to be resizable or not according to the initial vector. (I think that Eigen::Ref does something similar since you can build an Eigen::Ref<Eigen::Vector3d> )

@S-Dafarra
Copy link
Member Author

For point 5, I would add some template capability (I don't know how and what) to avoid to do this

foo(std::string, GenericContainer::Vector, GenericContainerVectorResizeMode)

It would be nice to have

foo(std::string, GenericContainer::Vector)

and then the GenericContainer::Vector is built to be resizable or not according to the initial vector. (I think that Eigen::Ref does something similar since you can build an Eigen::Ref<Eigen::Vector3d> )

I thought about this. I later realized that the issue here would be that the choice of being resizable or not would be done by the developer, and not by the user. This may be undesirable, since the user may use fixed-size vectors with methods that have been conceived by the developer to work with resizable arrays. Right now this is possible, and eventually, a runtime error is thrown if there is an attempt of resizing the vector, but otherwise it works fine. Instead, if we set this property from template, it would be a strong requirement already a compilation time.

Notice that right now the interfaces using a GenericContainer::Vector are already like

foo(std::string, GenericContainer::Vector)

The resize mode is added to the template methods that do the conversion to a GenericContainer::Vector (see here).

So, the real problem is that it is not possible to convert a random vector to a GenericContainer::Vector reference.

@S-Dafarra
Copy link
Member Author

So, the real problem is that it is not possible to convert a random vector to a GenericContainer::Vector reference.

This should be solved by #102. After this, the code that was like

void foo(const GenericContainer::Vector<double>& input);

can be turned in

void foo(const GenericContainer::Vector<const double>::Ref input);

while

void foo(GenericContainer::Vector<double>& input);

will become

void foo(GenericContainer::Vector<double>::Ref input);

The advantage of this, it that it can be used with all the vectors supported by GenericContainer::Vector. I have tried it also with block operations. The problem here is that this can be used only with the const version. Infact, block.data() returns a const pointer, since the memory may not be contiguous. The nice thing about Span though, is that it has a handy method called subspan. Hence, at least for vectors, we can achieve the same result with

Eigen::VectorXd test;
void foo(iDynTree::Span(test.data(), test.size()).subspan(5, 2))

Unfortunately, at the moment, it is not possible to use iDynTree::make_span with Eigen objects, but we can fix that.

@S-Dafarra
Copy link
Member Author

Unfortunately, at the moment, it is not possible to use iDynTree::make_span with Eigen objects, but we can fix that.

I guess this has been fixed robotology/idyntree#766

@GiulioRomualdi
Copy link
Member

We are currently using Eigen::Vector Similar to #96 (comment) I'm going to close this

@GiulioRomualdi GiulioRomualdi closed this as not planned Won't fix, can't repro, duplicate, stale Apr 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants