Improve edge evaluator interface to support compound edge evaluator #26

Levi-Armstrong · 2020-10-02T16:52:47Z

This reduces code duplication between edge evaluators and enables the ability for the compound edge evaluator to work correctly.

marip8

Nominally I think this is a good change. It's worth thinking about making the two interface function definitions (evaluate and considerEdge) as simple and intuitive as possible.

We really also need to comment these classes and what they're intended to be used for. There is a lot of ambiguity on the types of costs that these evaluators should return (i.e. L1 or L2 distance) and the assumptions they make (i.e. do they consider timing, axis order, etc.). We shouldn't have to dig into the source code to understand what these classes/functions do

descartes_light/include/descartes_light/interface/edge_evaluator.h

marip8 · 2020-10-03T00:09:50Z

descartes_light/include/descartes_light/interface/edge_evaluator.h

@@ -32,13 +32,59 @@ template <typename FloatType>
 class EdgeEvaluator
 {
 public:
+  using Ptr = typename std::shared_ptr<EdgeEvaluator<FloatType>>;
+  EdgeEvaluator(std::size_t dof) : dof_(dof) {}
  virtual ~EdgeEvaluator() {}

  virtual bool evaluate(const Rung_<FloatType>& from,


I don't really like the function signature here. Why does this function return false if the two rungs aren't connected by any vertices? It's not like the evaluation itself failed. I would propose returning the vector of edge lists instead; then let the user decide what it means to have a vector of edge lists that is empty.

std::vector<typename LadderGraph<FloatType>::EdgeList> evaluate(const Rung_<FloatType>& from, const Rung_<FloatType>& to)

Why does this function return false if the two rungs aren't connected by any vertices? It's not like the evaluation itself failed. I would propose returning the vector of edge lists instead; then let the user decide what it means to have a vector of edge lists that is empty.

This is an interface class that is only to used by the descartes code which keeps the building of the graph cleaner. The user should not directly be calling evaluate or considerEdge.

In this case, the "user" I'm referring to is actually us as developers, not necessarily end-users. I think a change to this interface would clear up the ambiguity with edges resizing we're discussing above. It would be really easy to create a helper function that can look through the output list of edges and determine if it is empty or not to replace the boolean check here. This wouldn't introduce much additional code to the graph construction and would improve the clarity of the edge evaluator interface

Ok, make sense. Lets create an issue on this and make the change in a separate PR. You good with this?

descartes_samplers/include/descartes_samplers/evaluators/impl/distance_edge_evaluator.hpp

marip8 · 2020-10-03T00:50:43Z

...lers/include/descartes_samplers/evaluators/impl/gantry_euclidean_distance_edge_evaluator.hpp

-    if (!rung.empty())
-      return true;
+  FloatType cost = 0.0;
+  for (std::size_t i = 2; i < this->dof_; ++i)


There are a lot of assumptions being made here:

The gantry has two external axes

The gantry axes are the first two values in the list

The joint distance of the gantry axes is not considered

We need to add these assumptions to the documentation at the very least. Ultimately, we should probably parameterize them with the arguments to the constructor.

Why is the motion of the first two axes of the gantry irrelevant here?

Based on experience it works better to minimize robot motion and let the gantry do most of the work if it can. Though this is old and not used anymore. I am good with marking it deprecated or removing it completely.

We don't necessarily need to delete it (unless you really think it needs to go); we just need to document these assumptions on the class definition so we can easily remember how it works

I think it has little value since most will likely use there own because MoveIt and Tesseract have there own way of defining kinematics. Tesseract implements its own which allow any number of gantry joints and I would assume if other motion planning libraries would do the same.

I update your Issue #5 to include documentation items that need to be addressed.

Levi-Armstrong · 2020-10-03T03:35:49Z

@marip8 I made most of the changes but there is a few I commented on to further discuss.

Once the review is finished I will squash everything into a single commit.

Levi-Armstrong · 2020-10-03T18:57:46Z

@johnwason Do you know how to fix the windows build?

johnwason · 2020-10-03T21:41:23Z

It looks like you are using the old ROS Windows repository with the azurewebsites url. They have started using a new one recently. You should upgrade to the latest install instructions:

http://wiki.ros.org/Installation/Windows

I also suggest using the github for building Windows, since they are using powershell instead of git bash for the environment. The ROS Windows has build in powershell support.

Levi-Armstrong · 2020-10-05T13:42:24Z

It looks like you are using the old ROS Windows repository with the azurewebsites url. They have started using a new one recently. You should upgrade to the latest install instructions:

http://wiki.ros.org/Installation/Windows

I also suggest using the github for building Windows, since they are using powershell instead of git bash for the environment. The ROS Windows has build in powershell support.

@johnwason Are you currently using this in any of your repository that I may use as a guide?

Levi-Armstrong · 2020-10-05T17:16:25Z

@marip8 I believe I have addressed the remaining issues in the latest commit. I removed the timing constraint and simplified the edge evaluator API. I also updated the issue on documentation related to include items that need to be addressed. I may have time in the near future to go through and add documentation to this repository.

marip8 · 2020-10-05T21:23:27Z

I think we should handle the timing constraint a little differently. I think we have 3 options:

Make timing a standalone evaluator
- All other edge evaluator implementations should be independent of timing
Make timing a parameter of the considerEdge interface (with default value = inf)
- Edge evaluators can choose whether or not they consider timing
- Con:
  - It's could be unclear to an end-user whether or not timing is being considered if it's not documented well
  - You might need to duplicate edge evaluators such that one considers timing and the second doesn't
Make timing an argument to the constructor of the edge evaluator implementation
- Cons:
  - You might need to duplicate edge evaluators such that one considers timing and the second doesn't

I prefer option 1 (standalone timing evaluator) because it can be easily combined with any other evaluator with the CompoundEdgeEvaluator and it eliminates ambiguity in the other evaluators about whether or not they consider timing.

Depending on how quickly you need this functionality, we could make this an issue address it in the future. If you're on board with option 1, though, it might be nice to knock it out with this PR

johnwason · 2020-10-05T22:10:10Z

@Levi-Armstrong Sorry no, I don't have a unit test using ROS Windows.

Levi-Armstrong · 2020-10-05T22:34:19Z

@marip8 I made the changes per our discussion, let me know if you see any issues.

…st calculation

marip8 · 2020-10-05T23:24:56Z

descartes_samplers/include/descartes_samplers/evaluators/impl/timing_edge_evaluator.hpp

+  Eigen::Matrix<FloatType, Eigen::Dynamic, 1> delta =
+      Eigen::Map<const Eigen::Matrix<FloatType, Eigen::Dynamic, 1>>(end, static_cast<Eigen::Index>(this->dof_)) -
+      Eigen::Map<const Eigen::Matrix<FloatType, Eigen::Dynamic, 1>>(start, static_cast<Eigen::Index>(this->dof_));
+  Eigen::Matrix<FloatType, Eigen::Dynamic, 1> joint_times = delta.cwiseQuotient(velocity_limits_).cwiseAbs();


FYI you can use Eigen::Array instead of Eigen::Matrix to do coefficient-wise operations "natively" without calling cwise...

I assumed they did the same thing under the hood. Is there a benefit for one over the other?

Eigen::Matrix is for linear algebra operations whereas Eigen::Array is for coefficient-wise operations. According to the documentation, you can convert one to the other with no run-time cost. I think it's just more convenient to use one over the other depending on what type of operations you're trying to do

johnwason · 2020-10-05T23:59:51Z

@ooeygui Can you help Levi with his question about CI with the updated ROS install instructions?

ooeygui · 2020-10-06T16:32:09Z

@johnwason Thanks for loop in - I have an email thread with Levi on the CI.

johnwason · 2020-10-06T20:39:24Z

@ooeygui I am interested in the solution to this problem as well.

Levi-Armstrong requested a review from marip8 October 2, 2020 16:53

marip8 requested changes Oct 3, 2020

View reviewed changes

Levi-Armstrong force-pushed the issue/FixCompoundEvaluator branch from 60aa1e8 to dea08d6 Compare October 3, 2020 03:46

Levi-Armstrong added 2 commits October 5, 2020 12:14

Improve edge evaluator interface to support compound edge evaluator

45ad500

Remove TimingConstraint and simplify edge evaluator interface

d4877ef

Levi-Armstrong force-pushed the issue/FixCompoundEvaluator branch from 8da5322 to d4877ef Compare October 5, 2020 17:14

Rename distance edge evaluator to timing edge evaluator and change co…

e374060

…st calculation

Levi-Armstrong force-pushed the issue/FixCompoundEvaluator branch from 7696e8c to e374060 Compare October 5, 2020 22:41

marip8 reviewed Oct 5, 2020

View reviewed changes

marip8 approved these changes Oct 5, 2020

View reviewed changes

Levi-Armstrong merged commit 81bd3f0 into swri-robotics:feature/CommandLanguage Oct 5, 2020

Levi-Armstrong deleted the issue/FixCompoundEvaluator branch October 5, 2020 23:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve edge evaluator interface to support compound edge evaluator #26

Improve edge evaluator interface to support compound edge evaluator #26

Levi-Armstrong commented Oct 2, 2020

marip8 left a comment

marip8 Oct 3, 2020

Levi-Armstrong Oct 3, 2020

marip8 Oct 5, 2020

Levi-Armstrong Oct 5, 2020

marip8 Oct 3, 2020

Levi-Armstrong Oct 3, 2020 •

edited

Loading

marip8 Oct 5, 2020

Levi-Armstrong Oct 5, 2020

Levi-Armstrong Oct 5, 2020

Levi-Armstrong commented Oct 3, 2020 •

edited

Loading

Levi-Armstrong commented Oct 3, 2020 •

edited

Loading

johnwason commented Oct 3, 2020

Levi-Armstrong commented Oct 5, 2020

Levi-Armstrong commented Oct 5, 2020

marip8 commented Oct 5, 2020 •

edited

Loading

johnwason commented Oct 5, 2020

Levi-Armstrong commented Oct 5, 2020

marip8 Oct 5, 2020

Levi-Armstrong Oct 5, 2020

marip8 Oct 6, 2020

johnwason commented Oct 5, 2020

ooeygui commented Oct 6, 2020

johnwason commented Oct 6, 2020

Improve edge evaluator interface to support compound edge evaluator #26

Improve edge evaluator interface to support compound edge evaluator #26

Conversation

Levi-Armstrong commented Oct 2, 2020

marip8 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Levi-Armstrong Oct 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Levi-Armstrong commented Oct 3, 2020 • edited Loading

Levi-Armstrong commented Oct 3, 2020 • edited Loading

johnwason commented Oct 3, 2020

Levi-Armstrong commented Oct 5, 2020

Levi-Armstrong commented Oct 5, 2020

marip8 commented Oct 5, 2020 • edited Loading

johnwason commented Oct 5, 2020

Levi-Armstrong commented Oct 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnwason commented Oct 5, 2020

ooeygui commented Oct 6, 2020

johnwason commented Oct 6, 2020

Levi-Armstrong Oct 3, 2020 •

edited

Loading

Levi-Armstrong commented Oct 3, 2020 •

edited

Loading

Levi-Armstrong commented Oct 3, 2020 •

edited

Loading

marip8 commented Oct 5, 2020 •

edited

Loading