Update single entry of coefficients in LinearCost and QuadraticCost.

[hongkai-dai]

Towards #22131

I tested with and without this change. For updating the coefficient in LinearCost for 1000 times

Use UpdateCoefficients: takes 0.130985ms
Use UpdateCoefficientEntry: takes 0.008051ms

---

This change is 

[hongkai-dai]

+@cohnt for feature review please, thanks!

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

[cohnt]

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee cohnt, needs platform reviewer assigned, needs at least two assigned reviewers

---

solvers/cost.cc line 63 at r1 (raw file):

void LinearCost::UpdateCoefficientEntry(int i, double val) {
  a_[i] = val;

I think we'll need some sort of protection for if the user passes in an out-of-bounds index i.

---

solvers/cost.cc line 105 at r1 (raw file):

void QuadraticCost::UpdateHessianEntry(int i, int j, double val,
                                       std::optional<bool> is_hessian_psd) {
  Q_(i, j) = val;

Ditto above -- make sure (i,j) is a valid index

---

solvers/cost.cc line 117 at r1 (raw file):

void QuadraticCost::UpdateLinearCoefficientEntry(int i, double val) {
  b_(i) = val;

Ditto above -- make sure i is a valid index

---

solvers/cost.h line 81 at r1 (raw file):

  /**
   * Updates one entry in the coefficient of the cost.

nit: Usually the code I've seen in drake doesn't have an asterisk on every line, but I'm not sure what the style guide expects.

[hongkai-dai]

Reviewable status: 4 unresolved discussions, LGTM missing from assignee cohnt, needs platform reviewer assigned, needs at least two assigned reviewers

---

solvers/cost.h line 81 at r1 (raw file):

Previously, cohnt (Thomas Cohn) wrote…

nit: Usually the code I've seen in drake doesn't have an asterisk on every line, but I'm not sure what the style guide expects.

The other function in this file all start the documentation with an asterisk. So I think we should be consistent within the file.

---

solvers/cost.cc line 63 at r1 (raw file):

Previously, cohnt (Thomas Cohn) wrote…

I think we'll need some sort of protection for if the user passes in an out-of-bounds index i.

Done.

---

solvers/cost.cc line 105 at r1 (raw file):

Previously, cohnt (Thomas Cohn) wrote…

Ditto above -- make sure (i,j) is a valid index

Done.

---

solvers/cost.cc line 117 at r1 (raw file):

Previously, cohnt (Thomas Cohn) wrote…

Ditto above -- make sure i is a valid index

Done.

[cohnt]

Reviewed 5 of 5 files at r2, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee cohnt, needs platform reviewer assigned, needs at least two assigned reviewers

---

solvers/cost.cc line 107 at r2 (raw file):

                                       std::optional<bool> is_hessian_psd) {
  DRAKE_DEMAND(i >= 0 && i < Q_.rows());
  DRAKE_DEMAND(j >= 0 && j < Q_.rows());

Should be j < Q_.cols()?

[hongkai-dai]

Reviewable status: 1 unresolved discussion, LGTM missing from assignee cohnt, needs platform reviewer assigned, needs at least two assigned reviewers

---

solvers/cost.cc line 107 at r2 (raw file):

Previously, cohnt (Thomas Cohn) wrote…

Should be j < Q_.cols()?

Since Q_ is square and symmetric, Q_.rows() == Q_.cols()

[cohnt]

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers

[hongkai-dai]

+@ggould-tri for platform review please, thanks!

Reviewable status: LGTM missing from assignee ggould-tri(platform)

[ggould-tri]

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

---

solvers/cost.h line 84 at r3 (raw file):

   * a[i] = val.
   * @param i The index of the coefficient to be updated.
   * @param The value of that updated entry.

nit: inconsistent formatting

Suggestion:

   * @param i The index of the coefficient to be updated.
   * @param val The value of that updated entry.

---

solvers/cost.h line 206 at r3 (raw file):

  /**
   * Updates both Q(i, j) and Q(j, i) to val
   * @param is_hessian_psd Whether the updated Hessian matrix Q is psd.

This is not adequate documentation for this rather subtle tribool value.
It needs to explain the nullopt behaviour and the implied performance/correctness tradeoff.

Suggestion:

   *
   * @param is_hessian_psd If this is `nullopt`, the new Hessian is
   *        checked (possibly expensively) for PSD-ness.  If this is
   *        set true/false, the cost's convexity is updated to that
   *        value without checking (which may be incorrect).
   *

---

solvers/cost.h line 213 at r3 (raw file):

   */
  void UpdateHessianEntry(int i, int j, double val,
                          std::optional<bool> is_hessian_psd);

BTW: This seems like one of the rare cases where defaulting would improve readability and safety -- the default behaviour is safe, while the non-default behaviour dangerously asserts an unchecked PSD-ness. WDYT?

Suggestion:

std::optional<bool> is_hessian_psd = std::nullopt

---

bindings/pydrake/solvers/test/evaluators_test.py line 64 at r3 (raw file):

        cost.update_constant_term(new_c=2)
        self.assertEqual(cost.c(), 2)

minor: No testing of the non-None Hessian cases.

---

solvers/cost.cc line 111 at r3 (raw file):

  if (i != j) {
    Q_(j, i) = val;
  }

BTW: Setting is faster than checking and setting, and you've already done the bounds check, so the compiler will optimize out the conditional anyway.

Suggestion:

  Q_(j, i) = val;

---

solvers/test/cost_test.cc line 231 at r3 (raw file):

  EXPECT_FALSE(cost->is_convex());

  // Updates the Hessian to make it convex.

typo inconsistent verb case.

Suggestion:

  // Update the Hessian to make it convex.

---

solvers/test/cost_test.cc line 253 at r3 (raw file):

  cost->update_linear_coefficient_entry(1, 3);
  EXPECT_TRUE(CompareMatrices(cost->b(), Eigen::Vector2d(10, 3)));

minor: No testing of the non-nullopt Hessian cases.

[hongkai-dai]

Reviewable status: complete! all discussions resolved, LGTM from assignees ggould-tri(platform),cohnt

---

solvers/test/cost_test.cc line 253 at r3 (raw file):

Previously, ggould-tri wrote…

minor: No testing of the non-nullopt Hessian cases.

Done. Good call.