This repository has been archived by the owner. It is now read-only.

Enables setting optimization objectives from ompl_planning.yaml #75

Merged
merged 4 commits into from Jul 12, 2016

Conversation

Projects
None yet
4 participants
@rbbg
Contributor

rbbg commented Jul 8, 2016

Not sure if this is the best way to do it, but I have seen it requested a number of times and I think it's a fairly minor change.

@v4hn

This comment has been minimized.

Show comment
Hide comment
@v4hn

v4hn Jul 8, 2016

Member

Is this based on an option already supported in ompl_planning.yaml?
If it is not, I would really prefer to have the actual names of the objectives in the config instead of a magic number.

Member

v4hn commented Jul 8, 2016

Is this based on an option already supported in ompl_planning.yaml?
If it is not, I would really prefer to have the actual names of the objectives in the config instead of a magic number.

@rbbg

This comment has been minimized.

Show comment
Hide comment
@rbbg

rbbg Jul 8, 2016

Contributor

Yes that does seem better. Thanks.

One other question: It seems to me like the default PathLengthOptimization is optimizing the 1-norm distance in jointspace when used by MoveIt!. Is this by choice? The 1-norm seems to be a little odd as a default optimization objective, at least from my point of view.

Contributor

rbbg commented Jul 8, 2016

Yes that does seem better. Thanks.

One other question: It seems to me like the default PathLengthOptimization is optimizing the 1-norm distance in jointspace when used by MoveIt!. Is this by choice? The 1-norm seems to be a little odd as a default optimization objective, at least from my point of view.

Show outdated Hide outdated ompl/ompl_interface/src/model_based_planning_context.cpp
{
logInform("%s: No optimization objective specified, defaulting to PathLengthOptimizationObjective");
objective.reset(new ompl::base::PathLengthOptimizationObjective(ompl_simple_setup_->getSpaceInformation()));
}

This comment has been minimized.

@v4hn

v4hn Jul 8, 2016

Member

could you rewrite the default case to something like this:

std::string objective;
it = cfg.find("optimization_objective");
if (it == cfg.end())
{
   objective = "PathLengthOptimizationObjective";
   logInform(" ... defaulting to " + objective); // with appropriate string type conversions I suppose...
}
else
{
  objective = it->second;
}

This avoids redundancy and the big else-block afterwards.

@v4hn

v4hn Jul 8, 2016

Member

could you rewrite the default case to something like this:

std::string objective;
it = cfg.find("optimization_objective");
if (it == cfg.end())
{
   objective = "PathLengthOptimizationObjective";
   logInform(" ... defaulting to " + objective); // with appropriate string type conversions I suppose...
}
else
{
  objective = it->second;
}

This avoids redundancy and the big else-block afterwards.

@v4hn

This comment has been minimized.

Show comment
Hide comment
@v4hn

v4hn Jul 8, 2016

Member

It seems to me like the default PathLengthOptimization is optimizing the 1-norm distance in jointspace when used by MoveIt!. Is this by choice?

I don't know @davetcoleman is our OMPL expert afaik :-)
But what norm would you prefer in joint space?

Member

v4hn commented Jul 8, 2016

It seems to me like the default PathLengthOptimization is optimizing the 1-norm distance in jointspace when used by MoveIt!. Is this by choice?

I don't know @davetcoleman is our OMPL expert afaik :-)
But what norm would you prefer in joint space?

@rbbg

This comment has been minimized.

Show comment
Hide comment
@rbbg

rbbg Jul 8, 2016

Contributor

Well, not sure what that was based on, but I always kind of assumed that it was using the 2-norm to begin with. I don't think the 2-norm is better, but I've talked to at least a couple of other people that had made the same assumption, hence the question.

Contributor

rbbg commented Jul 8, 2016

Well, not sure what that was based on, but I always kind of assumed that it was using the 2-norm to begin with. I don't think the 2-norm is better, but I've talked to at least a couple of other people that had made the same assumption, hence the question.

@davetcoleman

This comment has been minimized.

Show comment
Hide comment
@davetcoleman

davetcoleman Jul 8, 2016

Member

Yea, I was surprised recently by the discovery it was using 1-norm also - it really threw off some assumptions I was making in my research! I then assumed that in joint space, 1-norm is the only distance metric that makes sense? I tried to find some publications to back this up, but was unsuccessful. @isucan @mamoll - why does the MoveIt! distance function used in OMPL only use the 1-norm?

Member

davetcoleman commented Jul 8, 2016

Yea, I was surprised recently by the discovery it was using 1-norm also - it really threw off some assumptions I was making in my research! I then assumed that in joint space, 1-norm is the only distance metric that makes sense? I tried to find some publications to back this up, but was unsuccessful. @isucan @mamoll - why does the MoveIt! distance function used in OMPL only use the 1-norm?

@davetcoleman

This comment has been minimized.

Show comment
Hide comment
@davetcoleman

davetcoleman Jul 8, 2016

Member

Back to this PR: thanks for creating this PR! Could you provide some documentation for users who want to use this exposed configuration? It would be best if you added a folder to this repo: moveit_planners/ompl/ompl_interface/tutorials/doc/tutorial.rst and we could then link to it from [http://moveit.ros.org/documentation/tutorials/]. Its ok if the tutorial is really short for now, it would be a good start. For reference see here

Member

davetcoleman commented Jul 8, 2016

Back to this PR: thanks for creating this PR! Could you provide some documentation for users who want to use this exposed configuration? It would be best if you added a folder to this repo: moveit_planners/ompl/ompl_interface/tutorials/doc/tutorial.rst and we could then link to it from [http://moveit.ros.org/documentation/tutorials/]. Its ok if the tutorial is really short for now, it would be a good start. For reference see here

@mamoll

This comment has been minimized.

Show comment
Hide comment
@mamoll

mamoll Jul 11, 2016

@davetcoleman, the distance function you link to computes the distance for a single joint. The real "problem" arises when you try to combine distances from individual joints. The current approach is (most likely) to simply add the distances over individual joints together. This is simple and computationally efficient. More important than 1-norm vs. 2-norm is probably how the individual joint distances are weighted. A shoulder displacement typically has a much larger effect than bending a finger. Is this accounted for in MoveIt!?

It is not obvious to me what the "right" distance measure should be articulated mechanisms (or whether 2-norm would somehow be better than 1-norm). Swept volume seems like a reasonable choice, but tends to be expensive to compute. In my experience, motion planning algorithms that depend heavily on a distance metric tend to scale badly with increasing number of degrees of freedom.

mamoll commented Jul 11, 2016

@davetcoleman, the distance function you link to computes the distance for a single joint. The real "problem" arises when you try to combine distances from individual joints. The current approach is (most likely) to simply add the distances over individual joints together. This is simple and computationally efficient. More important than 1-norm vs. 2-norm is probably how the individual joint distances are weighted. A shoulder displacement typically has a much larger effect than bending a finger. Is this accounted for in MoveIt!?

It is not obvious to me what the "right" distance measure should be articulated mechanisms (or whether 2-norm would somehow be better than 1-norm). Swept volume seems like a reasonable choice, but tends to be expensive to compute. In my experience, motion planning algorithms that depend heavily on a distance metric tend to scale badly with increasing number of degrees of freedom.

@davetcoleman

This comment has been minimized.

Show comment
Hide comment
@davetcoleman

davetcoleman Jul 12, 2016

Member

@mamoll you are right that the current approach is to simply "add the distances over the individual joints together". @isucan added a notion of distance_factor_ but for all joint types it is always set to just 1.

A shoulder displacement typically has a much larger effect than bending a finger. Is this accounted for in MoveIt!?

In MoveIt! all joints are weighted the same, there there is a related notion of a projection_evaluator inside moveit_planners_ompl that takes i.e. the shoulder join's translation and uses it in some planners for better distance functions (I think).

Thanks for all that insight @mamoll, very interesting!

Member

davetcoleman commented Jul 12, 2016

@mamoll you are right that the current approach is to simply "add the distances over the individual joints together". @isucan added a notion of distance_factor_ but for all joint types it is always set to just 1.

A shoulder displacement typically has a much larger effect than bending a finger. Is this accounted for in MoveIt!?

In MoveIt! all joints are weighted the same, there there is a related notion of a projection_evaluator inside moveit_planners_ompl that takes i.e. the shoulder join's translation and uses it in some planners for better distance functions (I think).

Thanks for all that insight @mamoll, very interesting!

@davetcoleman

This comment has been minimized.

Show comment
Hide comment
@davetcoleman

davetcoleman Jul 12, 2016

Member

This tutorial looks great, thanks! Travis failed due to a rosdep error - I just retriggered it. When it passes +1

Member

davetcoleman commented Jul 12, 2016

This tutorial looks great, thanks! Travis failed due to a rosdep error - I just retriggered it. When it passes +1

@davetcoleman davetcoleman merged commit fb8ef47 into ros-planning:indigo-devel Jul 12, 2016

1 check passed

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

This comment has been minimized.

Show comment
Hide comment
@davetcoleman

davetcoleman Jul 12, 2016

Member

We need to add a link to this tutorial here once rosdoc_lite generates the new webpage (takes about 24 hours)

Member

davetcoleman commented Jul 12, 2016

We need to add a link to this tutorial here once rosdoc_lite generates the new webpage (takes about 24 hours)

@davetcoleman

This comment has been minimized.

Show comment
Hide comment
@davetcoleman

davetcoleman Jul 13, 2016

Member

We forgot the additional steps to turn on rosdoc, added here: #76

Member

davetcoleman commented Jul 13, 2016

We forgot the additional steps to turn on rosdoc, added here: #76

@davetcoleman

This comment has been minimized.

Show comment
Hide comment
@davetcoleman

davetcoleman Jul 18, 2016

Member

Cherry-picked to jade & kinetic

Member

davetcoleman commented Jul 18, 2016

Cherry-picked to jade & kinetic

@davetcoleman

This comment has been minimized.

Show comment
Hide comment
@davetcoleman

davetcoleman Jul 26, 2016

Member

I've added a link to the documentation here ros-planning/moveit.ros.org#19

Member

davetcoleman commented Jul 26, 2016

I've added a link to the documentation here ros-planning/moveit.ros.org#19

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.