search

stulp / dmpbbo

Python/C++ library for Dynamical Movement Primitives and Black-Box Optimization
GNU Lesser General Public License v2.1
227 stars 89 forks source link

Bug fixes to apply for the DmpWithGainSchdules #65

Closed ignacio-pm closed 2 years ago

ignacio-pm commented 2 years ago

In this pull request, some bug fixes were fixed and two functionality issues were discovered. Functionality in the demo_robot works but a couple of decisions should be made before merging. It is commented on the files but here I explain with a bit more detail. In demoImitationAndOptimization.cpp and demos_cpp/dmp_bbo/demoOptimizationDmp.cpp I got a compiling error because of introducing a vector of VectorXd in the getParameterVector function. I am not sure of the reason because the only thing that changed was the name of the function. Since I did not need these demos for my experiment, I deleted the vector functionality as a quick fix.

In line 762 of src/dmp/Dmp.cpp I noticed that when calling getParameterVectorSize in Dmp::getParameterVector from DmpWithGainSchedules, it used DmpWithGainSchedules::getParameterVectorSize and not Dmp::getParameterVectorSize. This caused an error in the size of the vector. It should be decided if it is desired to call Dmp::getParameterVectorSize by specifying the prefix Dmp:: or to change the functionality of the whole class. There might be more functions that would be affected too.

Check if the change of the loop of line 378 of src/dmp/DmpWithGainSchedules.cpp affects the functionality (I do not think so).

stulp commented 2 years ago

I'll now make a commit with some hints on how to implement DmpWithGainSchedules::setParameterVector(const std::vector& vector_values, bool normalized) (I have just 10 minutes for this just now)

stulp commented 2 years ago

This change may make learning inefficient, especially with Covariance Matrix Adaptation. I recommend not doing this.

The issue is that I did not have time to implement the std::vector variant of setParameterVector in DmpWithGainSchedules. It is commented out. Do you think you have enough knowledge to implement that, or should I give it a shot this evening?

Implemented in 876ed292be722f91c80. It compiles, but it is not tested yet!

With this implementation, there should be no need for this change: https://github.com/stulp/dmpbbo/pull/65/commits/57c1415276f4ce1e08f921ec54819b371d3a14a1#diff-6f720489f3271c0ea515e6d4d68759453a40d76bd4058e7b48783a99e6ff8fc0

ignacio-pm commented 2 years ago

You are right. I will review the new code and undo the changes of the commit https://github.com/stulp/dmpbbo/commit/57c1415276f4ce1e08f921ec54819b371d3a14a1#diff-6f720489f3271c0ea515e6d4d68759453a40d76bd4058e7b48783a99e6ff8fc0

ignacio-pm commented 2 years ago

I added the changes of https://github.com/stulp/dmpbbo/commit/876ed292be722f91c80369d18d2c9a1fc337cb51, reverted the changes of https://github.com/stulp/dmpbbo/commit/57c1415276f4ce1e08f921ec54819b371d3a14a1, and corrected a couple of typos.

However, I am not sure if the implementation of DmpWithGainSchedules::setParameterVector(const std::vector) is correct. Labels_gains is not used and I would do something more similar to DmpWithGainSchedules::setParameterVector(const Eigen::VectorXd). My personal opinion is to merge this pull request to the cpp_parameterizable branch and then solve this there or in another pull request.

ignacio-pm commented 2 years ago

Update: The problem was not to implement DmpWithGainSchedules::setParameterVector(std::vector<...>), but to implement Dmp::getParameterVector(std::vector). I did a first draft of the function that is not yet tested but it can be a starting point. I am not sure why it could compile in your computer without this function, since it is used in demoImitationAndOptimization.cpp and demos_cpp/dmp_bbo/demoOptimizationDmp.cpp.

stulp commented 2 years ago

@ignacio-pm, this is a pet project for which I can dedicate some minutes here and there. This unfortunately means I don't have time for proper code reviews. Since you are knowledgable of C++ and git, I think its easier if you push to this branch directly, rather than through pull request. For that I will now merge your pull request, and I have given you write acces to the repo. Let's work on cpp_parameterizable together, and in the end when we're both happy with it, I will merge it into master as part of the next release (i.e. please do not touch master in the meantime).

Any discussions or requests to look at code or implement features we can do through the issues rather than code reviews. I'm aware this is not the "proper" way, but I think its the most effective one, given my time constraints.

ignacio-pm commented 2 years ago

@stulp I did not have time in the last few days to look at the project due to a deadline. Thanks for the invitation to edit. However, it was invalid when I tried to accept it. Can you send it again? The new code worked for my experiments but I will try it on your example of demo_robot in the next few days.