ConcreteRobot uses ranker for PlantoTSR

[gilwoolee]

This PR updates ConcreteRobot class to use ConfigurationRanker when calling planToTSR. It changes the planToTSR logic a little bit accordingly.

---

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

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (master@fe6ba16). Click here to learn what that means.
The diff coverage is 0%.

@@            Coverage Diff            @@
##             master     #503   +/-   ##
=========================================
  Coverage          ?   77.66%           
=========================================
  Files             ?      262           
  Lines             ?     6784           
  Branches          ?        0           
=========================================
  Hits              ?     5269           
  Misses            ?     1515           
  Partials          ?        0

Impacted Files	Coverage Δ
src/distance/ConfigurationRanker.cpp	100% <ø> (ø)
src/planner/dart/ConfigurationToTSRPlanner.cpp	100% <ø> (ø)
...ude/aikido/distance/NominalConfigurationRanker.hpp	100% <ø> (ø)
.../aikido/planner/dart/ConfigurationToTSRPlanner.hpp	100% <ø> (ø)
include/aikido/distance/ConfigurationRanker.hpp	100% <ø> (ø)
...igurationToConfiguration_to_ConfigurationToTSR.hpp	100% <ø> (ø)
...igurationToConfiguration_to_ConfigurationToTSR.cpp	6% <0%> (ø)

[aditya-vk]

Thanks for the PR! We should also probably update the new planner API: ConfigurationToTSR and ConfigurationToConfiguration_to_ConfigurationToTSR to have a ranker?

[gilwoolee]

Thanks for the PR! We should also probably update the new planner API: ConfigurationToTSR and ConfigurationToConfiguration_to_ConfigurationToTSR to have a ranker?

They already are. Ranker is one of the constructor arguments in ConfigurationToConfigurationPlanner and ConfigurationToConfiguration_to_ConfigurationToTSR as this is a property of the planner not the problem. I changed them to ConstConfigurationRanker as well.

[brianhou]

Looks mostly good! Just a couple minor questions.

[aditya-vk]

Looks great!

[brianhou]

Actually, I think we should update the dart::ConfigurationToConfiguration_to_ConfigurationToTSR adapter to use the ranker? Also, we need to update the changelog.

[gilwoolee]

Actually, I think we should update the dart::ConfigurationToConfiguration_to_ConfigurationToTSR adapter to use the ranker? Also, we need to update the changelog.

@brianhou it was already using a ranker. I just changed it to use the Const version.

[brianhou]

I don't think the plan method actually uses the ranker anywhere, though?

aikido/src/planner/dart/ConfigurationToConfiguration_to_ConfigurationToTSR.cpp

Line 88 in 41032c7

while (generator->canSample())

[gilwoolee]

I don't think the plan method actually uses the ranker anywhere, though?

aikido/src/planner/dart/ConfigurationToConfiguration_to_ConfigurationToTSR.cpp

Line 88 in 41032c7

while (generator->canSample())

lol!!! haha since I didn't make that change in this PR I didn't check... Let me fix that :P

[gilwoolee]

@brianhou @aditya-vk please take another look!

[aditya-vk]

Why do we need to create another pointer here? Can we not do:

if (!mConfigurationRanker)
{
    mConfigurationRanker = NominalConfigurationRanker;
}

In robot/utils/planToTSR, the ranker has been taken with a const pointer so I guess that's fine there. Alternatively, we can leave this like this and change the data member to const since we are initializing it in the constructor anyway.

[gilwoolee]

Summarizing my slack discussion with @aditya-vk , I think we should keep it as is, for the following reasons:

1. if the planner is created with mConfigurationRanker = nullptr, the expectation is that a NominalConfigurationRanker would be created when this method is called and be initialized with the startState. So if we change the member variable as you suggested, the planner now has been mutated and it cannot be reused in the same way. Next time it is called, it'd use this ranker.
2. To fix, the ideal solution would be the second way you suggested, to have it as const mConfigurationRanker so that it never gets changed. Unfortunately, to my best understanding, because this class becomes a TSRPlanner via the templated adapter, it does not have access to mConfigurationRanker during the initialization, i.e. I can't do : mConfigurationRanker(std::move(ranker)) { ... } . It only has access to the constructor parameters of the PlannerAdapter, which is the stateSpace and metaSkeleton. See stackoverflow .

Hence, in order to keep the member variable as-is (as if it's a const) and initialize it in constructor if it's passed in, I think we need to keep it the way I have it now.

@aditya-vk @brianhou please confirm!

[aditya-vk]

That's right, there's no way to have it in the initialization list. Hmm.
I think what we have now is good then. Since we don't give an option to change the ranker at all unless a new planner is created, I think it should be fine 👍

[aditya-vk]

Left a nit comment, otherwise good to go! Travis seems to be complaining about formatting.

[aditya-vk]

LGTM!

Comment addressed