Bug fixes to apply for the DmpWithGainSchdules #65
Conversation
There was a problem hiding this comment.
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?
|
I'll now make a commit with some hints on how to implement DmpWithGainSchedules::setParameterVector(const std::vectorEigen::VectorXd& vector_values, bool normalized) (I have just 10 minutes for this just now) |
Implemented in 876ed29. It compiles, but it is not tested yet! With this implementation, there should be no need for this change: |
|
You are right. I will review the new code and undo the changes of the commit 57c1415#diff-6f720489f3271c0ea515e6d4d68759453a40d76bd4058e7b48783a99e6ff8fc0 |
|
I added the changes of 876ed29, reverted the changes of 57c1415, and corrected a couple of typos. However, I am not sure if the implementation of DmpWithGainSchedules::setParameterVector(const std::vectorEigen::VectorXd) 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. |
|
Update: The problem was not to implement DmpWithGainSchedules::setParameterVector(std::vector<...>), but to implement Dmp::getParameterVector(std::vectorEigen::VectorXd). 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. |
|
@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. |
|
@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. |
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).