-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Prevent IrisInConfigurationSpaceFromCliqueCover from crashing the whole program if IrisInConfigurationSpace fails #21376
base: master
Are you sure you want to change the base?
Conversation
…le program if IrisInConfigurationSpace fails
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a minimal fix for #21343. We have a more graceful fix with @wernerpe which we will implement in the future, but it is part of a larger internal overhaul.
+@wernerpe for feature review.
cc @RussTedrake Can you try this on your example that was failing?
Reviewable status: LGTM missing from assignee wernerpe, needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes (waiting on @AlexandreAmice)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee wernerpe, needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes (waiting on @AlexandreAmice)
planning/iris/iris_from_clique_cover.cc
line 248 at r1 (raw file):
"Iris builder thread {} failed to compute a set from a clique. This " "clique is being skipped. This can happen if the convex hull of the " "clique contains collisions. The exact error message is {}",
Maybe we should add a pointer to double checking the collision checker padding:
"Iris builder thread {} failed to compute a set from a clique. This "
"clique is being skipped. This can happen if the entire clique is closer than configuration_space_margin to an obstacle or convex hull of the "
"clique contains collisions. The exact error message is {}",
Code quote:
"Iris builder thread {} failed to compute a set from a clique. This "
"clique is being skipped. This can happen if the convex hull of the "
"clique contains collisions. The exact error message is {}",
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee wernerpe, needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes (waiting on @AlexandreAmice)
planning/iris/iris_from_clique_cover.cc
line 248 at r1 (raw file):
Previously, wernerpe (Peter Werner) wrote…
Maybe we should add a pointer to double checking the collision checker padding:
"Iris builder thread {} failed to compute a set from a clique. This "
"clique is being skipped. This can happen if the entire clique is closer than configuration_space_margin to an obstacle or convex hull of the "
"clique contains collisions. The exact error message is {}",
I chose not to talk about the collision checker padding because if the obstacle you're close to is a 'configuration_space_obstacle' defined in the IrisOptions
then padding your collision_checker
won't fix this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee wernerpe, needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes (waiting on @AlexandreAmice)
planning/iris/iris_from_clique_cover.cc
line 248 at r1 (raw file):
Previously, AlexandreAmice (Alexandre Amice) wrote…
I chose not to talk about the collision checker padding because if the obstacle you're close to is a 'configuration_space_obstacle' defined in the
IrisOptions
then padding yourcollision_checker
won't fix this.
Wait but there is no stepback for cobstacles, right? https://github.com/RobotLocomotion/drake/blob/master/geometry/optimization/iris.cc#L696
What causes it to throw an error then?
It turns out the test that was failing (only on CI and only for a particular flavor of mac), was calling IrisInConfigurationSpace directly. The clique cover test wasn't failing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee wernerpe, needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes
planning/iris/iris_from_clique_cover.cc
line 248 at r1 (raw file):
Previously, wernerpe (Peter Werner) wrote…
Wait but there is no stepback for cobstacles, right? https://github.com/RobotLocomotion/drake/blob/master/geometry/optimization/iris.cc#L696
What causes it to throw an error then?
The thrown error happens here.
Basically, what happens is that you add a hyperplane, take a step back using the configuration space margin, and then your center ends up on the wrong side of the hyperplane.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee wernerpe, needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes (waiting on @AlexandreAmice)
planning/iris/iris_from_clique_cover.cc
line 248 at r1 (raw file):
Previously, AlexandreAmice (Alexandre Amice) wrote…
The thrown error happens here.
Basically, what happens is that you add a hyperplane, take a step back using the configuration space margin, and then your center ends up on the wrong side of the hyperplane.
I am confused, the stepback (called configuration_space_margin) is set to 0 for the cobstacles (configuration_obstacles) if I am not mistaken. This would imply that the above case is not happening?
Here is the line in the code again: https://github.com/RobotLocomotion/drake/blob/master/geometry/optimization/iris.cc#L696
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee wernerpe, needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes (waiting on @AlexandreAmice)
planning/iris/iris_from_clique_cover.cc
line 248 at r1 (raw file):
Previously, wernerpe (Peter Werner) wrote…
I am confused, the stepback (called configuration_space_margin) is set to 0 for the cobstacles (configuration_obstacles) if I am not mistaken. This would imply that the above case is not happening?
Here is the line in the code again: https://github.com/RobotLocomotion/drake/blob/master/geometry/optimization/iris.cc#L696
Sure, cspace obstacles won't let that happen. But task-space obstacles might.
In general, Iris can throw for a lot of reasons, and I don't want to lose 10 regions because one edge case got hit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee wernerpe, needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes (waiting on @AlexandreAmice)
planning/iris/iris_from_clique_cover.cc
line 248 at r1 (raw file):
Previously, AlexandreAmice (Alexandre Amice) wrote…
Sure, cspace obstacles won't let that happen. But task-space obstacles might.
In general, Iris can throw for a lot of reasons, and I don't want to lose 10 regions because one edge case got hit.
Yes, agreed. But I stand by my point it is not the manually added c-obstacles that are causing this crash. I dont see how they could - the step back is not required as they are assumed to be convex and is set to zero. We should get the try catch merged though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee wernerpe, needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes (waiting on @AlexandreAmice)
planning/iris/iris_from_clique_cover.cc
line 241 at r1 (raw file):
iris_region.emplace(IrisInConfigurationSpace( checker.plant(), checker.plant_context(builder_id), iris_options)); } catch (const std::logic_error& e) {
BTW Catching exceptions is forbidden:
https://drake.mit.edu/styleguide/cppguide.html#Exceptions
Possibly platform review will allow it as a temporary style violation as a step along the way to a better solution that doesn't catch exceptions, but for that approach at minimum you'd need to lay out the plan for how we'll eventually get rid of the catch
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee wernerpe, needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes
planning/iris/iris_from_clique_cover.cc
line 241 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
BTW Catching exceptions is forbidden:
https://drake.mit.edu/styleguide/cppguide.html#Exceptions
Possibly platform review will allow it as a temporary style violation as a step along the way to a better solution that doesn't catch exceptions, but for that approach at minimum you'd need to lay out the plan for how we'll eventually get rid of the
catch
.
What would be your recommended way of handling this case then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee wernerpe, needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes (waiting on @AlexandreAmice)
planning/iris/iris_from_clique_cover.cc
line 241 at r1 (raw file):
Previously, AlexandreAmice (Alexandre Amice) wrote…
What would be your recommended way of handling this case then?
If we want to programmatically handle errors from IrisInConfigurationSpace
then it would need to return the error information instead of throwing.
This change is