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.

Sign up for GitHub

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

Jump to bottom

Add forward kinematics using an URDF to theseus.embodied.kinematics. #84

Merged
merged 49 commits into from
Apr 13, 2022
Merged

Add forward kinematics using an URDF to theseus.embodied.kinematics. #84

merged 49 commits into from
Apr 13, 2022

Conversation

exhaustin
Copy link
Contributor

@exhaustin exhaustin commented Feb 16, 2022 •
edited by joeaortiz
Loading

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.

@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 Feb 16, 2022
luisenp
luisenp reviewed Feb 18, 2022
View reviewed changes
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.

Nice job! This LGTM with some minor comments. @mhmukadam, any thoughts on the API for the UrdfRobotModel.forward_kinematics() method?

BTW, @exhaustin looks like you haven't installed the precommit hooks. You can follow the instructions here.

theseus/embodied/kinematics/tests/test_urdf_model.py Outdated Show resolved Hide resolved
theseus/embodied/kinematics/tests/test_urdf_model.py Outdated Show resolved Hide resolved
theseus/embodied/kinematics/robot_model.py Outdated Show resolved Hide resolved
@exhaustin
Copy link
Contributor Author

I installed the pre-commit hooks which indicated that the code wasn't passing due to the interfaces defined by the RobotModel parent class. The current definition feels off to me and I would also need understanding of how it's being used in collision.py.
Can we can discuss in person when Mustafa's back?

@luisenp
Copy link
Contributor

luisenp commented Feb 18, 2022

I installed the pre-commit hooks which indicated that the code wasn't passing due to the interfaces defined by the RobotModel parent class. The current definition feels off to me and I would also need understanding of how it's being used in collision.py. Can we can discuss in person when Mustafa's back?

That's OK. Once we agree on the interface for RobotModel, as I mentioned in my comment above, then I can make the necessary changes to RobotModel, 2D collision, etc. in a separate PR, and you can then rebase this one of top of that. Let's wait for @mhmukadam to comment on the discussion above.

@luisenp luisenp mentioned this pull request Feb 25, 2022
Merged
mhmukadam
mhmukadam reviewed Feb 28, 2022
View reviewed changes
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.

Looks good to me so far. I agree, we'll be able to ground the discussion more one we reach a full example, then we can work our way backwards from there to streamline the api.

mhmukadam
mhmukadam reviewed Apr 11, 2022
View reviewed changes
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.

Looks great. Nice work on the batched IK test! Just some minor comments and then we should be able to merge.

@luisenp luisenp mentioned this pull request Apr 12, 2022
Merged
mhmukadam
mhmukadam approved these changes Apr 13, 2022
View reviewed changes
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.

Nice work @exhaustin! Looks ready to merge once you add #155 and change Variable -> Vector.

@exhaustin exhaustin merged commit 58393ad into main Apr 13, 2022
@exhaustin exhaustin deleted the austinw.forward_kinematics branch April 13, 2022 20:57
suddhu pushed a commit to suddhu/theseus that referenced this pull request Jan 21, 2023
@exhaustin @luisenp
Add forward kinematics using an URDF to theseus.embodied.kinematics. (f…
91d1db6 
…acebookresearch#84)

* Update robot_model.py

* Implement fk using current drm

* Add DRM as dep

* Debug and add tests

* Formatting

* Simlify SE3 comparison with 'local'

* Renaming

* Fixed bug in Variable.update() that was breaking torch graph when batch_ignore_mask.any() == True.

* Debug typing

* Add IK test

* Fix IK test

* Add quaternion normalization

* Change test from script to test

* Address robot model comments

* Fix minor comments

* Remove debug comment and decrease cost tolerance for IK test

* Add test parametrizations for gradients and input types

* Fix th.Vector related issues

* Simplify imports

* Batched attempt

* Update type checking

* Revert import changes

* Debug batched test

* Add informative error msgs

* Add jacobian test

* tmp commit

* Debug jacobian

* Verbose error msg

* Add jacobian interface for fk

* Debug jacobian test

* Fix imports

* Add batched jacobians

* Change jacobian input typing

* Modify jacoobian interface

* Remove dim method

* Change type check to Vector again

* Tune optimization so tests run faster

Co-authored-by: Luis Pineda <luisen.p@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhmukadam mhmukadam mhmukadam approved these changes

@luisenp luisenp Awaiting requested review from luisenp

Assignees

@exhaustin exhaustin

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@exhaustin @luisenp @mhmukadam @facebook-github-bot