Refactor linear programming in convex optimization test #5122

hongkai-dai · 2017-02-10T00:55:06Z

The goal is to have separate test scripts gurobi_solver_test.cc and mosek_solver_test.cc, as mentioned in https://github.com/RobotLocomotion/drake/blob/master/drake/solvers/BUILD#L233. This is the first step, just refactored the linear programs in convex_optimization_test. I will continue to refactor the QP, SOCP and SDP in the test.

This PR exceeds 500 lines. But many of them are existing code, no math is changed.

This change is

hongkai-dai · 2017-02-10T01:32:30Z

+@david-german-tri for feature review.

Review status: 0 of 13 files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

david-german-tri · 2017-02-10T16:26:32Z

+(status: curate commits before merging)

Reviewed 9 of 13 files at r1.
Review status: 9 of 13 files reviewed at latest revision, 8 unresolved discussions.

drake/solvers/BUILD, line 342 at r1 (raw file):

    srcs = [
        "test/add_solver_util.cc",
        "test/add_solver_util.h",

Rather that repeat these utils in a bunch of srcs (which causes them to be recompiled for each), please put them in a separate drake_cc_library, set testonly = 1, and include that library in the deps of the various tests.

drake/solvers/test/add_solver_util.cc, line 16 at r1 (raw file):

    std::list<std::unique_ptr<MathematicalProgramSolverInterface>>*
    solver_list) {
  std::list<std::unique_ptr<MathematicalProgramSolverInterface>> all_solvers;

BTW why not just use a switch statement on solver_type to set a single unique_ptr<MPSI>, then check that ptr for available()? Seems more readable, and also O(N) instead of O(N^2), not that the efficiency really matters.

(Since you're just moving pre-existing code, feel free not to change it in this PR)

drake/solvers/test/CMakeLists.txt, line 32 at r1 (raw file):

macro(add_solver_test)
  add_executable(${ARGV}
          add_solver_util.cc

BTW, I don't really care too much about CMake code health at this point, but it seems like you could make a library here too, and then use drake_add_cc_test.

drake/solvers/test/mosek_solver_test.cc, line 16 at r1 (raw file):

  RunLinearPrograms(solver);
}

BTW extra whitespace here

drake/solvers/test/nonlinear_program_test.cc, line 156 at r1 (raw file):

GTEST_TEST(testNonlinearProgram, testNonConvexQPproblem1) {
  for (auto cost_form : NonConvexQPproblem1::cost_forms()) {

BTW META for file: consider const auto&. It doesn't really matter, since this is an enum, but it will be a little less surprising for readers.

drake/solvers/test/optimization_examples.cc, line 26 at r1 (raw file):

namespace test {

static std::set<CostForm> linear_cost_form{CostForm::kNonSymbolic,

"Variables of class type with static storage duration are forbidden"

Consider either allocating this object with new, or making a free function that returns it.

https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables

drake/solvers/test/optimization_examples.cc, line 824 at r1 (raw file):

  Eigen::Vector3d b_ub(numeric_limits<double>::infinity(), 5,
                       numeric_limits<double>::infinity());
  switch (cnstr_form) {

BTW Am I correctly understanding that this change adds a lot of coverage for symbolic costs and constraints? Nice!

drake/solvers/test/optimization_examples.cc, line 880 at r1 (raw file):

}

void RunLinearPrograms(const MathematicalProgramSolverInterface& solver) {

This approach rolls up a ton of test coverage into one GTest case, which means that failures will not be very diagnostic: future developers will have a hard time determining which program, cost, and constraint form they broke.

To fix this, I'd suggest replacing this function with a value-parameterized base class test fixture.

https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuide.md#how-to-write-value-parameterized-tests

For each solver, you'd then have class MosekLinearProgramTest : public LinearProgramTest, a single `TEST_P(MosekLinearProgramTest, LinearPrograms, and

INSTANTIATE_TEST_CASE_P(AllConfigurations, MosekLinearProgramTest, ::testing::Combine(costs, constraints, problems)

I know that sounds a little hairy, but I think it's actually a pretty small change from what you already have. Essentially your for loops just become switch statements on the params.



---


*Comments from [Reviewable](https://reviewable.io:443/reviews/robotlocomotion/drake/5122#-:-KccaQQCFTYEfDT85gTX:b-s794vx)*
<!-- Sent from Reviewable.io -->

hongkai-dai · 2017-02-11T00:05:19Z

Review status: 6 of 13 files reviewed at latest revision, 8 unresolved discussions.

drake/solvers/BUILD, line 342 at r1 (raw file):

Previously, david-german-tri (David German) wrote…

Rather that repeat these utils in a bunch of srcs (which causes them to be recompiled for each), please put them in a separate drake_cc_library, set testonly = 1, and include that library in the deps of the various tests.

Done. Thanks! I did not know we can add a testonly library.

drake/solvers/test/add_solver_util.cc, line 16 at r1 (raw file):

Previously, david-german-tri (David German) wrote…

BTW why not just use a switch statement on solver_type to set a single unique_ptr<MPSI>, then check that ptr for available()? Seems more readable, and also O(N) instead of O(N^2), not that the efficiency really matters.

(Since you're just moving pre-existing code, feel free not to change it in this PR)

I agree, but I am going to remove this file after another one or two PRs (https://github.com/RobotLocomotion/drake/pull/5122/files#diff-d0303f4358e2c08dc7c37920a852a20d), so I did not change this code

drake/solvers/test/CMakeLists.txt, line 32 at r1 (raw file):

Previously, david-german-tri (David German) wrote…

BTW, I don't really care too much about CMake code health at this point, but it seems like you could make a library here too, and then use drake_add_cc_test.

Done.

drake/solvers/test/mosek_solver_test.cc, line 16 at r1 (raw file):

Previously, david-german-tri (David German) wrote…

BTW extra whitespace here

Done.

drake/solvers/test/nonlinear_program_test.cc, line 156 at r1 (raw file):

Previously, david-german-tri (David German) wrote…

BTW META for file: consider const auto&. It doesn't really matter, since this is an enum, but it will be a little less surprising for readers.

Done.

drake/solvers/test/optimization_examples.cc, line 26 at r1 (raw file):

Previously, david-german-tri (David German) wrote…

"Variables of class type with static storage duration are forbidden"

Consider either allocating this object with new, or making a free function that returns it.

https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables

Done.

drake/solvers/test/optimization_examples.cc, line 824 at r1 (raw file):

Previously, david-german-tri (David German) wrote…

BTW Am I correctly understanding that this change adds a lot of coverage for symbolic costs and constraints? Nice!

Done. Yes, that is the purpose, and I actually find bugs in our code with this coverage. Will fix these bugs in my next PR.

drake/solvers/test/optimization_examples.cc, line 880 at r1 (raw file):

Previously, david-german-tri (David German) wrote…

This approach rolls up a ton of test coverage into one GTest case, which means that failures will not be very diagnostic: future developers will have a hard time determining which program, cost, and constraint form they broke.

To fix this, I'd suggest replacing this function with a value-parameterized base class test fixture.

https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuide.md#how-to-write-value-parameterized-tests

For each solver, you'd then have class MosekLinearProgramTest : public LinearProgramTest, a single `TEST_P(MosekLinearProgramTest, LinearPrograms, and

INSTANTIATE_TEST_CASE_P(AllConfigurations, MosekLinearProgramTest, ::testing::Combine(costs, constraints, problems)

I know that sounds a little hairy, but I think it's actually a pretty small change from what you already have. Essentially your for loops just become switch statements on the params.

Done. Thanks for the suggestion!

Comments from Reviewable

hongkai-dai · 2017-02-11T01:32:05Z

@drake-jenkins-bot linux-gcc-experimental-matlab please

Review status: 6 of 13 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

Comments from Reviewable

hongkai-dai · 2017-02-11T11:11:53Z

Review status: 6 of 13 files reviewed at latest revision, 4 unresolved discussions.

drake/solvers/test/optimization_examples.cc, line 880 at r1 (raw file):

Previously, hongkai-dai (Hongkai Dai) wrote…

Done. Thanks for the suggestion!

@david-german-tri , I used std::shared_ptr here because ::testing::VariableIn does not work well with std::vector<std::unique_ptr<T>>, that it tries to call the copy constructor of the storage type in std::vector, and the copy constructor for std::unique_ptr is deleted. Do you know a better way other than using std::shared_ptr?

Comments from Reviewable

david-german-tri · 2017-02-13T23:32:15Z

Reviewed 7 of 7 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

drake/solvers/BUILD, line 126 at r2 (raw file):

    srcs = ["test/add_solver_util.cc"],
    hdrs = ["test/add_solver_util.h"],
    linkstatic = 1,

META: this is no longer needed post #5155

drake/solvers/test/optimization_examples.cc, line 880 at r1 (raw file):

Previously, hongkai-dai (Hongkai Dai) wrote…

@david-german-tri , I used std::shared_ptr here because ::testing::VariableIn does not work well with std::vector<std::unique_ptr<T>>, that it tries to call the copy constructor of the storage type in std::vector, and the copy constructor for std::unique_ptr is deleted. Do you know a better way other than using std::shared_ptr?

This is an improvement, but wasn't quite what I meant. I was proposing that the parameter is a tuple of CostForm, ConstraintForm, and problem, where problem is either

a new enum, on which you switch, or
an std::function of CostForm and ConstraintForm that returns a std::unique_ptr<LinearProgram>

Either of these avoids the shared_ptr problem.

Comments from Reviewable

hongkai-dai · 2017-02-14T01:06:15Z

Review status: 8 of 13 files reviewed at latest revision, 2 unresolved discussions.

drake/solvers/BUILD, line 126 at r2 (raw file):

Previously, david-german-tri (David German) wrote…

META: this is no longer needed post #5155

Done.

drake/solvers/test/optimization_examples.cc, line 880 at r1 (raw file):

Previously, david-german-tri (David German) wrote…

This is an improvement, but wasn't quite what I meant. I was proposing that the parameter is a tuple of CostForm, ConstraintForm, and problem, where problem is either

a new enum, on which you switch, or

an std::function of CostForm and ConstraintForm that returns a std::unique_ptr<LinearProgram>

Either of these avoids the shared_ptr problem.

Done. Thanks for the suggestion, I used enum approach.

Comments from Reviewable

david-german-tri · 2017-02-14T15:00:47Z

Awesome!

Reviewed 5 of 5 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

drake/solvers/test/mosek_solver_test.cc, line 28 at r3 (raw file):

                            ::testing::ValuesIn(linear_cost_form()),
                            ::testing::ValuesIn(linear_constraint_form()),
                        ::testing::ValuesIn(linear_problems())));

BTW weird indent here (not worth re-running CI just for this, though)

Comments from Reviewable

hongkai-dai · 2017-02-14T17:29:04Z

+@jwnimmer-tri for platform review.

Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable

jwnimmer-tri · 2017-02-14T17:30:38Z

I can look, but would probably be on Friday (my on-call day).

hongkai-dai · 2017-02-14T17:35:56Z

@david-german-tri , would you like to serve as a platform reviewer for this PR?

Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable

david-german-tri · 2017-02-14T17:48:04Z

I think a second review is appropriate, but feel free to pick someone who isn't a platform reviewer.

hongkai-dai · 2017-02-14T17:52:42Z

+@sammy-tri for platform review. -@jwnimmer-tri for platform review.

Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable

sammy-tri · 2017-02-14T19:08:54Z

Partial pass completed. Will resume later this afternoon.

Reviewed 5 of 13 files at r1, 1 of 7 files at r2, 3 of 5 files at r3.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

drake/solvers/test/CMakeLists.txt, line 27 at r3 (raw file):

        optimization_examples.h
        mathematical_program_test_util.h)
set(drakeOptimizationPrivateHeaders

Why are we creating an empty variable here?

Comments from Reviewable

sammy-tri · 2017-02-14T21:16:03Z

Reviewed 1 of 13 files at r1, 1 of 7 files at r2, 2 of 5 files at r3.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

Comments from Reviewable

hongkai-dai · 2017-02-14T22:00:07Z

Review status: 10 of 13 files reviewed at latest revision, 2 unresolved discussions.

drake/solvers/test/CMakeLists.txt, line 27 at r3 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

Why are we creating an empty variable here?

Done.

drake/solvers/test/mosek_solver_test.cc, line 28 at r3 (raw file):

Previously, david-german-tri (David German) wrote…

BTW weird indent here (not worth re-running CI just for this, though)

Done.

Comments from Reviewable

sammy-tri · 2017-02-14T22:18:54Z

Reviewed 3 of 3 files at r4.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

hongkai-dai · 2017-02-14T22:27:07Z

-(status: curate commits before merging)

Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

…ion#5122) * WIP: refactor tests * refactor linear program tests, get segfault * more LP tests refactored * cmake compiles and runs, the linear program is refactored * minor change * WIP: bazel build fix * Bazel builds * add bazel test for gurobi_solver_test * cpplint * forgot to upload some files * minor fix * Address David's comments * Use parameter test * cpplint * fix a bug * Address David's comments * Address reviewers' comments

hongkai-dai added 11 commits February 9, 2017 14:33

WIP: refactor tests

f33dd64

refactor linear program tests, get segfault

8b31adc

more LP tests refactored

f032b11

cmake compiles and runs, the linear program is refactored

4840393

minor change

c34f617

WIP: bazel build fix

0808e80

Bazel builds

51c03f0

add bazel test for gurobi_solver_test

7f08690

cpplint

371335f

forgot to upload some files

bafb8f3

minor fix

c1b7fbf

hongkai-dai assigned david-german-tri Feb 10, 2017

david-german-tri added the status: curate commits before merging label Feb 10, 2017

hongkai-dai added 3 commits February 10, 2017 13:33

Address David's comments

bc232f3

Use parameter test

bf79c57

cpplint

c5ee83d

fix a bug

2f374cd

Address David's comments

3a36b1f

hongkai-dai assigned jwnimmer-tri Feb 14, 2017

hongkai-dai assigned sammy-tri and unassigned jwnimmer-tri Feb 14, 2017

Address reviewers' comments

cc7c977

hongkai-dai removed the status: curate commits before merging label Feb 14, 2017

hongkai-dai merged commit bc960c8 into RobotLocomotion:master Feb 14, 2017

hongkai-dai deleted the refactor_convex_optimization_test branch February 15, 2017 21:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor linear programming in convex optimization test #5122

Refactor linear programming in convex optimization test #5122

hongkai-dai commented Feb 10, 2017 •

edited by david-german-tri

Loading

hongkai-dai commented Feb 10, 2017

david-german-tri commented Feb 10, 2017

hongkai-dai commented Feb 11, 2017

hongkai-dai commented Feb 11, 2017

hongkai-dai commented Feb 11, 2017

david-german-tri commented Feb 13, 2017

hongkai-dai commented Feb 14, 2017

david-german-tri commented Feb 14, 2017

hongkai-dai commented Feb 14, 2017

jwnimmer-tri commented Feb 14, 2017

hongkai-dai commented Feb 14, 2017

david-german-tri commented Feb 14, 2017

hongkai-dai commented Feb 14, 2017

sammy-tri commented Feb 14, 2017

sammy-tri commented Feb 14, 2017

hongkai-dai commented Feb 14, 2017

sammy-tri commented Feb 14, 2017

hongkai-dai commented Feb 14, 2017

Refactor linear programming in convex optimization test #5122

Refactor linear programming in convex optimization test #5122

Conversation

hongkai-dai commented Feb 10, 2017 • edited by david-german-tri Loading

hongkai-dai commented Feb 10, 2017

david-german-tri commented Feb 10, 2017

hongkai-dai commented Feb 11, 2017

hongkai-dai commented Feb 11, 2017

hongkai-dai commented Feb 11, 2017

david-german-tri commented Feb 13, 2017

hongkai-dai commented Feb 14, 2017

david-german-tri commented Feb 14, 2017

hongkai-dai commented Feb 14, 2017

jwnimmer-tri commented Feb 14, 2017

hongkai-dai commented Feb 14, 2017

david-german-tri commented Feb 14, 2017

hongkai-dai commented Feb 14, 2017

sammy-tri commented Feb 14, 2017

sammy-tri commented Feb 14, 2017

hongkai-dai commented Feb 14, 2017

sammy-tri commented Feb 14, 2017

hongkai-dai commented Feb 14, 2017

hongkai-dai commented Feb 10, 2017 •

edited by david-german-tri

Loading