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

Implement the skeleton of the OptimalControlElement class and SE3Task #167

Merged
merged 19 commits into from
Jan 12, 2021

Conversation

GiulioRomualdi
Copy link
Member

@GiulioRomualdi GiulioRomualdi commented Dec 23, 2020

This PR implements the first skeleton for the TSID library. It is the first PR that will allow achieving #166. In detail, I defined an OptimalControlElement that is an abstract class that described a generic task that can be achieved by TSID. I also developed SE3Task whose purpose is to implement J dν/dt + dJ/dt ν = dv/dt*.

lie-group-controllers is now added as a dependency of blf. Once this PR has been merged lie-group-controllers should be added to the superbuild (cc @traversaro )

Since I would like to keep the PR small, I will implement the other tasks into other PR.

@GiulioRomualdi
Copy link
Member Author

GiulioRomualdi commented Dec 23, 2020

image
lovely!

@GiulioRomualdi GiulioRomualdi marked this pull request as draft December 23, 2020 18:48
@GiulioRomualdi GiulioRomualdi self-assigned this Jan 8, 2021
@GiulioRomualdi GiulioRomualdi changed the title Implement the skeleton of the OptimalControlElement class Implement the skeleton of the OptimalControlElement class and SE3Task Jan 8, 2021
@GiulioRomualdi GiulioRomualdi marked this pull request as ready for review January 8, 2021 16:21
@GiulioRomualdi
Copy link
Member Author

On macOS is failing because of

==> Installing dependencies for pybind11: python@3.9
==> Installing pybind11 dependency: python@3.9
==> Pouring python@3.9-3.9.1_5.catalina.bottle.tar.gz
Error: The `brew link` step did not complete successfully
The formula built, but is not symlinked into /usr/local
Could not symlink bin/2to3
Target /usr/local/bin/2to3
already exists. You may want to remove it:
  rm '/usr/local/bin/2to3'

@traversaro
Copy link
Collaborator

On macOS is failing because of

==> Installing dependencies for pybind11: python@3.9
==> Installing pybind11 dependency: python@3.9
==> Pouring python@3.9-3.9.1_5.catalina.bottle.tar.gz
Error: The `brew link` step did not complete successfully
The formula built, but is not symlinked into /usr/local
Could not symlink bin/2to3
Target /usr/local/bin/2to3
already exists. You may want to remove it:
  rm '/usr/local/bin/2to3'

See:

In short, the combination of homebrew and GitHub Actions is extremely unstable, and incidents like that are quite frequent. Once strategy may be probably to migrate to conda-forge provided dependencies at least for macOS.

@traversaro
Copy link
Collaborator

lie-group-controllers is now added as a dependency of blf. Once this PR has been merged lie-group-controllers should be added to the superbuild (cc @traversaro )

Done robotology/robotology-superbuild#574 .

@DanielePucci
Copy link
Member

CC @diegoferigo

@prashanthr05
Copy link
Collaborator

prashanthr05 commented Jan 8, 2021

@GiulioRomualdi I will take a look at this PR soon. But anyhow, for starters, a naming clarification.
If I am not wrong SE3Task is technically not a $SE(3)$ space defined task, instead is a $SO(3) \times \mathbb{R}^3$ task?

Theoretically, there are differences between the two due to the different definitions of exponential maps and logarithm maps.

Also in the implementation, at a higher level you set the pose, but at the lower level you instead split it into a rotation and a position object to perform the control. (This is misleading).

So in that case, do you think we should rename the class?

Comment on lines 32 to 36
class OptimalControlElement
{
protected:
Eigen::MatrixXd m_A; /**< Element Matrix */
Eigen::VectorXd m_b; /**< Element Vector */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest to name this in a way that is more specific to TSID, like TaskElement, hiding how that TSID problem is solved (either via optimal control, pseudoinverse, or other methods). OptimalControl is, in my opinion, generic and a bit misleading, considering that you are considering only linear elements.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here: f7cd662 I renamed the class as Task

@GiulioRomualdi GiulioRomualdi force-pushed the feature/optimal_control_element branch from d15f1cc to 0b72bae Compare January 12, 2021 10:09
@GiulioRomualdi
Copy link
Member Author

@GiulioRomualdi I will take a look at this PR soon. But anyhow, for starters, a naming clarification.
If I am not wrong SE3Task is technically not a SE(3) space defined task, instead is a SO(3)×R3 task?

Yes exactly

Theoretically, there are differences between the two due to the different definitions of exponential maps and logarithm maps.

Also in the implementation, at a higher level you set the pose, but at the lower level you instead split it into a rotation and a position object to perform the control. (This is misleading).

So in that case, do you think we should rename the class?

I tried to follow what was done in TSID. However, I agree with you that the name could be misleading. Indeed here the MIXED representation is used to define the twist and as you know in Lie theory only left/right trivializations are well defined.

Do you have other names in mind?

@prashanthr05
Copy link
Collaborator

prashanthr05 commented Jan 12, 2021

Do you have other names in mind?

A wordy choice would be in the direction of SO3xR3TaskMixed. where the prefix for Task will specify the task space and the suffix will specify the velocity representation ?

Or suppress the suffix, so that different choices are handled dynamically within the class (in the future), as done in iDynTree for some computations.

@traversaro
Copy link
Collaborator

Do you have other names in mind?

A wordy choice would be in the direction of SO3xR3TaskMixed. where the prefix for Task will specify the task space and the suffix will specify the velocity representation ?

I think that even if not mathematically super-precise, SE3Task may be a good compromise.

@GiulioRomualdi GiulioRomualdi force-pushed the feature/optimal_control_element branch from 1037d3e to 7a776b2 Compare January 12, 2021 11:58
@GiulioRomualdi
Copy link
Member Author

GiulioRomualdi commented Jan 12, 2021

@prashanthr05

Do you have other names in mind?

A wordy choice would be in the direction of SO3xR3TaskMixed. where the prefix for Task will specify the task space and the suffix will specify the velocity representation ?

Or suppress the suffix, so that different choices are handled dynamically within the class (in the future), as done in iDynTree for some computations.

In 7a776b2 I added a check to make sure that the KinDynComputations object handles quantities in mixed representation. About changing the name of the class I would like to keep it as shorter as possible. If you agree I can improve the documentation by explaining the "theoretical" issue you spot.

Copy link
Collaborator

@prashanthr05 prashanthr05 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor comment.

src/TSID/include/BipedalLocomotion/TSID/SE3Task.h Outdated Show resolved Hide resolved
return false;
}

if (m_robotAccelerationVariable.size != m_kinDyn->getNrOfDegreesOfFreedom() + 6)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe declare a const int for this magic 6? So it can be used here and elsewhere?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done bfa5eaa

@prashanthr05
Copy link
Collaborator

@prashanthr05

Do you have other names in mind?

A wordy choice would be in the direction of SO3xR3TaskMixed. where the prefix for Task will specify the task space and the suffix will specify the velocity representation ?
Or suppress the suffix, so that different choices are handled dynamically within the class (in the future), as done in iDynTree for some computations.

In 7a776b2 I added a check to make sure that the KinDynComputations object handles quantities in mixed representation. About changing the name of the class I would like to keep it as shorter as possible. If you agree I can improve the documentation by explaining the "theoretical" issue you spot.

Yes. I think it's ok.

@GiulioRomualdi
Copy link
Member Author

@prashanthr05

Do you have other names in mind?

A wordy choice would be in the direction of SO3xR3TaskMixed. where the prefix for Task will specify the task space and the suffix will specify the velocity representation ?
Or suppress the suffix, so that different choices are handled dynamically within the class (in the future), as done in iDynTree for some computations.

In 7a776b2 I added a check to make sure that the KinDynComputations object handles quantities in mixed representation. About changing the name of the class I would like to keep it as shorter as possible. If you agree I can improve the documentation by explaining the "theoretical" issue you spot.

Yes. I think it's ok.

72041a7 let me know if it's fine with you.

@prashanthr05
Copy link
Collaborator

@prashanthr05

Do you have other names in mind?

A wordy choice would be in the direction of SO3xR3TaskMixed. where the prefix for Task will specify the task space and the suffix will specify the velocity representation ?
Or suppress the suffix, so that different choices are handled dynamically within the class (in the future), as done in iDynTree for some computations.

In 7a776b2 I added a check to make sure that the KinDynComputations object handles quantities in mixed representation. About changing the name of the class I would like to keep it as shorter as possible. If you agree I can improve the documentation by explaining the "theoretical" issue you spot.

Yes. I think it's ok.

72041a7 let me know if it's fine with you.

Great, thanks!!

@GiulioRomualdi GiulioRomualdi merged commit 580bf41 into master Jan 12, 2021
@GiulioRomualdi GiulioRomualdi deleted the feature/optimal_control_element branch January 12, 2021 13:31
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

Successfully merging this pull request may close these issues.

5 participants