Adds Spectrahedron isa ConvexSet #19316
Conversation
RussTedrake
added
priority: medium
release notes: feature
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee TobiaMarcucci, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @RussTedrake and @TobiaMarcucci)
geometry/optimization/spectrahedron.h line 39 at r1 (raw file):
bool DoIsBounded() const final; bool DoPointInSet(const Eigen::Ref<const Eigen::VectorXd>& x,
It feels unnatural to have to pass x as a vector to check membership in the Spectrahedron. Should we relax this to Eigen::MatrixXd everywhere?
geometry/optimization/spectrahedron.cc line 28 at r1 (raw file):
Spectrahedron::Spectrahedron(const MathematicalProgram& prog) : ConvexSet(&ConvexSetCloner<Spectrahedron>, prog.num_vars()) { DRAKE_DEMAND(solvers::GetProgramType(prog) == solvers::ProgramType::kSDP);
Does this check that
@pre @p prog must have exactly one positive-semidefinite constraint which is
bound to *all* of the decision variables.
There was a problem hiding this comment.
Thanks, looks great so far! One nit below.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee TobiaMarcucci, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @RussTedrake and @TobiaMarcucci)
geometry/optimization/spectrahedron.cc line 28 at r2 (raw file):
Spectrahedron::Spectrahedron(const MathematicalProgram& prog) : ConvexSet(&ConvexSetCloner<Spectrahedron>, prog.num_vars()) { DRAKE_DEMAND(solvers::GetProgramType(prog) == solvers::ProgramType::kSDP);
nit When validating user input to a function, we should respond to mistakes using throw not std::abort.
Suggestion:
DRAKE_THROW_UNLESS
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee TobiaMarcucci, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @RussTedrake and @TobiaMarcucci)
geometry/optimization/spectrahedron.cc line 28 at r2 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
nit When validating user input to a function, we should respond to mistakes using
thrownotstd::abort.
... and to be clear, it's fine for the DoFoo() NVI implementations to use DEMAND, since those are an internal double-check not input validation. The input validation happens in the ConvexSet::Foo base class before it calls into the DoFoo function.
There was a problem hiding this comment.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee TobiaMarcucci, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @RussTedrake)
geometry/optimization/spectrahedron.cc line 60 at r2 (raw file):
std::vector<Binding<Constraint>> Spectrahedron::DoAddPointInNonnegativeScalingConstraints(
Just to be sure I understand this part. A Spectahedron can be defined by bounding-box constraints, linear inequality, quadratic inequalities, and semidefinite constraints? That's why one needs to go through all these cases within this method? if this is the case, this seems to be already quite close to the other approach we discussed, where each convex set in the GCS is put in canonical conic form and the perspective stuff is implemented only once for generic cone constraints?
EDIT: With this I don't mean to stop this PR, I'm just trying to understand what is the gap between this and the other approach.
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee TobiaMarcucci, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @RussTedrake and @TobiaMarcucci)
geometry/optimization/spectrahedron.h line 39 at r1 (raw file):
Previously, AlexandreAmice (Alexandre Amice) wrote…
It feels unnatural to have to pass
xas a vector to check membership in the Spectrahedron. Should we relax this toEigen::MatrixXdeverywhere?
Done. Right now, this x can be the entire list of decision variables for a mathematical program. (Plus this particular method is an overload from the base class).
I think this one should stay as is, but we can add additional sugar methods for the case you're thinking of in a follow-up. I've added a TODO.
geometry/optimization/spectrahedron.cc line 28 at r1 (raw file):
Previously, AlexandreAmice (Alexandre Amice) wrote…
Does this check that
@pre @p prog must have exactly one positive-semidefinite constraint which is bound to *all* of the decision variables.
No. I decided to relax that constraint. Good catch!
geometry/optimization/spectrahedron.cc line 28 at r2 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
... and to be clear, it's fine for the
DoFoo()NVI implementations to useDEMAND, since those are an internal double-check not input validation. The input validation happens in the ConvexSet::Foo base class before it calls into the DoFoo function.
Done.
geometry/optimization/spectrahedron.cc line 60 at r2 (raw file):
Previously, TobiaMarcucci (Tobia Marcucci) wrote…
Just to be sure I understand this part. A Spectahedron can be defined by bounding-box constraints, linear inequality, quadratic inequalities, and semidefinite constraints? That's why one needs to go through all these cases within this method? if this is the case, this seems to be already quite close to the other approach we discussed, where each convex set in the GCS is put in canonical conic form and the perspective stuff is implemented only once for generic cone constraints?
Done. Yes. (as discussed f2f).
There was a problem hiding this comment.
+@jwnimmer-tri for platform review, please
@TobiaMarcucci -- hoping for an LGTM from you when you're ready.
Reviewable status: 3 unresolved discussions, LGTM missing from assignees TobiaMarcucci,jwnimmer-tri(platform) (waiting on @jwnimmer-tri and @TobiaMarcucci)
There was a problem hiding this comment.
See RussTedrake#48 with fixes and suggestions for some of the discussions below.
Reviewed 3 of 6 files at r1, 3 of 3 files at r3, all commit messages.
Reviewable status: 9 unresolved discussions, LGTM missing from assignee TobiaMarcucci (waiting on @TobiaMarcucci)
geometry/optimization/spectrahedron.h line 75 at r3 (raw file):
copyable_unique_ptr<solvers::MathematicalProgram> sdp_{}; static const solvers::ProgramAttributes supported_attributes_;
The class ProgramAttributes has a non-trivial constructor and destructor, so need special handling for https://drake.mit.edu/styleguide/cppguide.html#Static_and_Global_Variables.
This is fixed in my meta-PR.
geometry/optimization/spectrahedron.cc line 40 at r3 (raw file):
const solvers::ProgramAttributes Spectrahedron::supported_attributes_ = { ProgramAttribute::kLinearCost, ProgramAttribute::kLinearConstraint,
I don't follow the reasoning for why we allow for linear costs, but not e.g. quadratic cost or generic costs. In the implementation, we're only checking a feasibility problem anyway and we remove all costs from the input prog. It seems to me like we should either advertise all costs in the "supported attributes" set, or possibly no costs with a disclaimer in the Doxygen that we only list the supported constraints and end up punting all costs anyway. Having just one cost listed here seems inconsistent.
Code quote:
ProgramAttribute::kLinearCost,geometry/optimization/spectrahedron.cc line 107 at r3 (raw file):
t; MatrixXd Ab = MatrixXd::Identity(binding.evaluator()->num_constraints(), binding.evaluator()->num_vars() + 1);
nit The copy-pasted code for subsetting [x;t] and num_vars + 1 somewhat obscures the bigger picture within of each loop. Some refactoring for conciseness would help readability.
This is fixed in my meta-PR.
geometry/optimization/spectrahedron.cc line 130 at r3 (raw file):
binding.evaluator()->GetDenseA(); Ab.rightCols<1>() = -binding.evaluator()->lower_bound(); prog->AddLinearConstraint(Ab, 0, 0, vars);
Here, we add a LinearConstraint to the new prog. It has the mathematical form of a linear equality constraint, but not the runtime type of one.
Arguably, that's a bug in MathematicalProgram::AddLinearConstraint not detecting that (ub.array() == lb.array()).all(), but in the meantime should we avoid the pothole by calling AddLinearEqualityConstraint instead?
The program classification will not be precise otherwise (though unlikely to matter in practice).
Suggestion:
prog->AddLinearEqualityConstraint(Ab, VectorXd::Zero(num_constraints), vars);geometry/optimization/test/spectrahedron_test.cc line 41 at r3 (raw file):
} /**
nit Don't use Doxygen opener for non-Doxygen code.
Code quote:
/**geometry/optimization/test/spectrahedron_test.cc line 95 at r3 (raw file):
0, x3, t3), ".*not implemented yet.*"); }
Working
Are these tests sufficient coverage? It seems like we could drop some constraints and still pass the x-true, x-false spot checks.
Will explore locally.
There was a problem hiding this comment.
Reviewable status: 9 unresolved discussions, LGTM missing from assignee TobiaMarcucci (waiting on @TobiaMarcucci)
geometry/optimization/test/spectrahedron_test.cc line 95 at r3 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Working
Are these tests sufficient coverage? It seems like we could drop some constraints and still pass the x-true, x-false spot checks.
Will explore locally.
Yeah, if I comment out the whole for (const auto& binding : sdp_->linear_constraints()) { ... } block, all tests still pass.
Probably the easiest fix is just to directly interrogate all of the constraints that came out for their exact mathematical form (checking the A matrix, bounds, etc). For your input program, it seems like the added constraints are not that complicated and we could easily desk-check the intended result.
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions
geometry/optimization/spectrahedron.h line 75 at r3 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
The class
ProgramAttributeshas a non-trivial constructor and destructor, so need special handling for https://drake.mit.edu/styleguide/cppguide.html#Static_and_Global_Variables.This is fixed in my meta-PR.
Done. thanks.
geometry/optimization/spectrahedron.cc line 40 at r3 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
I don't follow the reasoning for why we allow for linear costs, but not e.g. quadratic cost or generic costs. In the implementation, we're only checking a feasibility problem anyway and we remove all costs from the input
prog. It seems to me like we should either advertise all costs in the "supported attributes" set, or possibly no costs with a disclaimer in the Doxygen that we only list the supported constraints and end up punting all costs anyway. Having just one cost listed here seems inconsistent.
By convention, semidefinite programs can only ever have linear costs.
geometry/optimization/spectrahedron.cc line 107 at r3 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
nit The copy-pasted code for subsetting
[x;t]andnum_vars + 1somewhat obscures the bigger picture within of each loop. Some refactoring for conciseness would help readability.This is fixed in my meta-PR.
Done. Thanks.
geometry/optimization/spectrahedron.cc line 130 at r3 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Here, we add a
LinearConstraintto the newprog. It has the mathematical form of a linear equality constraint, but not the runtime type of one.Arguably, that's a bug in
MathematicalProgram::AddLinearConstraintnot detecting that(ub.array() == lb.array()).all(), but in the meantime should we avoid the pothole by calling AddLinearEqualityConstraint instead?The program classification will not be precise otherwise (though unlikely to matter in practice).
Done. good catch.
geometry/optimization/test/spectrahedron_test.cc line 41 at r3 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
nit Don't use Doxygen opener for non-Doxygen code.
Done.
geometry/optimization/test/spectrahedron_test.cc line 95 at r3 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Yeah, if I comment out the whole
for (const auto& binding : sdp_->linear_constraints()) { ... }block, all tests still pass.Probably the easiest fix is just to directly interrogate all of the constraints that came out for their exact mathematical form (checking the A matrix, bounds, etc). For your input program, it seems like the added constraints are not that complicated and we could easily desk-check the intended result.
I've added the ones that are easy comparison, and more checks around the ones that are more complicated. Reassembling all of the new matrices to check the scaling is a pretty heavy lift.
Code quote:
Vector<double, 7> xt_test;
xt_test << x_star, 1;
EXPECT_TRUE(prog3.CheckSatisfied(prog3.GetAllConstraints(), xt_test));
EXPECT_TRUE(
prog3.CheckSatisfied(prog3.GetAllConstraints(), VectorXd::Zero(7)));
xt_test << x_bad, 1;
EXPECT_FALSE(prog3.CheckSatisfied(prog3.GetAllConstraints(), xt_test));
There was a problem hiding this comment.
Reviewed 1 of 2 files at r4, 3 of 3 files at r5, all commit messages.
Reviewable status: 1 unresolved discussion
geometry/optimization/spectrahedron.cc line 147 at r5 (raw file):
if (binding.evaluator()->lower_bound().array().isFinite().any()) { Ab.rightCols<1>() = -binding.evaluator()->lower_bound(); prog->AddLinearConstraint(Ab, 0, kInf, vars);
If I comment out this line of code, no tests fail.
This adds the minimal functionality to be useful. I will follow-up with more PRs to fill in the "not implemented yet" blocks as needed.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: 1 unresolved discussion
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion
geometry/optimization/spectrahedron.cc line 147 at r5 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
If I comment out this line of code, no tests fail.
That was because of the linear constraint having no lower bound (not the testing). I've added a lower bound, and more tests.
This adds the minimal functionality to be useful. I will follow-up with more PRs to fill in the "not implemented yet" blocks as needed.
+@TobiaMarcucci for feature review, please.
fyi @jwnimmer-tri. I expect this will land on your platform day tomorrow. You have a lot of cleanups in the ConvexSet hierarchy; I tried to be consistent with most of them (but still have the copyable_unique_ptr in my bindings so far).
This change is