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

Change RobotModel.forward_kinematics() interface #94

Merged
merged 3 commits into from
Mar 15, 2022
Merged

Change RobotModel.forward_kinematics() interface #94

merged 3 commits into from
Mar 15, 2022

Conversation

luisenp
Copy link
Contributor

@luisenp luisenp commented Feb 25, 2022

Motivation and Context

As discussed in #84

@luisenp luisenp added the refactor Refactor library components label Feb 25, 2022
@luisenp luisenp self-assigned this Feb 25, 2022
@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 25, 2022
@exhaustin
Copy link
Contributor

I think the typing makes sense given our current context.
If it's okay with you, I would like to gain more understanding by prototyping the inverse kinematics test first, then return and confirm the changes in this PR.

@mhmukadam mhmukadam marked this pull request as draft March 7, 2022 17:16
exhaustin
exhaustin reviewed Mar 8, 2022
View reviewed changes
Copy link
Contributor

@exhaustin exhaustin left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Added some comments after going through the IK test.
Overall I think the direction of this interface looks really good!

theseus/embodied/kinematics/robot_model.py Outdated Show resolved Hide resolved
theseus/embodied/kinematics/robot_model.py Outdated Show resolved Hide resolved
theseus/embodied/kinematics/robot_model.py Outdated Show resolved Hide resolved
@luisenp luisenp marked this pull request as ready for review March 15, 2022 16:07
exhaustin
exhaustin approved these changes Mar 15, 2022
View reviewed changes
Copy link
Contributor

@exhaustin exhaustin left a comment

Choose a reason for hiding this comment

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

Lgtm, thanks for iterating through with us!

mhmukadam
mhmukadam approved these changes Mar 15, 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!

@luisenp luisenp merged commit 22a0fe6 into main Mar 15, 2022
@luisenp luisenp deleted the lep.changed_fk_interface branch March 15, 2022 17:22
suddhu pushed a commit to suddhu/theseus that referenced this pull request Jan 21, 2023
@luisenp
Change RobotModel.forward_kinematics() interface (facebookresearch#94)
9955950 
* Change RobotModel.forward_kinematics() interface.

* Removed unnecessary if statement in IdentityModel.

* Changed RobotModel.forward_kinematics() so that output is always a dict.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@exhaustin exhaustin exhaustin approved these changes

@mhmukadam mhmukadam mhmukadam approved these changes

Assignees

@luisenp luisenp

Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. refactor Refactor library components
Projects
None yet
Milestone
No milestone
Development

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