OSQP always reports primal and dual values regardless of the solver status.

[hongkai-dai]

Even when the problem is infeasible/unbounded/numerical_error.

This is requested by @AdityaBhat-TRI because he wants the OSQP solution of the diff IK even when the solver fails.

---

This change is 

[hongkai-dai]

@cohnt would you mind feature-reviewing this PR? Thanks!

Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes (waiting on @hongkai-dai)

[cohnt]

The CI failure is a test in solvers/test/mathematical_program_result_test.cc, which solves an infeasible program with OSQP and expects the entries for the variables to be NaN.

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: 3 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers

---

solvers/osqp_solver.cc line 359 at r1 (raw file):

    solver_details.rho_updates = work->info->rho_updates;

    // We set the primal and dual variable no matter the status of the solver.

nit "variables"

---

solvers/osqp_solver.h line 53 at r1 (raw file):

  override either option you should probably override both at once.

  At the end of OsqpSolver::Solve() function, we always return the value of the

nit Indentation error?

---

solvers/osqp_solver.h line 55 at r1 (raw file):

  At the end of OsqpSolver::Solve() function, we always return the value of the
primal and dual variables inside the OSQP solver, regardless of the solver
status (whether it is optimal, infeasible, unbounded, etc).

nit Is it worth including a tidbit about what (if anything) those values actually mean if the problem is infeasible or unbounded? Perhaps a link to the relevant OSQP documentation?

[rpoyner-tri]

+@cohnt so reviewable sees the actual assignment.

Reviewable status: 3 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers

[hongkai-dai]

Thanks @cohnt

+@SeanCurtis-TRI for Wednesday's platform review please, thanks!

Reviewable status: 3 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform)

[cohnt]

@drake-jenkins-bot linux-jammy-clang-bazel-experimental-everything-release please

[jwnimmer-tri]

Reviewable status: 4 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform)

---

a discussion (no related file):
BTW Why is the release note "breaking change" instead of "fix" or "feature"?

[SeanCurtis-TRI]

I agree with the currently outstanding feature review comments as well.

Reviewed 3 of 3 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: 6 unresolved discussions

---

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW Why is the release note "breaking change" instead of "fix" or "feature"?

This feels more like a fix to me. The only meaningful change to public API I can see is that the MathematicalProgramResult will produce answers where previously they were NaNs. But the SolutionResult is unchanged. So, sure, if someone out there was keying on whether the result was NaN instead of looking at the SolutionResult, this could conceivably break their code, but that's on them.

---

solvers/osqp_solver.h line 55 at r2 (raw file):

  At the end of OsqpSolver::Solve() function, we always return the value of the
primal and dual variables inside the OSQP solver, regardless of the solver
status (whether it is optimal, infeasible, unbounded, etc).

nit: It doesn't seem that this is as universally true as this documentation implies. As I read the .cc file, if solution_result is kInvalidInput, we skip setting primal/dual variables. It'd be good for this comment to be more precise. It's not quite "regardless of the solver status", but "for all status values except kInvalidInput".

---

solvers/test/mathematical_program_result_test.cc line 233 at r2 (raw file):

    EXPECT_EQ(result.get_solution_result(),
              SolutionResult::kInfeasibleConstraints);
    const auto x_sol = result.GetSolution(x);

BTW A note here indicating that this test doesn't really care what the reported solution is -- just that it's reported.

Although, in contrast to the previous test, do we want to assert that the values are not NaN?

[hongkai-dai]

Reviewable status: 3 unresolved discussions

---

a discussion (no related file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

This feels more like a fix to me. The only meaningful change to public API I can see is that the MathematicalProgramResult will produce answers where previously they were NaNs. But the SolutionResult is unchanged. So, sure, if someone out there was keying on whether the result was NaN instead of looking at the SolutionResult, this could conceivably break their code, but that's on them.

Got it, I will change this to "feature"

---

solvers/osqp_solver.h line 55 at r1 (raw file):

Previously, cohnt (Thomas Cohn) wrote…

nit Is it worth including a tidbit about what (if anything) those values actually mean if the problem is infeasible or unbounded? Perhaps a link to the relevant OSQP documentation?

I haven't found the OSQP documentation explaining what is the meaning of the primal/dual solution when the problem is unbounded/infeasible. Here is what it says https://osqp.org/docs/interfaces/C.html#_CPPv412OSQPSolution.

---

solvers/osqp_solver.h line 55 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: It doesn't seem that this is as universally true as this documentation implies. As I read the .cc file, if solution_result is kInvalidInput, we skip setting primal/dual variables. It'd be good for this comment to be more precise. It's not quite "regardless of the solver status", but "for all status values except kInvalidInput".

Done! Good call!

---

solvers/osqp_solver.cc line 359 at r1 (raw file):

Previously, cohnt (Thomas Cohn) wrote…

nit "variables"

Done.

---

solvers/test/mathematical_program_result_test.cc line 233 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW A note here indicating that this test doesn't really care what the reported solution is -- just that it's reported.

Although, in contrast to the previous test, do we want to assert that the values are not NaN?

I think here I am trying to test the general behavior of MathematicalProgramResult::GetSolution() function, which doesn't guarantee that the values are not NaN, when the problem is infeasible.

In the osqp_solver_test.cc, I want to test the specific behavior of OSQP solver, which we say should return the primal and dual variable values, even when the problem is infeasible.

[SeanCurtis-TRI]

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: 2 unresolved discussions

---

solvers/osqp_solver.h line 55 at r2 (raw file):

Previously, hongkai-dai (Hongkai Dai) wrote…

Done! Good call!

Incomplete?

---

solvers/test/mathematical_program_result_test.cc line 233 at r2 (raw file):

Previously, hongkai-dai (Hongkai Dai) wrote…

I think here I am trying to test the general behavior of MathematicalProgramResult::GetSolution() function, which doesn't guarantee that the values are not NaN, when the problem is infeasible.

In the osqp_solver_test.cc, I want to test the specific behavior of OSQP solver, which we say should return the primal and dual variable values, even when the problem is infeasible.

So, if I understand correctly:

1. The choice of using OsqpSolver is simply convenient as a way to get a solver to report "infeasible".
2. As documented, we want to confirm we can "query the information in the result". And the two things that do that is: GetSolution(x) doesn't throw, and its result is appropriately sized?

What does checking the size of x_sol do that simply confirming that GetSolution(x) is happy?

Also, is there a meaningful difference between calling GetSolution(x) and GetSolution() in terms of what this test is documented for?

[cohnt]

Reviewed 1 of 1 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: 4 unresolved discussions

---

solvers/osqp_solver.h line 55 at r1 (raw file):

Previously, hongkai-dai (Hongkai Dai) wrote…

I haven't found the OSQP documentation explaining what is the meaning of the primal/dual solution when the problem is unbounded/infeasible. Here is what it says https://osqp.org/docs/interfaces/C.html#_CPPv412OSQPSolution.

It just seems weird to me that we would be copying these values over and exposing them to the user, without actually knowing what those values mean. @AdityaBhat-TRI do you have any suggestions for what we might include here?

---

solvers/osqp_solver.h line 55 at r3 (raw file):

  At the end of OsqpSolver::Solve() function, we always return the value of the
  primal and dual variables inside the OSQP solver, regardless of the solver
  status (whether it is optimal, infeasible, unbounded, etc), except when .

nit Since this is outside the scope of the bulleted list above, I think all three of these lines should not be indented?

---

solvers/osqp_solver.h line 55 at r3 (raw file):

  At the end of OsqpSolver::Solve() function, we always return the value of the
  primal and dual variables inside the OSQP solver, regardless of the solver
  status (whether it is optimal, infeasible, unbounded, etc), except when .

nit Looks like this documentation sentence was cut off?

[hongkai-dai]

Reviewable status: 2 unresolved discussions

---

solvers/osqp_solver.h line 55 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Incomplete?

Done

---

solvers/osqp_solver.h line 55 at r3 (raw file):

Previously, cohnt (Thomas Cohn) wrote…

nit Since this is outside the scope of the bulleted list above, I think all three of these lines should not be indented?

Done

---

solvers/osqp_solver.h line 55 at r3 (raw file):

Previously, cohnt (Thomas Cohn) wrote…

nit Looks like this documentation sentence was cut off?

Done

---

solvers/test/mathematical_program_result_test.cc line 233 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

So, if I understand correctly:

1. The choice of using OsqpSolver is simply convenient as a way to get a solver to report "infeasible".
2. As documented, we want to confirm we can "query the information in the result". And the two things that do that is: GetSolution(x) doesn't throw, and its result is appropriately sized?

What does checking the size of x_sol do that simply confirming that GetSolution(x) is happy?

Also, is there a meaningful difference between calling GetSolution(x) and GetSolution() in terms of what this test is documented for?

Yes, the choice of OsqpSOlver is simply convenient. I switched to using Solve() function which isn't solver-specific.

I think checking the size of x_sol makes sure that the size is correct. It doesn't add much extra value, but doesn't hurt to keep this check.

GetSolution() returns all the decision variable values, GetSolution(x) returns the value for x. In terms of this test, I don't think there is much difference. I added the check on GetSolution() anyway

[SeanCurtis-TRI]

Reviewed 2 of 3 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: 2 unresolved discussions

---

solvers/test/mathematical_program_result_test.cc line 233 at r5 (raw file):

  EXPECT_EQ(result.GetSolution().size(), 2);

  EXPECT_EQ(result.get_optimal_cost(),

The change to Solve() has changed the reported cost. Instead of being infinite, it's reporting as zero. That suggests that a different solver got called.

That also suggests, that there's an error somewhere. Either

1. It is not an invariant that an "infeasible constraint" result should produce an infinite cost and this test is incorrect, or
2. some code is not satisfying the invariant.

[cohnt]

Reviewed 2 of 3 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: 2 unresolved discussions

[hongkai-dai]

Reviewable status: 2 unresolved discussions

---

solvers/test/mathematical_program_result_test.cc line 233 at r5 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

The change to Solve() has changed the reported cost. Instead of being infinite, it's reporting as zero. That suggests that a different solver got called.

That also suggests, that there's an error somewhere. Either

1. It is not an invariant that an "infeasible constraint" result should produce an infinite cost and this test is incorrect, or
2. some code is not satisfying the invariant.

Thanks Sean.

That is a mistake in our code. In MosekSolver, when the problem is infeasible or unbounded, it doesn't set the cost to infinity.

[cohnt]

Reviewed 2 of 2 files at r6, all commit messages.
Reviewable status: 2 unresolved discussions

---

solvers/test/mathematical_program_result_test.cc line 233 at r5 (raw file):

Previously, hongkai-dai (Hongkai Dai) wrote…

Thanks Sean.

That is a mistake in our code. In MosekSolver, when the problem is infeasible or unbounded, it doesn't set the cost to infinity.

Presumably you'll be creating an issue or PR somewhere to fix this?

[hongkai-dai]

Reviewable status: 2 unresolved discussions

---

solvers/test/mathematical_program_result_test.cc line 233 at r5 (raw file):

Previously, cohnt (Thomas Cohn) wrote…

Presumably you'll be creating an issue or PR somewhere to fix this?

Yes I will file a PR

[SeanCurtis-TRI]

Reviewed 2 of 2 files at r6, all commit messages.
Reviewable status: 1 unresolved discussion

[rpoyner-tri]

Reviewed all commit messages.
Reviewable status: 1 unresolved discussion

[AdityaBhat-TRI]

solvers/osqp_solver.h line 55 at r1 (raw file):

Previously, cohnt (Thomas Cohn) wrote…

It just seems weird to me that we would be copying these values over and exposing them to the user, without actually knowing what those values mean. @AdityaBhat-TRI do you have any suggestions for what we might include here?

I would add a bit more documentation here along the lines of: The interpretation of these values when the solver status is not optimal is not clearly defined. In particular, for infeasible or unbounded problems, these values do not represent a valid solution to the original problem and may be residual artifacts from the final solver iteration. (My intention of having these values in the first place was for debugging the QP's final state before failure).

[AdityaBhat-TRI]

solvers/osqp_solver.h line 55 at r1 (raw file):

Previously, AdityaBhat-TRI (Aditya Bhat) wrote…

I would add a bit more documentation here along the lines of: The interpretation of these values when the solver status is not optimal is not clearly defined. In particular, for infeasible or unbounded problems, these values do not represent a valid solution to the original problem and may be residual artifacts from the final solver iteration. (My intention of having these values in the first place was for debugging the QP's final state before failure).

Maybe a: "Users should always check the solver status before interpreting the returned primal and dual values."

[cohnt]

Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: complete! all discussions resolved, LGTM from assignees SeanCurtis-TRI(platform),cohnt

---

solvers/osqp_solver.h line 55 at r1 (raw file):

Previously, AdityaBhat-TRI (Aditya Bhat) wrote…

Maybe a: "Users should always check the solver status before interpreting the returned primal and dual values."

I'm satisfied with the current documentation of this method.