Profile Dictionary Update

[marip8]

Changes

• Replaced profile overrides with per-instruction remapping of profile names by namespace
  • The instruction getProfile method now accepts a namespace argument that defaults to an empty string. If the string is empty, the nominal profile is returned. If the namespace string is not empty, the overridden profile name is returned if it exists; otherwise the nominal profile name is returned
• Consolidated profile dictionary getter utilities with single getProfile function:
  • getProfile no longer returns a default profile. An exception is thrown if the named profile of the specified type cannot be found.
  • Error reporting was improved to make it easier to understand why the profile wasn't found
• Updated the motion planners to access profiles using the updated API
• Updated the task composer nodes to access profiles using the updated API
• Removed profile remapping from task composer input
• Updated unit tests
• Updated examples

Background

The move and composite instructions currently have a dependency on the profile dictionary which causes issues for the upcoming introduction of new profile interfaces and an updated profile dictionary (because the interfaces require information in move and composite instructions to produce motion planning profiles). Also, conceptually the profile dictionary belongs in the motion planning module. Most of this PR addresses the replacement of the current profile overrides (which were specified as a separate profile dictionary with different keys) with an optional profile name remapping for individual instructions.

After revising the profile dictionary, I also tried to simplify the API by consolidating several external helper functions (getProfileString, applyProfileOverrides, etc.) and add more error information for when profiles can't be found. I also removed the behavior of the profile dictionary of returning a default profile when the queried profile cannot be found. This is a change in the nominal behavior, but will help significantly in understanding whether the specified profiles were actually found and used during motion planning. In updating the examples, I found that most of the profile names specified in the program instructions did not have corresponding objects in the profile dictionary and that an arbitrary default profile was being used instead. I also added two helper functions for adding default planner and task composer profiles to a dictionary for the case where the default profiles are sufficient.

[marip8]

@Levi-Armstrong this PR should be ready for review.

The tesseract_examples/basic_cartesian_example unit tests are failing on the unstable and code coverage jobs. On my machine, I get the following error when it constructs the octomap from the point cloud:

tesseract_examples_basic_cartesian_example: /build/octomap-KdBKXz/octomap-1.9.3+dfsg/octomap/include/octomap/OcTreeDataNode.hxx:69: octomap::OcTreeDataNode<T>::~OcTreeDataNode() [with T = float]: Assertion `children == NULL' failed.

Any ideas on what might cause this? It doesn't seem to be related to any of the changes I made

[Levi-Armstrong]

Do you multiple versions installed?

[Levi-Armstrong]

Here is a PR which fixes the issue. I think we need to add the same logic to the pruneNode in tesseract_geometry

[Levi-Armstrong]

I tried adding the logic to our code but the object is protected so that is not going to work. Did any headers get arranged?

[marip8]

I tried adding the logic to our code but the object is protected so that is not going to work. Did any headers get arranged?

What object? The profile override setters and getters in the composite and move instructions should be public. I don't think I changed any header locations

[Levi-Armstrong]

I tried to add the following in the prune function, but children is not accessible.

        if (node->children != NULL){
          delete[] node->children;
          node->children = NULL;
        }

[marip8]

Let's continue the discussion about the octomap issue here since this PR hasn't made any changes that cause this issue.

[Levi-Armstrong]

I am about half way through the review. I think we should not hold this up due to the unit test failure. I am going to look into it this weekend and let you know what I find.

[Levi-Armstrong]

It looks like all of he profiles were removed. Was there a reason behind this?

[marip8]

None of them were ever actually added to the profile dictionary, so it was just pulling the default profile. That was the case in most of the examples actually. A lot of the changes I made in this PR were related to raising exceptions when requested profile names didn't have a corresponding profile entry in the dictionary.

[Levi-Armstrong]

@marip8 I was getting a different issue when running the example where it was failing due to missing profiles. After updating the cartesian example everything passes locally. I pushed my commit to your branch so we'll see if the tests pass now.

[Levi-Armstrong]

It looks like CI is passing now with my latest commit. I will finish my review early this week.

[codecov]

Codecov Report

Merging #255 (690bae1) into master (46df97e) will increase coverage by 0.20%.
The diff coverage is 77.58%.

@@            Coverage Diff             @@
##           master     #255      +/-   ##
==========================================
+ Coverage   58.61%   58.82%   +0.20%     
==========================================
  Files         214      211       -3     
  Lines        9729     9666      -63     
==========================================
- Hits         5703     5686      -17     
+ Misses       4026     3980      -46     

Impacted Files	Coverage Δ
...tesseract_command_language/composite_instruction.h	66.66% <ø> (ø)
...lude/tesseract_command_language/move_instruction.h	100.00% <ø> (ø)
...ore/include/tesseract_motion_planners/core/types.h	100.00% <ø> (ø)
...de/tesseract_task_composer/task_composer_problem.h	100.00% <ø> (ø)
...mposer/src/nodes/continuous_contact_check_task.cpp	15.87% <0.00%> (+0.72%)	⬆️
...sk_composer/src/nodes/fix_state_collision_task.cpp	32.33% <0.00%> (+0.28%)	⬆️
...ct_task_composer/src/nodes/motion_planner_task.cpp	64.28% <ø> (-1.24%)	⬇️
...ct_task_composer/src/nodes/profile_switch_task.cpp	0.00% <0.00%> (ø)
...ser/src/nodes/ruckig_trajectory_smoothing_task.cpp	0.00% <0.00%> (ø)
...r/src/nodes/time_optimal_parameterization_task.cpp	0.00% <0.00%> (ø)
... and 24 more

[marip8]

It seems like most of your comments are around throwing exceptions. These changes were mostly patches to the existing profile dictionary to remove the behavior of returning of default profiles when no profiles were actually added to the dictionary for a specific profile name. When we change the profile interfaces, we will introduce a new profile dictionary and the changes to the existing profile dictionary will go away. I would prefer not to reintroduce hasProfile here, because it seems like wasted effort. We can keep exception throwing in mind in the next profile dictionary.

I personally don't see the exceptions in this PR as a huge performance hit because most of them are only caught in order to add more information and then be rethrown. The only place where that is not the case is in the time parameterization tasks, and we could change that in future with a hasProfile method.

[marip8]

@marip8 Do you have a tesseract_ros branch with updates?

No I do not

[marip8]

@marip8 I was getting a different issue when running the example where it was failing due to missing profiles. After updating the cartesian example everything passes locally. I pushed my commit to your branch so we'll see if the tests pass now.

Must have missed those. I think that's another case where you really didn't have to specify custom profiles for any of the instructions. I would actually suggest removing the custom profile names to make it less confusing

[Levi-Armstrong]

It seems like most of your comments are around throwing exceptions. These changes were mostly patches to the existing profile dictionary to remove the behavior of returning of default profiles when no profiles were actually added to the dictionary for a specific profile name. When we change the profile interfaces, we will introduce a new profile dictionary and the changes to the existing profile dictionary will go away. I would prefer not to reintroduce hasProfile here, because it seems like wasted effort. We can keep exception throwing in mind in the next profile dictionary.

I personally don't see the exceptions in this PR as a huge performance hit because most of them are only caught in order to add more information and then be rethrown. The only place where that is not the case is in the time parameterization tasks, and we could change that in future with a hasProfile method.

Sounds good.

[marip8]

Are there any planning time benchmarks? If not, it would be interesting to try to convert some of the planning examples to benchmarks where we can measure planning time. Then it could help us justify decisions about throwing or not throwing exceptions. My hunch is that the exceptions will generally only be thrown in cases where a planning problem is not set up correctly and they won't otherwise be thrown, so we won't see a time penalty

[Levi-Armstrong]

My hunch is that the exceptions will generally only be thrown in cases where a planning problem is not set up correctly and they won't otherwise be thrown, so we won't see a time penalty

I don't think we currently have any in tesseract_planning. I have done internal benchmarking with exception and to use them for logical flow like what is done in time parameterization it is significant. It is only significant when an exception is thrown and caught. If no exception is thrown then you will not have a penalty so as long as they are not being used for logical flow they should be fine.

[Levi-Armstrong]

I found my messages. In my test case using godbolt it was 100 times slower to use exceptions for logical flow versus if, else for a simple test case. I would assume this increases based on the stack size it has to unwind so most use case would be larger than 100x penalty.

18.2991 if, else
18.1554 try, catch no exception thrown
2456.4 try, catch exception thrown

https://mortoray.com/the-true-cost-of-zero-cost-exceptions/

[marip8]

I'm on the same page about not using exceptions for logic handling where possible. For reference, do you have use cases where you are trying to run full planning pipelines at high frequency? Or are you running a single pipeline with one or more steps that run repeatedly at high frequency (e.g., online planning)?

[Levi-Armstrong]

I'm on the same page about not using exceptions for logic handling where possible. For reference, do you have use cases where you are trying to run full planning pipelines at high frequency? Or are you running a single pipeline with one or more steps that run repeatedly at high frequency (e.g., online planning)?

I don't have a public example, but a good example would be the package singulation demo where it is freespace and has a small cartesian component at the end before it picks up the package.

[Levi-Armstrong]

I will merge after the tesseract_ros PR is ready to merge.

[Levi-Armstrong]

@marip8 Do you want me to squash merge this?

[marip8]

That works for me; this should be complete as far as this PR goes

[Levi-Armstrong]

@marip8 It is merged into feat/ProfileRefactor which you can use to create PR's against as you work on the Profile Refactoring.