-
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
Lower bound in GcsTrajectoryOptimization subspace constraint should be -inf. #21384
base: master
Are you sure you want to change the base?
Conversation
Would +@wrangelvid be a good feature reviewer for this one? |
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.
Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee wrangelvid, needs platform reviewer assigned, needs at least two assigned reviewers
planning/trajectory_optimization/test/gcs_trajectory_optimization_test.cc
line 882 at r1 (raw file):
} GTEST_TEST(GcsTrajectoryOptimizationTest, SimpleSubspaceTest) {
I am in favor of fixing the test case that was supposed to catch this rather than writing a new test case. The reason why this silently failed was that we didn't check the solution of SolveShortestPath, which due to the rounding would simply choose the other path.
It shouldn't take too much effort to either check the solution path or to use SolveConvexRestrction to force a certain vertex sequence through a subspace.
…present in gcs_trajectory_optimization.cc.
@drake-jenkins-bot linux-jammy-clang-bazel-experimental-mosek-release please |
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.
I was having a bunch of trouble trying to isolate this typo from the underlying bug in #21386. So I've gone ahead and fixed the underlying bug in graph_of_convex_sets.cc
-- the new behavior is that if you somehow create one of these trivially-infeasible constraints, it throws an error when you try to construct its perspective.
The first commit fixes the underling bug in gcs, and causes the existing subspace test case to fail. (It also adds tests verifying this behavior.) The second commit fixes the bug in gcstrajopt, so that all test cases pass again.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee wrangelvid, needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @cohnt)
planning/trajectory_optimization/test/gcs_trajectory_optimization_test.cc
line 882 at r1 (raw file):
Previously, wrangelvid (David von Wrangel) wrote…
I am in favor of fixing the test case that was supposed to catch this rather than writing a new test case. The reason why this silently failed was that we didn't check the solution of SolveShortestPath, which due to the rounding would simply choose the other path.
It shouldn't take too much effort to either check the solution path or to use SolveConvexRestrction to force a certain vertex sequence through a subspace.
Fixing the underlying bug in gcs caused the original subspace test case to fail without any modification. Fixing the bug in gcstrajopt then allows this test to pass again. I've removed the additional test case I had proposed.
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.
Reviewed 3 of 4 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @cohnt)
+@rpoyner-tri for platform review per the schedule, please |
Fixes #21383.
This change is