[solvers] Port solver back-ends to use SpecificOptions #21345
Conversation
jwnimmer-tri
added
status: do not merge
priority: medium
status: do not review
release notes: none
jwnimmer-tri
force-pushed
the
solver-options-specific
branch
3 times, most recently
from
ea392f0 to
ee3a01c
Compare
There was a problem hiding this comment.
Reviewed 42 of 45 files at r1, 6 of 7 files at r2, 8 of 9 files at r3, all commit messages.
Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge"
jwnimmer-tri
force-pushed
the
solver-options-specific
branch
from
ee3a01c to
878e75d
Compare
jwnimmer-tri
force-pushed
the
solver-options-specific
branch
2 times, most recently
from
4d106e4 to
236d634
Compare
jwnimmer-tri
force-pushed
the
solver-options-specific
branch
from
236d634 to
99ce3aa
Compare
jwnimmer-tri
force-pushed
the
solver-options-specific
branch
2 times, most recently
from
393929f to
e1549aa
Compare
jwnimmer-tri
force-pushed
the
solver-options-specific
branch
2 times, most recently
from
637959d to
34db3a1
Compare
There was a problem hiding this comment.
Reviewed 1 of 45 files at r1, 4 of 30 files at r5, 3 of 3 files at r7, 1 of 4 files at r8, 3 of 3 files at r9, 2 of 11 files at r10, all commit messages.
Reviewable status: 2 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge" (waiting on @jwnimmer-tri)
solvers/specific_options.h line 25 at r10 (raw file):
generic SolverOptions struct into the specific solver's options API. This class is intended a short-lived helper. It aliases an immutable list of
nit: Ambiguous. "Short-lived" as in "soon to be deprecated" or "short-lived" as in "narrowly scoped in the control flow"?
Code quote:
short-livedsolvers/test/solver_base_test.cc line 165 at r10 (raw file):
// In SolverBase, we have two flavors of DoSolve to test here. (This is the // only test case were testing both is relevant / important.)
typo
Suggestion:
where
jwnimmer-tri
force-pushed
the
solver-options-specific
branch
2 times, most recently
from
c993465 to
4a26338
Compare
|
For the next feature review, I'll want a fresh set of eyes so removing -@ggould-tri for now. |
jwnimmer-tri
force-pushed
the
solver-options-specific
branch
from
4a26338 to
14a06ed
Compare
There was a problem hiding this comment.
Reviewed 1 of 9 files at r3, 1 of 1 files at r4, 29 of 30 files at r5, 2 of 2 files at r6, 3 of 3 files at r7, 4 of 4 files at r8, 3 of 3 files at r9, 11 of 11 files at r10, 5 of 6 files at r11, 6 of 6 files at r13, all commit messages.
Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers
|
+@hongkai-dai are you available for feature review, please? |
There was a problem hiding this comment.
Reviewed 13 of 45 files at r1, 1 of 7 files at r2, 2 of 9 files at r3, 14 of 30 files at r5, 3 of 3 files at r7, 3 of 3 files at r9, 7 of 11 files at r10, 2 of 6 files at r11, 6 of 6 files at r13, all commit messages.
Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers
solvers/csdp_solver.cc line 418 at r13 (raw file):
options->template Pop<int>("drake::RemoveFreeVariableMethod") .value_or(static_cast<int>(RemoveFreeVariableMethod::kNullspace)); if (!(method >= 1 && method <= 3)) {
1 and 3 are magic number here? Maybe document them?
jwnimmer-tri
force-pushed
the
solver-options-specific
branch
from
14a06ed to
1a7e71d
Compare
There was a problem hiding this comment.
+@SeanCurtis-TRI for platform review per schedule, please.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee SeanCurtis-TRI(platform)
solvers/csdp_solver.cc line 418 at r13 (raw file):
Previously, hongkai-dai (Hongkai Dai) wrote…
1 and 3 are magic number here? Maybe document them?
Done.
There was a problem hiding this comment.
Reviewed 1 of 2 files at r14.
Reviewable status: LGTM missing from assignee SeanCurtis-TRI(platform)
jwnimmer-tri
added
release notes: feature
There was a problem hiding this comment.
Reviewed 13 of 45 files at r1, 1 of 7 files at r2, 2 of 9 files at r3, 14 of 30 files at r5, 3 of 3 files at r7, 3 of 3 files at r9, 6 of 11 files at r10, 2 of 6 files at r11, 5 of 6 files at r13, 2 of 2 files at r14, all commit messages.
Reviewable status: 1 unresolved discussion
solvers/nlopt_solver.cc line 336 at r14 (raw file):
} nlopt::algorithm ParseNloptAlgorithm(const std::string& algorithm) {
BTW Blech....a string called algorithm and a type nlopt::algorithm.
In the near future, we anticipate changing the SolverOptions API in support of loading and saving (i.e., serialization). That's especially troublesome for our solver back-ends that consume its information, given the low-level and hodge-podge ways in which they hunt for and apply their specific options. In anticipation of that, here we port all solvers to the new SolverBase::DoSolve2 virtual function that uses SpecificOptions instead of SolverOptions. The DoSolve API remains unchanged, for any out-of-tree solvers.
jwnimmer-tri
force-pushed
the
solver-options-specific
branch
from
1a7e71d to
5777e3d
Compare
There was a problem hiding this comment.
Reviewed 2 of 2 files at r14, 1 of 1 files at r15, all commit messages.
Reviewable status:complete! all discussions resolved, LGTM from assignees SeanCurtis-TRI(platform),hongkai-dai
solvers/nlopt_solver.cc line 336 at r14 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
BTW Blech....a
stringcalledalgorithmand a typenlopt::algorithm.
Buffalo buffalo buffalo ...
There was a problem hiding this comment.
Reviewed 1 of 1 files at r15, all commit messages.
Reviewable status:
solvers/nlopt_solver.cc line 336 at r14 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Buffalo buffalo buffalo ...
I didn't even broach the nlopt_algorithm. So, how about that for buffaloes?
…on#21345) In the near future, we anticipate changing the SolverOptions API in support of loading and saving (i.e., serialization). That's especially troublesome for our solver back-ends that consume its information, given the low-level and hodge-podge ways in which they hunt for and apply their specific options. In anticipation of that, here we port all solvers to the new SolverBase::DoSolve2 virtual function that uses SpecificOptions instead of SolverOptions. The DoSolve API remains unchanged, for any out-of-tree solvers.
Towards #20967.
This change is