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

Add inverse kinematics example #538

Merged
merged 9 commits into from
Jun 27, 2023
Merged

Add inverse kinematics example #538

merged 9 commits into from
Jun 27, 2023

Conversation

fantaosha
Copy link
Contributor

Motivation and Context

How Has This Been Tested

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have completed my CLA (see CONTRIBUTING)
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@fantaosha fantaosha added the enhancement New feature or request label Jun 20, 2023
@fantaosha fantaosha self-assigned this Jun 20, 2023
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 20, 2023
Base automatically changed from lep.suddhus_fork_changes to main June 21, 2023 14:44
Copy link
Contributor

@luisenp luisenp left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! Left some comments.

Besides addressing this, I think we should include comments to guide people throughout the example. The scripts in this folder are meant for people to learn how to use theseus.

examples/labs/inverse_kinematics.py Outdated Show resolved Hide resolved
theta = torch.rand(10, robot.dof, dtype=dtype)
targeted_poses_ee: torch.Tensor = fk(theta)[0]
theta = torch.zeros_like(theta)
for iter in range(50):
Copy link
Contributor

Choose a reason for hiding this comment

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

We should be able to use theseus for this, right? We can show how to create a custom cost function with some vector optim vars for theta and wrap fk and jfk computation. Then we just add to objective and optimize.

This cost function could even be added to theseus.embodied once we merge FK from labs.

Copy link
Contributor

Choose a reason for hiding this comment

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

As discussed offline, let's keep the current version and focus on adding more explanations and references. We can remove the CostFunction for now, but keep the code around somewhere, in case we have time to add it and clean it up properly soon.

theta = torch.zeros_like(theta)
for iter in range(50):
jac_b, poses = jfk_b(theta)
error = SE3.log(SE3.compose(SE3.inv(poses[-1]), targeted_poses_ee)).view(-1, 6, 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can be made more concise if we use LieTensor class, which has a local() implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will add local() to lie.functional. See #542

examples/labs/inverse_kinematics.py Outdated Show resolved Hide resolved
tests/labs/embodied/robot/test_forward_kinematics.py Outdated Show resolved Hide resolved
examples/labs/inverse_kinematics.py Outdated Show resolved Hide resolved
theseus/labs/embodied/robot/forward_kinematics.py Outdated Show resolved Hide resolved
(theta,) = optim_vars
(targeted_pose,) = aux_vars
pose = th.SE3(tensor=fk(theta.tensor)[0])
return (pose.inverse().compose(targeted_pose)).log_map()
Copy link
Member

Choose a reason for hiding this comment

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

we can call targeted_pose.local(pose)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it pose.local(targeted_pose)? In either case, agree that we should call local().

Copy link
Member

@mhmukadam mhmukadam Jun 23, 2023

Choose a reason for hiding this comment

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

That's what we have in the Local cost function in /embodied/misc.

In the longer term we should actually make a unified version of this cost function that can optionally call in to FK and apply local on pose/point/rot of a specific joint.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the right way to do this would be this, which unfortunately we don't have time to implement, but it would be very nice to have.

Copy link
Member

@mhmukadam mhmukadam left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@luisenp luisenp left a comment

Choose a reason for hiding this comment

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

Code looks good, but left some minor comments. I think the most important is to use this example to clearly explain the API, because we don't have any documentation for this code.

# the selected links, in that order. The return types of these functions are as
# follows:
#
# - fk: return poses of selected links
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should be more explicit here, this description doesn't help the reader understand the API.

For example, fk returns N + 1 outputs, where N is the number of selected links. The first N correspond to poses (tensors, I think?). And I think the last output is a list containing poses for all links? It would be really useful to include this level of detail.

Same goes for the jacobians.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, fk will only return the tuple of selected link poses.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, my bad, I missed this slicing for fk function. Sorry about that. You should still mention that the return is a tuple of pose tensors, one for each selected link, in the same order as the link names given as input.

Also, can you reword the jacobians part so it says "returns a tuple where the first element is a list with the body jacobians of selected links, and the second element is a tuple with the corresponding poses."?

(theta,) = optim_vars
(targeted_pose,) = aux_vars
pose = th.SE3(tensor=fk(theta.tensor)[0])
return (pose.inverse().compose(targeted_pose)).log_map()
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it pose.local(targeted_pose)? In either case, agree that we should call local().

@fantaosha fantaosha requested a review from luisenp June 26, 2023 02:20
@fantaosha fantaosha changed the base branch from main to lep.graduate_fk_labs June 27, 2023 15:11
Base automatically changed from lep.graduate_fk_labs to main June 27, 2023 18:16
@fantaosha fantaosha merged commit d7f898c into main Jun 27, 2023
@fantaosha fantaosha deleted the taoshaf.ik_example branch June 27, 2023 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants