Fix ConvexSet move operations

[jwnimmer-tri]

Requires #19321. Closes #19310. Towards #19279.

---

This change is 

[jwnimmer-tri]

Reviewed 23 of 23 files at r1, all commit messages.
Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @jwnimmer-tri)

[jwnimmer-tri]

\CC @RussTedrake FYI, or in case you'd like to feature-review.

[jwnimmer-tri]

Reviewed 6 of 6 files at r2, all commit messages.
Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @jwnimmer-tri)

[jwnimmer-tri]

On second look, there are still several lingering bugs related to zero-dimensional set handling. I've converted back to "Draft" while I work on those.

[jwnimmer-tri]

Reviewed 26 of 26 files at r3, all commit messages.
Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge" (waiting on @jwnimmer-tri)

[jwnimmer-tri]

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge" (waiting on @jwnimmer-tri)

[jwnimmer-tri]

Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge"

---

geometry/optimization/convex_set.cc line 47 at r6 (raw file):

    const Eigen::Ref<const solvers::VectorXDecisionVariable>& x,
    const symbolic::Variable& t) const {
  DRAKE_THROW_UNLESS(ambient_dimension() > 0);

Working

Some unit tests are hitting this.

Either they are incorrectly formulated (in which case we should fix the test), or they really do need to add this vacuously false constraint, in which case we need to figure out how to add a vacuously false constraint to MathematicalProgram without it throwing an exception.

[jwnimmer-tri]

Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge"

---

geometry/optimization/convex_set.cc line 47 at r6 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Working

Some unit tests are hitting this.

Either they are incorrectly formulated (in which case we should fix the test), or they really do need to add this vacuously false constraint, in which case we need to figure out how to add a vacuously false constraint to MathematicalProgram without it throwing an exception.

Actually, moving this discussion to #19321.

[jwnimmer-tri]

+@RussTedrake for feature review or delegation, please.

[jwnimmer-tri]

Reviewed 3 of 25 files at r7, 29 of 29 files at r8, all commit messages.
Reviewable status: LGTM missing from assignee RussTedrake(platform), needs at least two assigned reviewers (waiting on @jwnimmer-tri and @RussTedrake)

[RussTedrake]

geometry/optimization/convex_set.h line 244 at r8 (raw file):

 private:
  reset_after_move<int> ambient_dimension_;

in your comments about threading, you said "readability/maintainability of the code may trump other concerns". In my view, you are making something that should be simple look very complicated. (here an in the lines above). I think it's unlikely that developers will get this right out of the box. I've just now read the reset_after_move docs (i was not aware)... but don't know if that is enough for me to justify this complexity?

[jwnimmer-tri]

Reviewable status: 1 unresolved discussion, LGTM missing from assignee RussTedrake(platform), needs at least two assigned reviewers

---

geometry/optimization/convex_set.h line 244 at r8 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

in your comments about threading, you said "readability/maintainability of the code may trump other concerns". In my view, you are making something that should be simple look very complicated. (here an in the lines above). I think it's unlikely that developers will get this right out of the box. I've just now read the reset_after_move docs (i was not aware)... but don't know if that is enough for me to justify this complexity?

It didn't read as complicated at all to me, but let's run with the idea anyway.

If we assume that this is too difficult as-is, I'm not sure where you're going with the thought process. Maybe we could add more documentation nearby here, to better explain? Something like:

  // When this object is moved from (via the move constructor or move
  // assignment operator), the reset_after_move wrapper will reset the
  // ambient dimension to zero. This is important because subclasses
  // that are composite sets (e.g., MinkowskiSum) will have their
  // child sets moved away, leaving them with no dimensions to report.

The bottom line is that the move constructors are all broken on master. Unless we have a fix that ends up with this same effect (however we spell it), the only other alternative is to remove (= delete) all of the move operators on ConvexSet and all of its subclasses, leaving only the copy operators. Is that a better fix than this approach?

[RussTedrake]

Reviewed 3 of 25 files at r7, 29 of 29 files at r8, all commit messages.
Reviewable status: 1 unresolved discussion, needs at least two assigned reviewers

---

geometry/optimization/convex_set.h line 244 at r8 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

It didn't read as complicated at all to me, but let's run with the idea anyway.

If we assume that this is too difficult as-is, I'm not sure where you're going with the thought process. Maybe we could add more documentation nearby here, to better explain? Something like:

  // When this object is moved from (via the move constructor or move
  // assignment operator), the reset_after_move wrapper will reset the
  // ambient dimension to zero. This is important because subclasses
  // that are composite sets (e.g., MinkowskiSum) will have their
  // child sets moved away, leaving them with no dimensions to report.

The bottom line is that the move constructors are all broken on master. Unless we have a fix that ends up with this same effect (however we spell it), the only other alternative is to remove (= delete) all of the move operators on ConvexSet and all of its subclasses, leaving only the copy operators. Is that a better fix than this approach?

The last line of the comment helps me understand the goal. I initially failed to see the danger in having the ambient dimension have a non-zero value sticking around.

It's complicated because it suddenly asks the develops to understand a lot more about move semantics, etc. And one has to go and read the reset_after_move header file to learn more. We have important contributors to the ConvexSet classes who are still coming up to speed on the more nuanced aspects of C++, and I don't want to scare them all away.

I think adding the comment here provides sufficient justification for the added complexity.

[jwnimmer-tri]

+@sherm1 for platform review per schedule, please.

Reviewable status: 1 unresolved discussion, LGTM missing from assignee sherm1(platform)

---

geometry/optimization/convex_set.h line 244 at r8 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

The last line of the comment helps me understand the goal. I initially failed to see the danger in having the ambient dimension have a non-zero value sticking around.

It's complicated because it suddenly asks the develops to understand a lot more about move semantics, etc. And one has to go and read the reset_after_move header file to learn more. We have important contributors to the ConvexSet classes who are still coming up to speed on the more nuanced aspects of C++, and I don't want to scare them all away.

I think adding the comment here provides sufficient justification for the added complexity.

Done.

Thanks, that's good context. I've expanded the comment to help guide newer developers.

[sherm1]

Platform with two suggestions

Reviewed 2 of 25 files at r7, 27 of 29 files at r8, 3 of 3 files at r9.
Reviewable status: 2 unresolved discussions

---

geometry/optimization/test/cartesian_product_test.cc line 80 at r9 (raw file):

  // The old set is in a valid but unspecified state. For convenience we'll
  // assert that it's empty, but that's not the only valid implementation,
  // just the one we happen to currently use.

BTW I don't think it's necessary or helpful to talk here about other implementations we might have done. Consider removing the second sentence (META). (Also the current behavior seems like the obvious choice -- likely everyone will be happy with it forever.)

---

geometry/optimization/test/spectrahedron_test.cc line 230 at r9 (raw file):

  EXPECT_NO_THROW(dut.AddPointInSetConstraints(
      &constraints, constraints.NewContinuousVariables<6>()));
  EXPECT_EQ(constraints.positive_semidefinite_constraints().size(), 1);

BTW not clear to me what this additional test is accomplishing -- can you add a comment explaining?

[jwnimmer-tri]

Reviewed 3 of 3 files at r9, 1 of 1 files at r10, 1 of 1 files at r11, all commit messages.
Reviewable status: complete! all discussions resolved, LGTM from assignees sherm1(platform),RussTedrake(platform)

---

geometry/optimization/test/cartesian_product_test.cc line 80 at r9 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW I don't think it's necessary or helpful to talk here about other implementations we might have done. Consider removing the second sentence (META). (Also the current behavior seems like the obvious choice -- likely everyone will be happy with it forever.)

In general I think it's important for unit tests to make it clear when they are asserting something beyond the documented contract of the API.

Would there be a less confusing phrasing you can think of?

---

geometry/optimization/test/spectrahedron_test.cc line 230 at r9 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW not clear to me what this additional test is accomplishing -- can you add a comment explaining?

Done.

[sherm1]

geometry/optimization/test/cartesian_product_test.cc line 80 at r9 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

In general I think it's important for unit tests to make it clear when they are asserting something beyond the documented contract of the API.

Would there be a less confusing phrasing you can think of?

IMO the first sentence achieved that adequately. Several of the new tests had just the first sentence for the same condition; I thought that was fine.

[jwnimmer-tri]

geometry/optimization/test/cartesian_product_test.cc line 80 at r9 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

IMO the first sentence achieved that adequately. Several of the new tests had just the first sentence for the same condition; I thought that was fine.

Right, those were the tests that only tested the "valid but unspecified" predicate. In the tests that assert "empty" instead of "unspecified", we have the extra text. I think I'm OK leaving it.