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

Add getJacobian tests #2375

Merged
merged 4 commits into from Sep 26, 2023
Merged

Conversation

marioprats
Copy link
Contributor

Description

This change adds some basic tests for the RobotState getJacobian method, as a first step towards #2345. The tests cover the different types of joints (fixed, revolute, planar, prismatic) with a simple 4DOF mock robot. It also cleans up some unused code and old-style literal strings.

Checklist

  • Required by CI: Code is auto formatted using clang-format
  • Extend the tutorials / documentation reference
  • Document API changes relevant to the user in the MIGRATION.md notes
  • Create tests, which fail without this PR reference
  • Include a screenshot if changing a GUI
  • While waiting for someone to review your request, please help review another open pull request to support the maintainers

@codecov
Copy link

codecov bot commented Sep 19, 2023

Codecov Report

Patch coverage has no change and project coverage change: +0.07% 🎉

Comparison is base (94fea79) 50.74% compared to head (772e823) 50.80%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2375      +/-   ##
==========================================
+ Coverage   50.74%   50.80%   +0.07%     
==========================================
  Files         386      386              
  Lines       31970    31973       +3     
==========================================
+ Hits        16219    16241      +22     
+ Misses      15751    15732      -19     

see 5 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@henningkayser henningkayser left a comment

Choose a reason for hiding this comment

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

Thank you for these test cases. Implementation looks good to me, there are still some open questions from my side. Also, how confident can we be about the angle offset 1e-04 given the fairly small delta and what is an expected tolerance?

moveit_core/robot_state/test/robot_state_test.cpp Outdated Show resolved Hide resolved
moveit_core/robot_state/test/robot_state_test.cpp Outdated Show resolved Hide resolved
moveit_core/robot_state/test/robot_state_test.cpp Outdated Show resolved Hide resolved
moveit_core/robot_state/test/robot_state_test.cpp Outdated Show resolved Hide resolved
@henningkayser
Copy link
Member

clang-tidy has some complaints about variable naming https://github.com/ros-planning/moveit2/actions/runs/6233581824/job/16919090852?pr=2375#step:12:9507

@marioprats
Copy link
Contributor Author

Thank you for these test cases. Implementation looks good to me, there are still some open questions from my side. Also, how confident can we be about the angle offset 1e-04 given the fairly small delta and what is an expected tolerance?

Not sure what's a good way to put bounds on the expected offset, but 1e-04 is small enough I think?
I could make it 1e-05 by also reducing the time step, which sounds right.

Copy link
Contributor

@sjahr sjahr 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. Just a couple of more suggestions to sprinkle more const over the code (I hope all of them work and I did not miss required mutability). But not blocking the merge

moveit_core/robot_state/test/robot_state_test.cpp Outdated Show resolved Hide resolved
moveit_core/robot_state/test/robot_state_test.cpp Outdated Show resolved Hide resolved
moveit_core/robot_state/test/robot_state_test.cpp Outdated Show resolved Hide resolved
moveit_core/robot_state/test/robot_state_test.cpp Outdated Show resolved Hide resolved
moveit_core/robot_state/test/robot_state_test.cpp Outdated Show resolved Hide resolved
moveit_core/robot_state/test/robot_state_test.cpp Outdated Show resolved Hide resolved
moveit_core/robot_state/test/robot_state_test.cpp Outdated Show resolved Hide resolved
moveit_core/robot_state/test/robot_state_test.cpp Outdated Show resolved Hide resolved
moveit_core/robot_state/test/robot_state_test.cpp Outdated Show resolved Hide resolved
@henningkayser henningkayser merged commit c8a0eb0 into moveit:main Sep 26, 2023
8 checks passed
m-elwin pushed a commit to m-elwin/moveit2 that referenced this pull request Dec 4, 2023
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

3 participants