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

Benchmarking with different Motion Planners (STOMP, CHOMP, OMPL) #992

Merged
merged 8 commits into from Aug 1, 2018

Conversation

Projects
None yet
3 participants
@raghavendersahdev
Copy link
Contributor

raghavendersahdev commented Jul 20, 2018

Benchmarking with different Motion Planners (STOMP, CHOMP, OMPL)

This PR is a work done by Raghavender as a part of 2018 Google summer of code. As a part of this PR, benchmarking works for different motion planners (CHOMP, STOMP, OMPL) available in MoveIt. For a particular benchmark(start sates, planning scene, goal states, queries, etc.), different planners can be evaluated.

Checklist

  • addition of yaml files and launch files for including STOMP and CHOMP planners in environments with and without obstacles.
  • fixing the existing benchmarking code to make it work with STOMP and CHOMP.
- plugin: chomp_interface/CHOMPPlanner
planners:
- CHOMP
- plugin: stomp_moveit/StompPlannerManager

This comment has been minimized.

@mamoll

mamoll Jul 20, 2018

Contributor

maybe stomp_moveit should be renamed to stomp_interface to be consistent with OMPL and CHOMP?

This comment has been minimized.

@davetcoleman

davetcoleman Jul 20, 2018

Member

I'm guessing this is tied by the package name

This comment has been minimized.

@raghavendersahdev

raghavendersahdev Jul 21, 2018

Contributor

yes this is the package name: stomp_moveit

This comment has been minimized.

@mamoll

mamoll Jul 22, 2018

Contributor

My point was that perhaps the package should be renamed for consistency. This can be done later, if others agree.

This comment has been minimized.

@davetcoleman

davetcoleman Jul 27, 2018

Member

i agree it would be nice

@@ -27,3 +27,4 @@ benchmark_config:
- plugin: my_plugin_ns/my_plugin
planners:
- MyPlanner

This comment has been minimized.

@davetcoleman

davetcoleman Jul 20, 2018

Member

please don't introduce changes like this into PRs since nothing else in this file was modified

This comment has been minimized.

@raghavendersahdev

raghavendersahdev Jul 21, 2018

Contributor

sure this was my mistake, removed this change

@@ -532,6 +532,9 @@ bool BenchmarkExecutor::plannerConfigurationsExist(const std::map<std::string, s
planning_interface::PlannerManagerPtr pm = planner_interfaces_[it->first];
const planning_interface::PlannerConfigurationMap& config_map = pm->getPlannerConfigurations();

if (pm->getDescription().compare("stomp") || pm->getDescription().compare("chomp"))

This comment has been minimized.

@davetcoleman

davetcoleman Jul 20, 2018

Member

what is this for? add comment inline

This comment has been minimized.

@raghavendersahdev

raghavendersahdev Jul 21, 2018

Contributor

added comments explaining this, I put this here because for STOMP and CHOMP, the benchmarking was throwing an error as this line for CHOMP/STOMP returns a size of 0 for the config_map parameter which was causing an error. After putting this check, everything worked fine.

@mamoll

mamoll approved these changes Jul 22, 2018

Copy link
Contributor

mamoll left a comment

Ready to be merged once one small comment is updated.

- plugin: chomp_interface/CHOMPPlanner
planners:
- CHOMP
- plugin: stomp_moveit/StompPlannerManager

This comment has been minimized.

@mamoll

mamoll Jul 22, 2018

Contributor

My point was that perhaps the package should be renamed for consistency. This can be done later, if others agree.

@@ -532,6 +532,11 @@ bool BenchmarkExecutor::plannerConfigurationsExist(const std::map<std::string, s
planning_interface::PlannerManagerPtr pm = planner_interfaces_[it->first];
const planning_interface::PlannerConfigurationMap& config_map = pm->getPlannerConfigurations();

// if the planner is chomp or stomp skip this function and return true for checking planner configurations for the
// planning group other wise an error occurs, this is kind of a trick which works well

This comment has been minimized.

@mamoll

mamoll Jul 22, 2018

Contributor

Even with other wise replaced with otherwise, I still can't parse this. Why is this a trick? Perhaps the issue that with OMPL, a specific planning algorithm needs to be defined for a planning group, whereas with STOMP and CHOMP this is not necessary.

This comment has been minimized.

@raghavendersahdev

raghavendersahdev Jul 24, 2018

Contributor

changed comment

@raghavendersahdev raghavendersahdev force-pushed the raghavendersahdev:benchmarking branch from b236e84 to 3165b80 Jul 25, 2018

@raghavendersahdev

This comment has been minimized.

Copy link
Contributor

raghavendersahdev commented Jul 25, 2018

Sorry, I did something bad here while rebasing, accidentally rebased with the wrong local branch (raghavendersahdev/moveit kinetic branch) rather than the actual remote ros-planning/moveit kinetic branch, I am trying to fix it.

So at the end I basically merged the commits from this PR into the current one. But I am trying to fix this using git reflog

@davetcoleman

This comment has been minimized.

Copy link
Member

davetcoleman commented Jul 27, 2018

its not clear if i should review this

@raghavendersahdev

This comment has been minimized.

Copy link
Contributor

raghavendersahdev commented Jul 27, 2018

@davetcoleman I had a question for you, is it possible to undo a rebase after I already pushed it (I think it is kind of complicated coz I deleted my local branch on my laptop) ?

I actually wanted to rebase my raghavendersahdev/moveit benchmarking branch with the ros-planning/moveit kinetic branch ...But accidentally I rebased this with my own current raghavendersahdev/moveit kinetic branch.....So what ended up happening was => the wrong rebase merged the commits from my kinetic branch which also has a PR#987 open currently......

This specifc PR has only 6 commits but the branch which I used to rebase this with had 10 commits so I ended up with 16 commits in this PR.

Should I get rid of this PR and open a new one ? .... I am Sorry for the confusion pertaining to this specific PR.

@davetcoleman

This comment has been minimized.

Copy link
Member

davetcoleman commented Jul 27, 2018

you can branch off of kinetic-devel then cherry-pick the relevant commits back on top of that, and it should work out

@raghavendersahdev

This comment has been minimized.

Copy link
Contributor

raghavendersahdev commented Jul 27, 2018

ok, thanks Dave, I will read up on git cherry-pick and try to get rid of the 10 non-relevant commits in my raghavendersahdev/moveit benchmarking branch to only have the original 6 commits in the benchmarking branch

@raghavendersahdev raghavendersahdev force-pushed the raghavendersahdev:benchmarking branch from 3165b80 to ed9bdf6 Jul 29, 2018

@raghavendersahdev

This comment has been minimized.

Copy link
Contributor

raghavendersahdev commented Jul 29, 2018

Hi @davetcoleman , I have fixed this PR and removed the redundant commits added due to my wrong rebase earlier. This PR is now ready for review and Travis passes.

# of motion plan queries and start states in the Kitchen scene.

# Five planners from two different plugins are run a total of 50 times each, with a
# maximum of 10 seconds per run. Output is stored in the /home/benchmarks directory.

This comment has been minimized.

@davetcoleman

davetcoleman Jul 30, 2018

Member

Output is stored in the /home/benchmarks directory.

This is not true, right? Line 18 has a different location

This comment has been minimized.

@raghavendersahdev

raghavendersahdev Jul 31, 2018

Contributor

yes, you are right, fixed!

# of motion plan queries and start states in the Kitchen scene.

# Five planners from two different plugins are run a total of 50 times each, with a
# maximum of 10 seconds per run. Output is stored in the /home/benchmarks directory.

This comment has been minimized.

@davetcoleman

davetcoleman Jul 30, 2018

Member

same directory issue

This comment has been minimized.

@raghavendersahdev
// planning group otherwise an error occurs, because for OMPL a specific planning algorithm needs to be defined for
// a planning group, whereas with STOMP and CHOMP this is not necessary
if (pm->getDescription().compare("stomp") || pm->getDescription().compare("chomp"))
return true;

This comment has been minimized.

@davetcoleman

davetcoleman Jul 30, 2018

Member

this should be continue not return... in case OMPL is also in the list of planner interfaces to check

This comment has been minimized.

@raghavendersahdev

raghavendersahdev added some commits Jul 31, 2018

q

@davetcoleman davetcoleman merged commit e4937c4 into ros-planning:kinetic-devel Aug 1, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

davetcoleman added a commit that referenced this pull request Aug 1, 2018

Benchmarking with different Motion Planners (STOMP, CHOMP, OMPL) (#992)
* addition of launch files and yaml files for benchmarking to work with STOMP and CHOMP

* trick to make CHOMP and STOMP work with the existing benchmarks

* changed default output directory in yaml files

* applied clang format

* addressed PR requested changes

* fixed comment message

* fixed some bugs in comments, addressed PR requested changes

* q
@davetcoleman

This comment has been minimized.

Copy link
Member

davetcoleman commented Aug 1, 2018

cherry picked to melodic

MohmadAyman added a commit to MohmadAyman/moveit that referenced this pull request Aug 25, 2018

Benchmarking with different Motion Planners (STOMP, CHOMP, OMPL) (ros…
…-planning#992)

* addition of launch files and yaml files for benchmarking to work with STOMP and CHOMP

* trick to make CHOMP and STOMP work with the existing benchmarks

* changed default output directory in yaml files

* applied clang format

* addressed PR requested changes

* fixed comment message

* fixed some bugs in comments, addressed PR requested changes

* q
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment