improved checking for joint-space jumps of Cartesian path #843
Conversation
… removing throw error from abs jump test
- test for both, relative and absolute jumps (if corresponding thresholds > 0) - skip revolute/prismatic if threshold is <= 0
If the trajectory consists only of the first (current) robot state, there is no need to test for joint space jumps and thus we avoid printing the new warning "too few points to test" in this case.
| robot_state->copyJointGroupPositions(joint_model_group, joint_positions); | ||
| joint_positions[0] -= 1.01; | ||
| robot_state->setJointGroupPositions(joint_model_group, joint_positions); | ||
| traj.push_back(robot_state); |
There was a problem hiding this comment.
On further reflection, I think we should have the cutoff further than 1 index from the end of the list. Returning 3/4 is susceptible to off by one errors
There was a problem hiding this comment.
I don't get this point. Can you please elaborate?
There was a problem hiding this comment.
Off by one errors are one of the most common types of bugs. The probability that some future bug slips past the test because of an off by one error that returns the length of the input trajectory minus one rather than actually finding the jump is non-trivial. While developing #821 I found a bug in my own code that did exactly this. It would be really easy to add another point to the trajectory so that the test would expect 3 out of 5 rather than 3 out of 4.
This was the thinking behind the changes that got reverted in 20dc432.
|
|
||
| // direct call of absolute version: factor slightly smaller than 3 (1.01 > 2.99 * 1.01 / 3) | ||
| generateTestTraj(traj, robot_model, joint_model_group); | ||
| double fraction = robot_state::RobotState::testRelativeJointSpaceJump(joint_model_group, traj, 2.99); |
There was a problem hiding this comment.
I think this is a regression from the tests in #821. More test coverage is generally better. Why did you revert these?
std::size_t generateTestTraj(std::vector<std::shared_ptr<robot_state::RobotState>>& traj,
const moveit::core::RobotModelConstPtr& robot_model,
const robot_model::JointModelGroup* joint_model_group)
{
traj.clear();
std::size_t n_joints = joint_model_group->getJointModelNames().size();
for (std::size_t traj_ix = 0; traj_ix < 3; ++traj_ix)
{
std::shared_ptr<robot_state::RobotState> robot_state1(new robot_state::RobotState(robot_model));
robot_state1->setToDefaultValues();
traj.push_back(robot_state1);
}
// This is a jump
std::shared_ptr<robot_state::RobotState> robot_state2(new robot_state::RobotState(robot_model));
robot_state2->setToDefaultValues();
std::vector<double> joint_positions;
robot_state2->copyJointGroupPositions(joint_model_group, joint_positions);
joint_positions[0] -= 1.01;
robot_state2->setJointGroupPositions(joint_model_group, joint_positions);
traj.push_back(robot_state2);
return traj.size();
}
TEST_F(LoadPR2, testJointSpaceJumpAbsolute)
{
const robot_model::JointModelGroup* joint_model_group = robot_model->getJointModelGroup("right_arm");
std::vector<std::shared_ptr<robot_state::RobotState>> traj;
std::size_t traj_len = generateTestTraj(traj, robot_model, joint_model_group);
robot_state::JumpThreshold jt_abs(1.0, 1.0);
double fraction = robot_state::RobotState::testJointSpaceJump(joint_model_group, traj, jt_abs);
EXPECT_NEAR(traj.size(), 3, 0.01);
EXPECT_NEAR(fraction, 3. / (double)traj_len, 0.01);
}
TEST_F(LoadPR2, testJointSpaceJumpRelative)
{
const robot_model::JointModelGroup* joint_model_group = robot_model->getJointModelGroup("right_arm");
std::vector<std::shared_ptr<robot_state::RobotState>> traj;
std::size_t traj_len = generateTestTraj(traj, robot_model, joint_model_group);
robot_state::JumpThreshold jt_rel(1.0);
double fraction = robot_state::RobotState::testJointSpaceJump(joint_model_group, traj, jt_rel);
EXPECT_NEAR(traj.size(), 3, 0.01);
EXPECT_NEAR(fraction, 3. / (double)traj_len, 0.01);
}
There was a problem hiding this comment.
As discussed here, your test extensions were overly complex and didn't added any functionality. In 20dc432 you already reverted most of those changes.
I simply cleaned them up even more to facilitate readability.
I agree that more test coverage is better. However, I'm puzzled why you complain, as I have even more tests, namely testing for both direct (testAbsoluteJointSpaceJump / testRelativeJointSpaceJump) and indirect calls (testJointSpaceJump).
| robot_state->copyJointGroupPositions(joint_model_group, joint_positions); | ||
| joint_positions[0] -= 1.01; | ||
| robot_state->setJointGroupPositions(joint_model_group, joint_positions); | ||
| traj.push_back(robot_state); |
There was a problem hiding this comment.
Off by one errors are one of the most common types of bugs. The probability that some future bug slips past the test because of an off by one error that returns the length of the input trajectory minus one rather than actually finding the jump is non-trivial. While developing #821 I found a bug in my own code that did exactly this. It would be really easy to add another point to the trajectory so that the test would expect 3 out of 5 rather than 3 out of 4.
This was the thinking behind the changes that got reverted in 20dc432.
|
I synced this and other changes to melodic just now. |
This is a rework of #821, with cleaner implementation, particularly more readable unittests.
I also dropped the recently added flags
test_for_relative_jumpandtest_for_absolute_jumpin theJumpThreshold struct. @mlautman, @v4hn, @davetcoleman please review to finally merge this.I also already included Michael's PR #831.
From my point of view, this PR can be squash-merged if finally accepted.