Skip to content

Conversation

@mattking-smith
Copy link
Collaborator

Purpose

This factor enables users to specify a desired SE(3) pose and the corresponding linear and angular velocity to the end-effector of a manipulator.

Add Features

  • C++ unit test
  • MATLAB example with Kinova Gen3 manipulator

@mattking-smith mattking-smith added the enhancement New feature or request label May 17, 2023
Copy link
Collaborator

@varunagrawal varunagrawal left a comment

Choose a reason for hiding this comment

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

Some high level comments. Will do a full review after these are addressed.

if (H1) {
// Jacobian for the joint positions
*H1 = (Matrix(12, arm_.dof()) << H_ep * J_jpx_jp[arm_.dof() - 1],//
- J_jvx_jp[arm_.dof() - 1]).finished();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Some weird formatting issue is happening here. Can you please reformat the file?

std::cout << s << "WorkspacePoseVelocityPrior :" << std::endl;
Base::print("", keyFormatter);
std::cout << "desired end-effector pose : ";
des_pose_.print(); // << std::endl;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Kill the comment if the std::endl is not being used.

template <class ARCHIVE>
void serialize(ARCHIVE& ar, const unsigned int version) {
ar& boost::serialization::make_nvp(
"NoiseModelFactor1", boost::serialization::base_object<Base>(*this));
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this should NoiseModelFactor2 and not NoiseModelFactor1

WorkspacePoseVelocityPrior() {}

/**
* Constructor
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add documentation for all parameters.

gtsam::DefaultKeyFormatter) const {
std::cout << s << "WorkspacePoseVelocityPrior :" << std::endl;
Base::print("", keyFormatter);
std::cout << "desired end-effector pose : ";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nitpick: capitalize the first letter so the output looks prettier?
Similarly for the below output of the end effector velocity.

Vector6 des_vel = (Vector6() << 0, 0, 0, 0, 0, 0).finished();
WorkspacePoseVelocityPrior factor(0, 0, cost_model, arm, des_pose, des_vel);
actual = factor.evaluateError(q, qdot, &H_pose_act, &H_vel_act);
expect = (Vector(12) << 00.613943126, 1.48218982, -0.613943126, 1.1609828,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you please comment where you got these numbers from? And please mark this as a regression test.

0.706727485, -0.547039678, 0, -0.141421356237309, 0,
-0.070710678118655, 0.070710678118655, -0.1000)
.finished();
H_pose_exp = numericalDerivative11(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not use numericalDerivative21 and numericalDerivative22? It computes the numerical derivative of a binary function wrt the first and second argument respectively.

qdot, 1e-6);
EXPECT(assert_equal(expect, actual, 1e-6));
EXPECT(assert_equal(H_pose_exp, H_pose_act, 1e-6));
EXPECT(assert_equal(H_vel_exp, H_vel_act, 1e-6));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Assertions should follow immediately after the computation so that the code can fail early without doing extra computation. In this case, the first assert_equal should be right after the expect variable definition.

@mattking-smith mattking-smith marked this pull request as draft October 5, 2025 14:04
@mattking-smith mattking-smith self-assigned this Oct 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants