Return the new cost and slack variables from AddMaximizeLogDeterminantSymmetricMatrixCost

[hongkai-dai]

This change is 

[hongkai-dai]

+@jwnimmer-tri for feature review please, thanks!

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

[jwnimmer-tri]

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers (waiting on @hongkai-dai)

---

solvers/mathematical_program.h, line 1206 at r1 (raw file):

   * log(Z(i, i)) >= t(i)
   * and we will minimize -∑ᵢt(i).
   * @param X A symmetric positive semidefinite matrix X, whose log(det(X)) will

nit The "be maximized" text that ends the sentence got severed here.

---

solvers/mathematical_program.cc, line 562 at r1 (raw file):

  const auto cost = AddLinearCost(-Eigen::VectorXd::Ones(t.rows()), t);
  return std::make_tuple(cost, t, Z_var);

nit If I'm reading correctly, I think that t and Z_var should be moved-from here, instead of copied?

---

solvers/test/exponential_cone_program_examples.cc, line 98 at r1 (raw file):

      b.cast<symbolic::Expression>().transpose() / 2, c;
  prog.AddPositiveSemidefiniteConstraint(ellipsoid_psd);
  const auto log_det_ret = prog.AddMaximizeLogDeterminantSymmetricMatrixCost(

Missing unit test coverage for the contents of Z.

---

solvers/test/exponential_cone_program_examples.cc, line 126 at r1 (raw file):

  EXPECT_NEAR(result.get_optimal_cost(), expected_cost, tol);
  EXPECT_NEAR(result.EvalBinding(std::get<0>(log_det_ret))(0), expected_cost,
              tol);

The <0> template mumble here is non-obvious. Back up on L98 at the call site, you should use structured bindings to store the resulting tuple into three different named locals, without any use of tuples in the unit test.

[hongkai-dai]

Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers (waiting on @jwnimmer-tri)

---

solvers/mathematical_program.cc, line 562 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit If I'm reading correctly, I think that t and Z_var should be moved-from here, instead of copied?

Done. Good call.

---

solvers/test/exponential_cone_program_examples.cc, line 98 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Missing unit test coverage for the contents of Z.

Done. Good call. The added test helps to find a bug.

---

solvers/test/exponential_cone_program_examples.cc, line 126 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

The <0> template mumble here is non-obvious. Back up on L98 at the call site, you should use structured bindings to store the resulting tuple into three different named locals, without any use of tuples in the unit test.

I tried to do something like this

Binding<LinearCost> cost;
VectorX<symbolic::Variable> t;
MatrixX<symbolic::Variable> Z;
std::tie(cost, t, Z) = prog.AddMaximizeLogDeterminantSymmetricMatrixCost();

but the compilation fails because Binding<LinearCost> doesn't have a default constructor.

So I still use the struct as the return, but add named variable for each entry in the tuple.

[jwnimmer-tri]

Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers (waiting on @hongkai-dai)

---

solvers/mathematical_program.h, line 1209 at r2 (raw file):

   * be maximized.
   * @return (cost, t, Z) cost is -∑ᵢt(i), we also return the newly created
   * slack variables t and Z.

The cost uses a triangular Z with the upper part as zero.

However, as of r2 the Z variables are returned as a symmetric matrix (no zeros), whereas in r1 the upper part was undefined (rather than symmetric). I liked r1 better, but in any case you should choose one and then document in the API here which one it is.

---

solvers/mathematical_program.cc, line 548 at r2 (raw file):

    }
    diag_Z(j, j) = Z(j, j);
  }

nit I almost remarked the first time, but with the new code it's more acute. This loop is quite difficult to follow. Pulling out the Z_lower iteration step into its own dedicated line would be a big improvement:

  int Z_lower_index = 0;
  for (int j = 0; j < X_rows; ++j) {
    for (int i = j; i < X_rows; ++i) {
      const Variable z = Z_lower(Z_lower_index++);
      Z(i, j) = z;
      Z_var(i, j) = z;
      if (i != j) {
        Z_var(j, i) = z;
      }
    }
    diag_Z(j, j) = Z(j, j);
  }

---

solvers/test/exponential_cone_program_examples.cc, line 126 at r1 (raw file):

Previously, hongkai-dai (Hongkai Dai) wrote…

I tried to do something like this

Binding<LinearCost> cost;
VectorX<symbolic::Variable> t;
MatrixX<symbolic::Variable> Z;
std::tie(cost, t, Z) = prog.AddMaximizeLogDeterminantSymmetricMatrixCost();

but the compilation fails because Binding<LinearCost> doesn't have a default constructor.

So I still use the struct as the return, but add named variable for each entry in the tuple.

See https://drake.mit.edu/styleguide/cppguide.html#Type_deduction at "structured bindings".

Something like:

const auto [linear_cost, det_log_t, det_log_z] = prog.Add...(...);

[hongkai-dai]

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

---

solvers/mathematical_program.h, line 1209 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

The cost uses a triangular Z with the upper part as zero.

However, as of r2 the Z variables are returned as a symmetric matrix (no zeros), whereas in r1 the upper part was undefined (rather than symmetric). I liked r1 better, but in any case you should choose one and then document in the API here which one it is.

Good call, I switched it back to lower diagonal matrix.

---

solvers/mathematical_program.cc, line 548 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

nit I almost remarked the first time, but with the new code it's more acute. This loop is quite difficult to follow. Pulling out the Z_lower iteration step into its own dedicated line would be a big improvement:

  int Z_lower_index = 0;
  for (int j = 0; j < X_rows; ++j) {
    for (int i = j; i < X_rows; ++i) {
      const Variable z = Z_lower(Z_lower_index++);
      Z(i, j) = z;
      Z_var(i, j) = z;
      if (i != j) {
        Z_var(j, i) = z;
      }
    }
    diag_Z(j, j) = Z(j, j);
  }

I removed Z_var and switched back to r0. Do you think it is good now?

---

solvers/test/exponential_cone_program_examples.cc, line 126 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

See https://drake.mit.edu/styleguide/cppguide.html#Type_deduction at "structured bindings".

Something like:

const auto [linear_cost, det_log_t, det_log_z] = prog.Add...(...);

I see, thanks a lot!

[jwnimmer-tri]

feature, modulo the pydrake CI fix.

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: needs at least two assigned reviewers (waiting on @hongkai-dai)

---

solvers/mathematical_program.h, line 1209 at r2 (raw file):

Previously, hongkai-dai (Hongkai Dai) wrote…

Good call, I switched it back to lower diagonal matrix.

OK I was initially unhappy with switching the return type of Z to use Expression to denote "a Variable, or zero" in the returned Z, but after reading the unit test I think it's fine, actually.

It would be nice to overload MathematicalProgramResult::GetSolution() to take Matrix<Expression> at some future point to reduce boilerplate, but this is fine as-is.

---

solvers/mathematical_program.cc, line 548 at r2 (raw file):

Previously, hongkai-dai (Hongkai Dai) wrote…

I removed Z_var and switched back to r0. Do you think it is good now?

Ya, I think the loop is clear enough now.

[hongkai-dai]

+@sherm1 for platform review please, thanks!

Reviewable status: LGTM missing from assignee sherm1(platform) (waiting on @jwnimmer-tri and @sherm1)

---

solvers/mathematical_program.h, line 1209 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

OK I was initially unhappy with switching the return type of Z to use Expression to denote "a Variable, or zero" in the returned Z, but after reading the unit test I think it's fine, actually.

It would be nice to overload MathematicalProgramResult::GetSolution() to take Matrix<Expression> at some future point to reduce boilerplate, but this is fine as-is.

We actually have MathematicalProgramResult::GetSolution(Matrix<Expression>), it also returns a Matrix, so I will need to convert each entry of symbolic::Expression to double value anyway.

[jwnimmer-tri]

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: LGTM missing from assignee sherm1(platform) (waiting on @sherm1)

---

solvers/mathematical_program.h, line 1209 at r2 (raw file):

Previously, hongkai-dai (Hongkai Dai) wrote…

We actually have MathematicalProgramResult::GetSolution(Matrix<Expression>), it also returns a Matrix, so I will need to convert each entry of symbolic::Expression to double value anyway.

Oh indeed, I didn't scroll long enough.

Given that, anExtractDoubleOrThrow(result.GetSolution(Z)) would avoid the loop.

[hongkai-dai]

Reviewable status: LGTM missing from assignee sherm1(platform) (waiting on @jwnimmer-tri and @sherm1)

---

solvers/mathematical_program.h, line 1209 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Oh indeed, I didn't scroll long enough.

Given that, anExtractDoubleOrThrow(result.GetSolution(Z)) would avoid the loop.

Cool, I didn't realize that ExtractDoubleOrThrow works for symbolic expression. Thanks!

[jwnimmer-tri]

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: LGTM missing from assignee sherm1(platform) (waiting on @sherm1)

[sherm1]

Platform with one BTW request.

Reviewed 1 of 5 files at r1, 2 of 3 files at r3, 1 of 1 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: 1 unresolved discussion (waiting on @hongkai-dai)

---

solvers/mathematical_program.h, line 1209 at r5 (raw file):

   * be maximized.
   * @return (cost, t, Z) cost is -∑ᵢt(i), we also return the newly created
   * slack variables t and Z.

BTW seeing this description I expected t and Z to be the same type, specifically "variables". Below I see that one is a Variable and the other an Expression. From the previous discussion it appears that there is a good reason for that. But please consider explaining that reason in the comment here or as a note.

[sherm1]

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: complete! all discussions resolved, LGTM from assignees jwnimmer-tri(platform),sherm1(platform) (waiting on @hongkai-dai)