Skip to content
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

Retrieve dual solution for conic constraint in Mosek. #14383

Merged

Conversation

hongkai-dai
Copy link
Contributor

@hongkai-dai hongkai-dai commented Nov 21, 2020

As a step towards #13285


This change is Reviewable

@hongkai-dai
Copy link
Contributor Author

We believe the current CI failure is due to a bug in Mosek. Currently Drake uses Mosek 9.0. @TobiaMarcucci tested with Mosek 9.2 and the solution is correct. I will upgrade Drake's supported Mosek to 9.2, and also update TRI's Mosek license.

Copy link
Contributor Author

@hongkai-dai hongkai-dai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+@avalenzu for feature review please, thanks!

Reviewable status: LGTM missing from assignee avalenzu, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @avalenzu)

Copy link
Contributor

@avalenzu avalenzu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checkpoint

Reviewable status: 3 unresolved discussions, LGTM missing from assignee avalenzu, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @avalenzu and @hongkai-dai)


solvers/mathematical_program_result.h, line 254 at r1 (raw file):

   * inequality, linear equality, bounding box constraints, and general
   * nonlinear constraints), We interpret the dual variable value as the "shadow
   * price" of the original problem. Namely if we change the constraint bound by

nit: "we" should no longer be capitalized.


solvers/mathematical_program_result.h, line 278 at r1 (raw file):

   * the dual solution is the same as the linear inequality constraint.
   *
   * The interpretation for the dual variable to conic constraint x ∈ K can be

The overlap between the types of constraints discussed in this paragraph and the following one makes this section a bit confusing.

The conic constraint support in Gurobi is only for (rotated) Lorentz cones constraints right?

If so, it feels to me like this paragraph should come after the one about Lorentz cones and strictly deal with non-Lorentz cones in solvers other than Gurobi.


solvers/mathematical_program_result.h, line 316 at r1 (raw file):

   *    solution to the (rotated) Lorentz cone constraint doesn't have the
   *    "shadow price" interpretation, but should lie in the dual cone, and
   *    satisfy the KKT * condition. For more information, refer to

nit: stray "*" here.

Copy link
Contributor Author

@hongkai-dai hongkai-dai left a 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 avalenzu, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @avalenzu)


solvers/mathematical_program_result.h, line 278 at r1 (raw file):

Previously, avalenzu (Andres Valenzuela) wrote…

The overlap between the types of constraints discussed in this paragraph and the following one makes this section a bit confusing.

The conic constraint support in Gurobi is only for (rotated) Lorentz cones constraints right?

If so, it feels to me like this paragraph should come after the one about Lorentz cones and strictly deal with non-Lorentz cones in solvers other than Gurobi.

Done. Good call.

Copy link
Contributor

@avalenzu avalenzu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review pass done. Source looks good. I've left a couple notes in-line in the tests.

Reviewed 4 of 5 files at r1, 1 of 1 files at r2.
Reviewable status: 5 unresolved discussions, LGTM missing from assignee avalenzu, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @hongkai-dai)


solvers/mathematical_program_result.h, line 313 at r2 (raw file):

   * , PSD cone, etc. When the problem is solved by a convex solver (like SCS,
   * Mosek, CSDP, etc), often it has a dual
   * variable z ∈ K*, where K* is the dual cone. Here the dual variable DOESN'T

nit: Stray line break


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

INSTANTIATE_TEST_SUITE_P(
    MosekTest, TestEllipsoidsSeparation,
    ::testing::ValuesIn(GetEllipsoidsSeparationProblems()));

nit: Extraneous whitespace change?


solvers/test/second_order_cone_program_examples.h, line 326 at r1 (raw file):

                           const SolverOptions& solver_options, double tol);

// @param rotated_lorentz_cone_with_coeffcieint_two. Set this to true if this

nit: s/coeffcieint/coefficient/


solvers/test/second_order_cone_program_examples.cc, line 485 at r1 (raw file):

  prog.AddLinearCost(x(1));
  if (solver.available()) {
    // By default the dual solution for second order cone is not computed.

If the default is to not compute the dual solution, won't the GetDualSolution() call fail if solver_options doesn't set the appropriate option?


solvers/test/second_order_cone_program_examples.cc, line 517 at r1 (raw file):

  prog.AddLinearCost(x);
  if (solver.available()) {
    SolverOptions options;

This variable is unused.

Copy link
Contributor Author

@hongkai-dai hongkai-dai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 3 unresolved discussions, LGTM missing from assignee avalenzu, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @avalenzu)


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

Previously, avalenzu (Andres Valenzuela) wrote…

nit: Extraneous whitespace change?

Yeah somehow clang-format like to format the code in this way.


solvers/test/second_order_cone_program_examples.cc, line 485 at r1 (raw file):

Previously, avalenzu (Andres Valenzuela) wrote…

If the default is to not compute the dual solution, won't the GetDualSolution() call fail if solver_options doesn't set the appropriate option?

Done. Good catch. Previously this test was just for Gurobi, where the dual solution is not computed by default. Now this test is for any solver, and the comment is not valid any more.


solvers/test/second_order_cone_program_examples.cc, line 517 at r1 (raw file):

Previously, avalenzu (Andres Valenzuela) wrote…

This variable is unused.

Done.

Copy link
Contributor

@avalenzu avalenzu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: +@sherm1 for platform review.

Reviewed 2 of 3 files at r3.
Reviewable status: LGTM missing from assignee sherm1(platform) (waiting on @avalenzu and @sherm1)

Copy link
Contributor

@avalenzu avalenzu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 3 files at r3.
Reviewable status: LGTM missing from assignee sherm1(platform) (waiting on @sherm1)

Copy link
Member

@sherm1 sherm1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Platform :lgtm: pending a few nits and possibly an additional unit test.

Reviewed 2 of 5 files at r1, 3 of 3 files at r3.
Reviewable status: 3 unresolved discussions (waiting on @hongkai-dai)


solvers/mathematical_program_result.h, line 309 at r3 (raw file):

   *    https://docs.mosek.com/9.2/capi/prob-def-conic.html#duality-for-conic-optimization
   *    as an explanation.
   * The interpretation for the dual variable to conic constraint x ∈ K can be

nit: I believe you need a blank line here to avoid having this paragraph absorbed into paragraph number 3.


solvers/mathematical_program_result.h, line 311 at r3 (raw file):

   * The interpretation for the dual variable to conic constraint x ∈ K can be
   * different. Here K is a convex cone, including exponential cone, power cone
   * , PSD cone, etc. When the problem is solved by a convex solver (like SCS,

nit: misplaced comma


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

}

GTEST_TEST(MosekSolver, SocpDualSolution1) {

minor: please add comments explaining why these tests are here. It's not obvious to me what is the relationship between these tests and the new types of dual solution results. Are they all being tested? I didn't spot a test for the exponential cone constraint.

Copy link
Contributor Author

@hongkai-dai hongkai-dai left a 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 (waiting on @avalenzu and @sherm1)


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

Previously, sherm1 (Michael Sherman) wrote…

minor: please add comments explaining why these tests are here. It's not obvious to me what is the relationship between these tests and the new types of dual solution results. Are they all being tested? I didn't spot a test for the exponential cone constraint.

Good call. I added the test for the exponential cone constraint dual solution.

Copy link
Member

@sherm1 sherm1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 5 of 5 files at r4.
Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignees avalenzu,sherm1(platform)

@hongkai-dai
Copy link
Contributor Author

@drake-jenkins-bot retest all please

Copy link
Contributor

@avalenzu avalenzu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 5 of 5 files at r4.
Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignees avalenzu,sherm1(platform)

@hongkai-dai hongkai-dai merged commit 935f56a into RobotLocomotion:master Jan 6, 2021
@hongkai-dai hongkai-dai deleted the mosek_cone_dual_solution branch November 5, 2021 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants