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 SequenceConfigurationToConfigurationMetaPlanner #564

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

gilwoolee
Copy link
Contributor

This PR adds SequenceConfigurationToConfigurationMetaPlanner, in preparation for #467.

SequenceConfigurationToConfigurationMetaPlanner takes a list of ConfigurationToConfigurationPlanner and use them in sequence to solve a ConfigurationToConfiguration problem.

While a generic SequenceMetaPlanner can be used to serve this purpose, we need this planner to be of type ConfigurationToConfigurationPlanner in order to be used by PlannerAdapter ConfigurationToConfiguration_to_ConfigurationToTSR.

Later, we may want to make a templated SequenceMetaPlanner<T>, but I'd leave it as a future TODO as it adds complexity.

[Explain how this pull request was tested.]
TODO. I will test after the initial PR review.


Before creating a pull request

  • Document new methods and classes
  • Format code with make format

Before merging a pull request

  • Set version target by selecting a milestone on the right side
  • Summarize this change in CHANGELOG.md
  • Add unit test(s) for this change

@brianhou
Copy link
Contributor

I think the current way that we've been using this in libherb is to have a sequence of [ConfigToConfig, ConfigToTSR] where ConfigToTSR is an explicit adaptation of the ConfigToConfig.

It seems like the new way would be a sequence of [[ConfigToConfig]s, [ConfigToTSR]s], where the first is of type SequenceConfigurationToConfigurationMetaPlanner and the second is an explicit adaption. Is that correct?

@gilwoolee
Copy link
Contributor Author

@brianhou I'm not sure I fully followed.

the current way we've been using this in libherb is to have a sequence of [ConfigToConfig, ConfigToTSR] where ConfigToTSR is an explicit adaptation of the ConfigToConfig.

Let me just share how I was initially hoping to use this. ConcreteRobot currently has two potential places for SequenceMetaPlanner. One is PlanToConfiguration, where we currently call Snap and RRTConnect in sequence. This can be replaced by a generic SequenceMetaPlanner (code example).

The other place is for PlanToTSR (I think this is what you're referring to), where we (1) sample a bunch of configs (2) try all of them w/ Snap (3) try all of them w/ RRTConnect. This is where we might need this, if we choose to follow the spec of PlanToTSR regarding the ranking. That is, we want to actually try our best w/ all possible planners per configuration before trying the next one, we'd need this planner. Here's how I initially thought we'd use it.

That said, it's against the fail-fast policy, so I don't think it'd be wise to do it this way for PlanToTSR. I still don't quite like the long code for PlanToTSR, so how about calling SnapConfigurationToConfigurations and RRTConfigurationToConfigurations in sequence?

Regardless, we'd need a similar solution to this if we want to pass any single problem meta planner into a dart::PlannerAdapter, which I thought was one of the main use cases meta planners. I'm happy to table this PR if unnecessary. :)

@aditya-vk aditya-vk removed their request for review September 17, 2021 03:55
@codecov
Copy link

codecov bot commented Sep 17, 2021

Codecov Report

Merging #564 (47d6ed5) into master (ce7dd79) will decrease coverage by 0.09%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master     #564      +/-   ##
==========================================
- Coverage   73.07%   72.97%   -0.10%     
==========================================
  Files         219      220       +1     
  Lines        7865     7875      +10     
==========================================
  Hits         5747     5747              
- Misses       2118     2128      +10     
Impacted Files Coverage Δ
...equenceConfigurationToConfigurationMetaPlanner.cpp 0.00% <0.00%> (ø)

@egordon egordon self-requested a review as a code owner November 23, 2022 06:23
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.

3 participants