-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Fix ParseConstraint for cases with infs in rhs or lhs. #19903
Fix ParseConstraint for cases with infs in rhs or lhs. #19903
Conversation
@drake-jenkins-bot mac-arm-ventura-clang-bazel-experimental-release please (#19335 known flake) |
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.
+@rpoyner-tri for platform review please, thanks!
Reviewed 2 of 2 files at r1.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee rpoyner-tri(platform)
solvers/create_constraint.cc
line 367 at r1 (raw file):
// -∞ <= lhs - rhs <= 0 v(k) = lhs - rhs; lb(k) = -std::numeric_limits<double>::infinity();
BTW, use -kInf
here?
solvers/create_constraint.cc
line 398 at r1 (raw file):
v(k) = lhs - rhs; lb(k) = 0.0; ub(k) = std::numeric_limits<double>::infinity();
Ditto.
Previously, cases like ``` b = [0.12, inf] ParseConstraint(x <= b) ``` would fail with ``` abort: Failure at solvers/create_constraint.cc:88 in ParseConstraint(): condition '!std::isnan(new_lb(i))' failed. ``` due to an `inf - inf` situation. Now we handle those cases properly.
6b559aa
to
4f7e527
Compare
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 rpoyner-tri(platform)
solvers/create_constraint.cc
line 398 at r1 (raw file):
Previously, hongkai-dai (Hongkai Dai) wrote…
Ditto.
Done.
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 1 of 1 files at r2.
Reviewable status: LGTM missing from assignee rpoyner-tri(platform)
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 1 of 2 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: complete! all discussions resolved, LGTM from assignees rpoyner-tri(platform),hongkai-dai
Previously, cases like
would fail with
due to an
inf - inf
situation. Now we handle those cases properly.+@hongkai-dai for feature review, please.
This change is