-
Notifications
You must be signed in to change notification settings - Fork 124
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 prismatic joint #416
Add prismatic joint #416
Conversation
26d2a70
to
e9fd82c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments to address.
theseus/embodied/robot/joint.py
Outdated
if prismatic_axis.ndim == 1: | ||
prismatic_axis = prismatic_axis.view(-1, 1) | ||
|
||
if prismatic_axis.ndim != 2 or prismatic_axis.shape != (3, 1): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here similar comment as I had for RevoluteJoint
on using numel()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored. And now the error message is "The prismatic axis must have 3 elements."
super().__init__(name, parent, child, origin, dtype) | ||
self._axis[3] = 1 | ||
|
||
def _translation_impl(self, angle: torch.Tensor) -> torch.Tensor: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code in _translation_impl
and relative_pose
of these 3 classes is exactly the same except for one number, so you can just have a base class _PrismaticJointAxis
that defines a member variable self._axis_idx: Optional[int] = None
, which gets overridden by all base classes. Then we have
def _translation_impl(self, angle: torch.Tensor) -> torch.Tensor:
self._check_angle_shape(angle)
trans = angle.new_zeros(angle.shape[0], 3)
trans[:, self._axis_idx] = angle
return trans
def relative_pose(self, angle: torch.Tensor) -> torch.Tensor:
self._check_angle_shape(angle)
ret = angle.new_empty(angle.shape[0], 3, 4)
ret[:, :, :3] = self.origin[:, :, :3]
ret[:, :, 3] = angle * self.origin[:, :, self._axis_idx] + self.origin[:, :, 3]
return ret
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored. Thanks!
return 1 | ||
|
||
@abc.abstractmethod | ||
def _translation_impl(self, angle: torch.Tensor) -> torch.Tensor: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as in RevoluteJoint
regarding a translation()
method, with similar structure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored.
return 1 | ||
|
||
@abc.abstractmethod | ||
def _translation_impl(self, angle: torch.Tensor) -> torch.Tensor: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe translation()
and rotation()
(in the other class) should be called translate()
and rotate()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think noun should be better than verb since the outputs are 3x3
or 3x1
tensors, which are transformations.
1a2ce88
to
51702ff
Compare
635c542
to
99ccd46
Compare
name: str, | ||
revolute_axis: torch.Tensor, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe just axis
, I am guessing revolute is implied from the class name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Besides comments here, my other comments are similar to the revolute joint PR so we can follow here what we decide there.
theseus/embodied/robot/joint.py
Outdated
if prismatic_axis.ndim == 1: | ||
prismatic_axis = prismatic_axis.view(-1, 1) | ||
|
||
if prismatic_axis.ndim != 2 or prismatic_axis.shape != (3, 1): | ||
raise ValueError("The prismatic axis must be a 3-D vector.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I imagine this logic of "checking if the input is a valid vector of correct length, and unsqueezing it into a vertical vector if it's 1-D" would be very commonly used, not just within the robot model module. Should this be a piece of logic that can be standardized and deduplicated within an utils
of some sort?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we anticipate this to be used outside robot model, it would be useful to make an utility function. Otherwise, we should at least delegate this repeated logic to the super class.
* add link * add id to joint * add id to link * add robot model * add name to robot * backup * refactor joint * refactor link * add robot model * robot model works * a minior refactor of joint * add ancestor to links * update dof computation * add forward kinematics function * Add differentiable forward kinematics (#420) * fixed a bug in FixedJoint * add angle_ids * add jacobians to FK * forward kinematics works * rename FK * add tests for differentiable kinematics * rename tests * simplify the computation of origin * add jacobian tests for forward kinematics * fixed a bug for vectorize=True in SE3 * improve the efficiency of forward kinematics * add tests for backward propagation * update lie group load paths * Add device to robot model (#422) * add device to robot model * Refactor Joint.dof as a property (#423) * refactor joint dof * add virtual end-effector for tests * seems to work after name changes * rename parent->parent_link and child->child_link * rename angle_ids -> ancestor_active_joint_ids * refactor tests with torch.testing.assert_close * small refactoring of the code * add Robot.get_links() * remove some setters for link and joint * made some functions private for encapsulation * consolidate if block and classmethod->staticmethod * refactor set_robot_from_urdf_file * rename ancestor_active_joint_ids -> ancestor_non_fixed_joint_ids * id=-1 -> id=None * refactor the code for simplicity * move link into joint
* add prismatic joint * refactor prismatic joint * Add robot model (#417) * add link * add id to joint * add id to link * add robot model * add name to robot * backup * refactor joint * refactor link * add robot model * robot model works * a minior refactor of joint * add ancestor to links * update dof computation * add forward kinematics function * Add differentiable forward kinematics (#420) * fixed a bug in FixedJoint * add angle_ids * add jacobians to FK * forward kinematics works * rename FK * add tests for differentiable kinematics * rename tests * simplify the computation of origin * add jacobian tests for forward kinematics * fixed a bug for vectorize=True in SE3 * improve the efficiency of forward kinematics * add tests for backward propagation * update lie group load paths * Add device to robot model (#422) * add device to robot model * Refactor Joint.dof as a property (#423) * refactor joint dof * add virtual end-effector for tests * seems to work after name changes * rename parent->parent_link and child->child_link * rename angle_ids -> ancestor_active_joint_ids * refactor tests with torch.testing.assert_close * small refactoring of the code * add Robot.get_links() * remove some setters for link and joint * made some functions private for encapsulation * consolidate if block and classmethod->staticmethod * refactor set_robot_from_urdf_file * rename ancestor_active_joint_ids -> ancestor_non_fixed_joint_ids * id=-1 -> id=None * refactor the code for simplicity * move link into joint
* add revolute joint * refactor revolute joint * add parent and child to joint * update the path to load so3 and se3 * Add prismatic joint (#416) * add prismatic joint * refactor prismatic joint * Add robot model (#417) * add link * add id to joint * add id to link * add robot model * add name to robot * backup * refactor joint * refactor link * add robot model * robot model works * a minior refactor of joint * add ancestor to links * update dof computation * add forward kinematics function * Add differentiable forward kinematics (#420) * fixed a bug in FixedJoint * add angle_ids * add jacobians to FK * forward kinematics works * rename FK * add tests for differentiable kinematics * rename tests * simplify the computation of origin * add jacobian tests for forward kinematics * fixed a bug for vectorize=True in SE3 * improve the efficiency of forward kinematics * add tests for backward propagation * update lie group load paths * Add device to robot model (#422) * Refactor Joint.dof as a property (#423) * refactor joint dof * add virtual end-effector for tests * seems to work after name changes * rename parent->parent_link and child->child_link * rename angle_ids -> ancestor_active_joint_ids * refactor tests with torch.testing.assert_close * small refactoring of the code * add Robot.get_links() * remove some setters for link and joint * made some functions private for encapsulation * consolidate if block and classmethod->staticmethod * refactor set_robot_from_urdf_file * rename ancestor_active_joint_ids -> ancestor_non_fixed_joint_ids * id=-1 -> id=None * refactor the code for simplicity * move link into joint * simplify the code * minor revision to be consistent with Lie group refatcoring * fixed a CI error * move files to labs
Motivation and Context
How Has This Been Tested
Types of changes
Checklist