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.

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

Remove Iterative Spline and Iterative Parabola time-param algorithms (v2) #1780

Merged
merged 3 commits into from
Dec 6, 2022

Conversation

AndyZe
Copy link
Member

@AndyZe AndyZe commented Dec 3, 2022

We've been using TOTG by default for awhile now. The only drawback of TOTG I'm aware of is that it can't handle nonzero initial or final velocities/accelerations. So I was thinking that would be a reason to keep the older algorithms (ISTP and IPTP). But, I added a little test and it looks like neither of those algorithms handles it either. Even worse -- they just fail without throwing any error. So I think we should delete them.

3 commits:

  • The first commit adds a simple test for IPTP. The test passes but the output is wrong. (It didn't do anything.)
20: [ RUN      ] TestTimeParameterization.NonzeroInitialVelAccel
20: Trajectory has 2 points over 0.000 seconds
20:   waypoint   0 time 0.000 pos  0.000 vel  0.000 acc  0.000
20:   waypoint   1 time 0.000 pos  0.000 vel  0.000 acc  0.000
20: [       OK ] TestTimeParameterization.NonzeroInitialVelAccel (0 ms)
  • The second commit adds a simple test for ISTP. Again, the test passes but the output is wrong. The state at the final waypoint should be (position, vel, accel) == (0.01, 0.1, 0.2).
20: [ RUN      ] TestTimeParameterization.NonzeroInitialVelAccel
20: Trajectory has 4 points over 0.343 seconds
20:   waypoint   0 time 0.000 pos  0.000 vel  0.000 acc  0.742
20:   waypoint   1 time 0.118 pos  0.003 vel  0.021 acc -0.394
20:   waypoint   2 time 0.253 pos  0.005 vel  0.029 acc  0.526
20:   waypoint   3 time 0.343 pos  0.010 vel  0.100 acc  1.036
20: [       OK ] TestTimeParameterization.NonzeroInitialVelAccel (0 ms)
  • The third commit deletes both of the plugins.

Note, this is a rebased version of #1191. I wasn't able to re-open that PR because "the branch was recreated or force-pushed." I'm re-opening the PR because there is new interest in changing the API of TOTG. Specifically, some want to be able to change tolerances or timesteps without constructing a new TOTG object.

@AndyZe AndyZe requested review from sjahr and nbbrooks December 3, 2022 04:45
@codecov
Copy link

codecov bot commented Dec 3, 2022

Codecov Report

Base: 50.90% // Head: 50.25% // Decreases project coverage by -0.65% ⚠️

Coverage data is based on head (7dd8eee) compared to base (86991d1).
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1780      +/-   ##
==========================================
- Coverage   50.90%   50.25%   -0.64%     
==========================================
  Files         378      374       -4     
  Lines       31728    31274     -454     
==========================================
- Hits        16149    15715     -434     
+ Misses      15579    15559      -20     
Impacted Files Coverage Δ
...eit_core/robot_trajectory/src/robot_trajectory.cpp 56.65% <0.00%> (-10.40%) ⬇️
moveit_core/robot_state/src/robot_state.cpp 47.87% <0.00%> (-0.07%) ⬇️
...include/moveit/robot_trajectory/robot_trajectory.h 98.67% <0.00%> (-0.03%) ⬇️
...dl_kinematics_plugin/src/kdl_kinematics_plugin.cpp 79.93% <0.00%> (+1.14%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@tylerjw tylerjw left a comment

Choose a reason for hiding this comment

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

Deleting code is an easy approve. Thank you for looking into this and determining we shouldn't keep this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants