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

Added separate LockedPlanningMethod and ClonedPlanningMethod calls. #280

Merged
merged 9 commits into from
Apr 1, 2016

Conversation

psigen
Copy link
Member

@psigen psigen commented Mar 25, 2016

This should be merged after #278 and #279.

This separates the @PlanningMethod decorator into two decorators:

  • @LockedPlanningMethod locks the current robot environment and executes the wrapped method.
  • @ClonedPlanningMethod clones the current robot environment and executes the wrapped method.

This distinction is useful for planning calls that are so fast that they take less time than the cloning operation itself, such as optimistic plans like SnapPlanner and fast mathematical solutions such as KunzCircularBlender.

This commit refactors all existing planning calls except SnapPlanner to use @ClonedPlanningMethod and updates the README to explain the distinction. @PlanningMethod is changed to inherit from @ClonedPlanningMethod but print a warning if it is used.

psigen added 2 commits March 25, 2016 17:29
This separates the @PlanningMethod decorator into two decorators:

- @LockedPlanningMethod locks the current robot environment and
  executes the wrapped method.
- @ClonedPlanningMethod clones the current robot environment and
  executes the wrapped method.

This distinction is useful for planning calls that are so fast that
they take less time than the cloning operation itself, such as
optimistic plans like `SnapPlanner` and fast mathematical solutions
such as `KunzCircularBlender`.

This commit refactors all existing planning calls except `SnapPlanner`
to use `@ClonedPlanningMethod` and updates the README to explain
the distinction.  `@PlanningMethod` is changed to inherit from
`@ClonedPlanningMethod` but print a warning if it is used.
parallel and to paralellize planning and execution. In general, locked
planning methods are used for calls that will terminate extremely quickly,
while cloned planning methods are used for calls that might take a significant
amount of time.
Copy link
Member

Choose a reason for hiding this comment

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

This description does not clearly articulate the difference between the two decorators. For example, it currently implies that @LockedPlanningMethod allows PrPy to parallelize multiple planners. I think you should reword this.

Copy link
Member

Choose a reason for hiding this comment

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

Typo: paralellize

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure I'll fix your typos 😉
854b6f3

@mkoval
Copy link
Member

mkoval commented Mar 26, 2016

I am generally supportive of this change.

My only concern is about the potential confusion introduced by planner.env. This environment is created in the PlannerBase class, even in sub-class that only use @LockedPlanningMethod. We also explicitly clone into planner.env inside the Ranked meta planner, which is unnecessary for @LockedPlanningMethods.

I am not sure what the best way to address this is. At a minimum, we need to clearly document that @*PlanningMethods must return a Trajectory in the same Environment as robot.GetEnv(). I believe this handles both cases.

@psigen
Copy link
Member Author

psigen commented Mar 26, 2016

I made some doc changes in 788d1d5 to clarify the above.

I think it's largely harmless to have PlannerBase create a single env per planner, because most planners will have cloned methods or a mix of cloned and locked methods, and I would rather not have to divide the PlannerBase up and have to switch classes over when they start having ClonedPlanningMethods.

Metaplanners will probably need to always use cloning for now, because they need to lock an env to maintain ActiveDOFs and other initial configuration to start up other planners, and there is currently no way to lock an env just until it has been cloned by a few planners. It is very tricky to synchronize this otherwise.

@mkoval
Copy link
Member

mkoval commented Mar 27, 2016

👍 Thanks for improving the documentation. I will merge this once Travis finishes on #278.

@mkoval
Copy link
Member

mkoval commented Mar 30, 2016

@psigen This will not merge cleanly. I suspect that it conflicts with something in #278. Could you resolve this on the command line?

@psigen
Copy link
Member Author

psigen commented Mar 31, 2016

Not sure what the issue was, it merged just fine on command-line with no input.
@mkoval: Should be good to merge now.

@mkoval
Copy link
Member

mkoval commented Apr 1, 2016

Finally got Travis to run on this. LGTM.

@mkoval mkoval merged commit eadd1b9 into master Apr 1, 2016
@mkoval mkoval deleted the feature/more_planningmethods branch April 1, 2016 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants