Parallelize the generic method for checking boundedness of a convex set.

[cohnt]

The generic boundedness check iteratively tries to maximize or minimize the ith component of a vector in a set; a set is unbounded if any of these programs is unbounded. This can naturally be parallelized, which should lead to speedups for convex sets in high-dimensional spaces. (Note that this boundedness check is currently used by Spectrahedron and Intersection, so I've enabled parallelization in the test cases in those files.)

+@hongkai-dai for feature review, if you have the time?

(I've set the default behavior to not use parallelism, since for many cases, I expect the overhead of SolveInParallel will outweigh the speedup resulting from the parallelization. But I could be convinced the other way. @AlexandreAmice let me know if you have thoughts!)

---

This change is 

[AlexandreAmice]

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: 6 unresolved discussions, LGTM missing from assignee hongkai-dai, needs platform reviewer assigned, needs at least two assigned reviewers

---

geometry/optimization/convex_set.h line 101 at r1 (raw file):

  mathematical programs to check boundedness, if the derived class does not
  provide a more efficient boundedness check. */
  bool IsBounded(Parallelism parallelism = Parallelism::None()) const {

btw: You can set this to Parallelism::Max() and then choose to use fewer than Parallelism cores if the ambient dimension of the set is small.

Code quote:

Parallelism parallelism = Parallelism::None()

---

geometry/optimization/convex_set.cc line 22 at r1 (raw file):

using solvers::LinearCost;
using solvers::MathematicalProgram;
using solvers::MathematicalProgramResult;

btw if you use this using, you should change all instances of solvers::MathematicalProgramResult to MathematicalProgramResult

---

geometry/optimization/convex_set.cc line 115 at r1 (raw file):

    // All programs are the same size, so we can use static scheduling.
    std::vector<MathematicalProgramResult> results = solvers::SolveInParallel(
        prog_ptrs, nullptr, nullptr, std::nullopt, parallelism, false);

nit:

Suggestion:

    // All programs are the same size, so we use static scheduling.
    std::vector<MathematicalProgramResult> results = solvers::SolveInParallel(
        prog_ptrs, nullptr, nullptr, std::nullopt, parallelism, false /* use_dynamic_schedule */);

---

geometry/optimization/test/intersection_test.cc line 147 at r1 (raw file):

  Intersection S2(H1, H1);
  EXPECT_FALSE(S2.IsBounded(Parallelism::Max()));

In the build file, make sure that you have:

drake_cc_googletest(
    name = "convex_set_test",
    deps = [
        ":convex_set",
        "//common/test_utilities:eigen_matrix_compare",
        "//common/test_utilities:expect_throws_message",
    ],
   num_threads = 2
)

Also at some point in this file make sure that you have:

// Cross-check that our BUILD file allows parallelism.
 DRAKE_DEMAND(Parallelism::Max().num_threads() > 1);

---

geometry/optimization/test/spectrahedron_test.cc line 67 at r1 (raw file):

  Spectrahedron spect(prog);
  EXPECT_EQ(spect.ambient_dimension(), 3 * (3 + 1) / 2);
  EXPECT_TRUE(spect.IsBounded(Parallelism::Max()));

Some of theses tests need to use Parallelism::None()

Code quote:

EXPECT_TRUE(spect.IsBounded(Parallelism::Max()));

---

bindings/pydrake/geometry/geometry_py_optimization.cc line 112 at r1 (raw file):

            cls_doc.IntersectsWith.doc)
        .def("IsBounded", &ConvexSet::IsBounded,
            py::arg("parallelism") = Parallelism::None(), cls_doc.IsBounded.doc)

All methods with Parallelism need py::call_guard<py::gil_scoped_release>() to avoid potential deadlocks

Suggestion:

        .def("IsBounded", &ConvexSet::IsBounded,
            py::arg("parallelism") = Parallelism::None(),
            py::call_guard<py::gil_scoped_release>(),
             cls_doc.IsBounded.doc)

[AlexandreAmice]

-@hongkai-dai +@AlexandreAmice for feature review.

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

[AlexandreAmice]

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

---

geometry/optimization/convex_set.cc line 126 at r1 (raw file):

    }
  }
  // No optimization problems were unbounded, so we return true.

Suggestion:

// All optimization problems were bounded, so we return true.

[cohnt]

Thanks for taking on the feature review! :)

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

---

geometry/optimization/convex_set.h line 101 at r1 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

btw: You can set this to Parallelism::Max() and then choose to use fewer than Parallelism cores if the ambient dimension of the set is small.

I want to keep the default behavior to be no parallelism, since many sets we check for boundedness elsewhere inside Drake are simple enough that the overhead would be detrimental. Is it worth adding a code quote down the lines of

if (parallelism.num_threads() > 2 * ambient_dimension()) {
  parallelism = Parallelism(2 * ambient_dimension());
}

Or does Parallelism know not to set aside additional cores?

---

geometry/optimization/convex_set.cc line 22 at r1 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

btw if you use this using, you should change all instances of solvers::MathematicalProgramResult to MathematicalProgramResult

Done.

---

geometry/optimization/convex_set.cc line 115 at r1 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

nit:

Done.

---

geometry/optimization/convex_set.cc line 126 at r1 (raw file):

    }
  }
  // No optimization problems were unbounded, so we return true.

Done.

---

geometry/optimization/test/intersection_test.cc line 147 at r1 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

In the build file, make sure that you have:

drake_cc_googletest(
    name = "convex_set_test",
    deps = [
        ":convex_set",
        "//common/test_utilities:eigen_matrix_compare",
        "//common/test_utilities:expect_throws_message",
    ],
   num_threads = 2
)

Also at some point in this file make sure that you have:

// Cross-check that our BUILD file allows parallelism.
 DRAKE_DEMAND(Parallelism::Max().num_threads() > 1);

I've added this to the tests for Intersection and Spectrahedron.

---

geometry/optimization/test/spectrahedron_test.cc line 67 at r1 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

Some of theses tests need to use Parallelism::None()

I've set it up so all instances are tested with and without parallelism.

---

bindings/pydrake/geometry/geometry_py_optimization.cc line 112 at r1 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

All methods with Parallelism need py::call_guard<py::gil_scoped_release>() to avoid potential deadlocks

Done.

[AlexandreAmice]

I think you need one more test that checks IsBounded iterating through the batches. You could do something like an HPolyhedron::MakeL1Ball(100).

Reviewed 2 of 5 files at r2.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee AlexandreAmice, needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @cohnt)

---

geometry/optimization/convex_set.cc line 63 at r3 (raw file):

namespace {
void ConstructBoundednessProgram(MathematicalProgram* prog, const ConvexSet& s,
                                 int dimension, bool maximize) {

just to disambiguate with ambient_dimension

Suggestion:

int dimension_index

---

geometry/optimization/convex_set.cc line 87 at r3 (raw file):

  // minimize or maximize x[i] for each dimension i. If any solves are
  // unbounded, the set is not bounded.
  for (int i = 0; i < s.ambient_dimension(); ++i) {

can avoid the copy-paste.

Suggestion:

for (int i = 0; i < s.ambient_dimension(); ++i) {
  for(const bool maximize_i : {true, false} {
  }

---

geometry/optimization/convex_set.cc line 111 at r3 (raw file):

  // At the end of each batch, we check to see if we've identified an unbounded
  // direction. If so, we immediately return. We use a large batch size to trade
  // off the overhead of spinning of threads with the runtime of solving the

nit

Suggestion:

up

[cohnt]

Done.

Note that an L1 ball needs 2^n halfspaces, so I'm using HPolyhedron::MakeUnitBox(100).

Reviewable status: 2 unresolved discussions, LGTM missing from assignee AlexandreAmice, needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @cohnt)

---

geometry/optimization/convex_set.cc line 63 at r3 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

just to disambiguate with ambient_dimension

Done.

---

geometry/optimization/convex_set.cc line 87 at r3 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

can avoid the copy-paste.

Done.

---

geometry/optimization/convex_set.cc line 111 at r3 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

nit

Done.

[AlexandreAmice]

Reviewed 1 of 5 files at r2, 2 of 2 files at r4, all commit messages.
Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @cohnt)

[cohnt]

+@sherm1 for platform review per the schedule, please

[AlexandreAmice]

Reviewed 1 of 1 files at r5.
Reviewable status: LGTM missing from assignee sherm1(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @cohnt)

[SeanCurtis-TRI]

-@sherm1 +@SeanCurtis-TRI for platform review.

Reviewable status: LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @cohnt)

[SeanCurtis-TRI]

First pass done

Reviewed 2 of 6 files at r1, 3 of 5 files at r2, 1 of 2 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: 10 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @cohnt)

a discussion (no related file):
The one thought that concerns me is the user's ability to inform their expectations on whether a particular convex set actually honors the parallelism request. There are lots of implementations of DoIsBoundedShortcut(). However, the public API doesn't indicate this at all.

It'd be a shame for a user to set parallelism and then get confused as to why it doesn't seem to make a difference.

One solution is for each class to explicitly document whether it honors parallelism. Another is to give an API where a convex set reports whether it respects parallelism. (This is probably a bit heavy handed.) Final option is to put a note at the declaration of IsBounded() that derived classes that honor parallelism document it, otherwise there is no expectation that it'll make a difference.

---

a discussion (no related file):
ConvexHull::DoIsBoundedShortcut() returns std::nullopt. That means it always falls through to the GenericDoIsBounded() implementation. Should that not also have a parallelism test in its test?

---

---

geometry/optimization/convex_set.h line 97 at r5 (raw file):

  (typically small) optimization problems. Check the derived class documentation
  for any notes.

BTW The note in the commit message I think would be an appropriate user message. A warning that parallelism is not a panacea and the user should confirm that increased parallelism actually helps them and not assumes that it does.

Alternatively, if you think users of GOCS are sufficiently parallelism-savvy that this would be immediately obvious to them, you can leave it out.

---

geometry/optimization/convex_set.h line 98 at r5 (raw file):

  for any notes.

  @param parallelism specifies the number of cores to use when solving

nit As this is not a contract, a different word may be appropriate.

Suggestion:

requests

---

geometry/optimization/convex_set.h line 99 at r5 (raw file):
btw: The phrase, "if the derive class does not provide...." is a bit cumbersome.

The sense I get is that a derived class is allowed to opt out of parallel computation. Possible reasons:

• Parallelism is unnecessary because there's a nice cheap way to do it.
• Parallelism may be impractical (this is an imagined one; I'm sure we don't have any examples of this).

Perhaps it would be better phrased as:

Requests the number of cores to use when determining boundedness. Derived classes may opt out of parallelism.

or, based on my suggestion above:

Requests the number of cores to use when determining boundedness. Derived classes that honor the request explicitly say so in their documentation.

---

geometry/optimization/convex_set.cc line 114 at r5 (raw file):

  // unbounded, the set is not bounded.
  int num_batches = 1 + (n / batch_size);
  for (int batch_idx = 0; batch_idx < num_batches; ++batch_idx) {

BTW I feel the index semantics could be more easily understood. You're implicitly measuring batch size as "number of dimensions" but creating twice as many mathematical programs because you're doing one direction per dimension. That makes the concept of "batch size" seem like a lie as we index mathematical programs at indexes greater than batch_start + batch_size.

Perhaps it would be better to say dimensions_per_batch and then the batch_size is simply the number of mathematical programs (2 * dimensions_per_batch)

At the very least, you should augment your documentation to be clear that "batch size" is not the same as "number of mathematical programs per batch". And a further note indicating how you're organizing the mathematical programs (e.g., positive dimension extents in first block, negative in second).

Alternatively, if you made this set up look more like the the serial set up where you also iterate through maximize/minimize, it might be easier for someone to read and grok both methods as they re-enforce each other.

But something to help clarify the approach here would aid future readers.

---

geometry/optimization/convex_set.cc line 119 at r5 (raw file):

    int this_batch_size = batch_end - batch_start;

    std::vector<MathematicalProgram> progs(2 * this_batch_size);

BTW I don't know how much you're trying to squeeze the overhead. But declaring the vector once for each batch leads to allocation per batch. If you declare it on the outside and then resize it to the current batch size, you'll only ever get a single allocation. You'll still have to re-initialize the values.

---

geometry/optimization/convex_set.cc line 138 at r5 (raw file):

                                 parallelism, false /* use_dynamic_schedule */);
    for (const auto& result : results) {
      if (result.get_solution_result() == solvers::SolutionResult::kUnbounded ||

nit: This should be using ProgramResultImpliesUnbounded().

---

geometry/optimization/test/intersection_test.cc line 293 at r5 (raw file):

}

GTEST_TEST(IntersectionTest, BoundedCheckBatching) {

nit We need some note indicating that logic in convex_set is actually being tested in intersection_test.cc. (Assuming there's no straightforward way to test ConvexSet implementation in convex_set_test.cc.

---

geometry/optimization/test/intersection_test.cc line 294 at r5 (raw file):

GTEST_TEST(IntersectionTest, BoundedCheckBatching) {
  // Check that a high dimensional intersection can be properly checked for

I'm not fully grokking this test.

As I understand it, this test is attempting to confirm the batching logic is correct. For this to be interesting, we need the following:

1. We need to make sure that multiple batches get generated.
2. We need to make sure that the determination of unboundedness doesn't occur in the first batch (just so that subsequent batches matter).
3. We should make sure that not all batches are the same size (to confirm the oddball final batch gets handled correctly).
4. The brass ring is to put the unbounded dimension in the vector in a place where the batches would be most likely to make an error.

Do I have that right?

Assuming I do:

1. This is going to require asserting the number of threads, knowing the batch size based on the thread, and making sure the number of dimensions is large enough to provide multiple batches. Most of that will be hard-coded assertions based on looking at the implementation code.
2. You've only got one unbounded example and, presumably, it is detected in the last batch. We should be clear about this.
3. I'm pretty sure that bounded has uniformly sized batch sizes and unbounded has non-uniform sized batch sizes. You've correlated the "boundedness" with this "batch-size" property. The test should test it the other direction as well (confirming that it's not a batch logic property that is determining boundedness).
4. This is largely theoretical. I'd imagine if there were such an interesting "place" it would be the first or last mathematical program in any batch. I'm not sure how interesting that is to justify the effort.

Ultimately, if that is all sane, this test isn't doing enough of the right things. If that was all nonsense, I don't understand the purpose of this test and that could use clarification.

[jwnimmer-tri]

FYI on reason the batch thing is difficult to understand (and the sequential case is broken-slow now) is because SolveInParallel is not designed to be used when you are creating programs on the fly until you find a counterexample. Since we are writing C++ here, we could write a proper par-for loop instead of trying to use a helper function with this impedance mismatch.

Reviewable status: 13 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @cohnt)

---

geometry/optimization/convex_set.cc line 74 at r5 (raw file):

}

inline bool ProgramResultImpliesUnbounded(

nit Remove useless inline directive. Functions defined at the point of declaration are always inline.

---

geometry/optimization/convex_set.cc line 90 at r5 (raw file):

    for (const bool maximize_i : {true, false}) {
      MathematicalProgram prog;
      ConstructBoundednessProgram(&prog, s, i, maximize_i /* maximize */);

This is likely to be vastly slower than the existing conditions on master. On master, we make one program and update its linear cost's coefficients. In IsBoundedSequential here now, we are creating a new program from scratch every single time.

---

geometry/optimization/convex_set.cc line 114 at r5 (raw file):

  // unbounded, the set is not bounded.
  int num_batches = 1 + (n / batch_size);
  for (int batch_idx = 0; batch_idx < num_batches; ++batch_idx) {

nit idx is not a GSG-valid abbreviation.

https://drake.mit.edu/styleguide/cppguide.html#General_Naming_Rules

[jwnimmer-tri]

... we could write a proper par-for loop instead of trying to use a helper function with this impedance mismatch.

Indeed, thinking more about it now, the obvious solution is to create a pool N of programs (N is number of threads) and only ever call UpdateCoefficients inside of the par-for loop.

Also, isn't the complexity class of the program the same every time? I would think we want to GetAvailableSolvers on it up front (nixing Ipopt), creating N of the best solver ahead of time, and then just calling Solve on it directly. Doing the ChooseBestSolver logic each time seems wasteful.

[cohnt]

Thanks for the suggestions! I'll take a crack at rewriting this with a par-for loop in the next couple days. Keeping a pool of programs and solvers should be a lot faster.

When I do that, I'll also make fixes regarding the other comments (since presumably, some will no longer be relevant).

Reviewable status: 13 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)

a discussion (no related file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

ConvexHull::DoIsBoundedShortcut() returns std::nullopt. That means it always falls through to the GenericDoIsBounded() implementation. Should that not also have a parallelism test in its test?

I think ConvexHull was introduced after ConvexSet::IsBounded(), so I'm not sure why it doesn't have any test cases. I've added some

---

[AlexandreAmice]

Reviewable status: 17 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @cohnt)

---

geometry/optimization/convex_set.cc line 151 at r8 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

I suspect it's fine. The time to read the atomic value is miniscule, particularly compared to the time to do the program. And, given that once a thread has read "unbounded state", it will never query the atomic lock again, you won't get any locks on subsequent problems.

The original implementation had theatomic lock being queried every time even when the unbounded state was read. I worried that if the unbounded state was hit, then all the threads querying the lock in sequence could get a bit slow. I am satisfied with the new implementation.

[cohnt]

On it! Very excited to get this merged -- thanks for all your help!

I think I've gotten all the virtual functions out of the header files.

Reviewable status: 10 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)

a discussion (no related file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

I've flagged one of instances, but they all suffer from the same problem:

When you use using ConvexSet::IsBounded(), doxygen doesn't print it with the Parallelism parallelism parameter. So, references to that named parameter are ineffectual (screen grab below).

Turns out if we explicitly add @param parallelism ... in the documentation, doxygen figures out that it should include the parameter, at least in the list of member functions. But it still doesn't show it in the detailed documentation, so I've dropped in a note and link to the base class documentation. Here's how it looks:

Is this an acceptable solution? If so, I'll update the remaining classes to match.

---

---

geometry/optimization/cartesian_product.h line 94 at r10 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: Typo?

This description is very "implementation" heavy. You could keep at a conceptual level with:

This class honors requests for parallelism only so far as its constituent sets do.

I like the conceptual description.

---

geometry/optimization/convex_hull.h line 69 at r10 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: Same note as on cartesian_product.h.

Done.

---

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

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: This phrase is ambiguous. "Can be" implies there's a "can't be" and that suggests that requesting parallelism would be...a problem? I'd propose the following:

  /**  ...
  ...
  (typically small) optimization problems. Each derived class documents the cost
  of its boundedness test and whether it honors the request for parallelism.
  (Derived classes which do not have a specialized check will, by default, honor
  parallelism requests.) Note that the overhead of multithreading may lead to
...

Done.

---

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

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: Grammar.

Taken as is, "method" should be plural.

Taking a further step back. It's worth noting that it's not a question of "methods" that support it, but derived convex sets that support it.

requests the number of cores to use when solving mathematical programs to check boundedness, subject to whether a particular derived class honors parallelism.

(This re-uses the "honor parallelism" language you used above.)

Done.

---

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

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Note: the new language doesn't "promise" how the parallelism is used. It specifically says that the parameter constitutes a "request" and has the caveat about "methods that can support it".

My preference is Sean's wording above

---

geometry/optimization/convex_set.h line 329 at r10 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

nit: spurious whitespace change.

Removed, and modified the DoIsBoundedShortcutParallel documentation to match.

---

geometry/optimization/convex_set.cc line 158 at r8 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

I think this is a case showing why being able to mutate the A vector manually may be a good thing. Like I said, the larger the ambient dimension, the more important parallelization should be, but the more this repeated allocation is going to cost us.

cc @hongkai-dai for thoughts.

Maybe we can merge the current version (even with this inefficiency, this should be much faster). And I can drop a TODO pending a more efficient method to change the coefficients of an evaluator?

---

geometry/optimization/convex_set.cc line 71 at r9 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW Possibly worth a note indicating in what sense the program is "empty".

Done.

---

geometry/optimization/convex_set.cc line 102 at r9 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW A tiny optimization you might consider. The object vector starts all zeros. Each for loop sets just one entry. Rather than setting zeros across the whole vector at the beginning of each loop iteration, why not simply reset the one entry the iteration used back to zero at the end of each iteration?

Entering and exiting the loop with the objectives all being zero is kind of a nice loop invariant.

This same trick works in the parallel variant, thus eliminating objective_vector_nonzero_indices variable.

Nice! I'll make that change for the parallel implementation as well.

---

geometry/optimization/convex_set.cc line 120 at r9 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW This assertion is slightly confusing. Why does the parallel implementation care more about this property of ConstructEmptyBoundednessProgram() more than the serial implementation?

I think we can just remove the assertion.

---

geometry/optimization/convex_set.cc line 126 at r9 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: typo

Done.

---

geometry/optimization/vpolytope.h line 108 at r10 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: Another case where the implementation function should not have been moved out of the .cc file and into the .h file.

Done.

---

geometry/optimization/test/convex_hull_test.cc line 221 at r10 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW Perhaps also put a note here, telling readers to look into intersection_test.cc for more extensive testing.

Done.

---

geometry/optimization/test/intersection_test.cc line 302 at r10 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: I need you to elaborate on this last statement. The point of this test is to make sure you're using the parallel implementation. However, there's no way to confirm that this is the case. We can only set up circumstantial evidence.

I think you should be explicit about that. All of these steps are a best-faith effort to ensure that GenericDoIsBounded() is called, but there's no external means for confirming that.

I added a sentence to this effect. The way I confirmed that parallelism was being used properly is by pulling up a system monitor and looking at the utilization of my CPU cores, while running the python snippet I pasted in earlier in the review process. (Should I mention that fact as well, perhaps with a permalink to the review thread?)

---

solvers/gurobi_solver.cc line 957 at r8 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Go ahead and pull it back out.

Done.

[SeanCurtis-TRI]

Reviewed 15 of 15 files at r11, all commit messages.
Reviewable status: 6 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @cohnt)

a discussion (no related file):

Previously, cohnt (Thomas Cohn) wrote…

Turns out if we explicitly add @param parallelism ... in the documentation, doxygen figures out that it should include the parameter, at least in the list of member functions. But it still doesn't show it in the detailed documentation, so I've dropped in a note and link to the base class documentation. Here's how it looks:

Is this an acceptable solution? If so, I'll update the remaining classes to match.

If the function signature included the parameter (see the image below), I'd say it's a slam dunk.

However, the parameter is included in the summary:

So, the one place it's missing is the spelling under the linked bullet.

The only thing I can think of is using macros and that's probably a bridge too far.

So, let's call this good enough until we have some future epiphany. Go ahead and spread this format around to the other derived classes.

---

---

geometry/optimization/convex_set.cc line 151 at r8 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

The original implementation had theatomic lock being queried every time even when the unbounded state was read. I worried that if the unbounded state was hit, then all the threads querying the lock in sequence could get a bit slow. I am satisfied with the new implementation.

Ahhh...I'd missed the earlier version.

Quick question: this adds complexity to avoid a cost. Is this an observable cost? The theoretical issue of multiple branches all stopping to look at the bool at the same doesn't seem like it would be an appreciable, discernible cost? In other words, is this a premature optimization, or has it been justified?

---

geometry/optimization/convex_set.cc line 158 at r8 (raw file):

Previously, cohnt (Thomas Cohn) wrote…

Maybe we can merge the current version (even with this inefficiency, this should be much faster). And I can drop a TODO pending a more efficient method to change the coefficients of an evaluator?

I had a similar thought -- it's a shame that we have to write the whole cost vector over and over when we're only ever toggling a single value. But that's for future work.

---

geometry/optimization/convex_set.cc line 143 at r11 (raw file):

  // Pre-allocate the objective vectors, initially filling them with all zeros.
  std::vector<VectorXd> objective_vectors;

BTW The shorthand, RAII version of that would be:

  std::vector<VectorXd> objective_vectors(
      parallelism.num_threads(), VectorXd::Zero(s.ambient_dimension()));

[AlexandreAmice]

Reviewed 4 of 24 files at r8, 1 of 2 files at r10, 15 of 15 files at r11, all commit messages.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @cohnt)

---

geometry/optimization/convex_set.cc line 151 at r8 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Ahhh...I'd missed the earlier version.

Quick question: this adds complexity to avoid a cost. Is this an observable cost? The theoretical issue of multiple branches all stopping to look at the bool at the same doesn't seem like it would be an appreciable, discernible cost? In other words, is this a premature optimization, or has it been justified?

It hasn't been justified experimentally for this particular code.

---

geometry/optimization/convex_set.cc line 158 at r8 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

I had a similar thought -- it's a shame that we have to write the whole cost vector over and over when we're only ever toggling a single value. But that's for future work.

Can you write in a TODO for me Tommy.

---

geometry/optimization/convex_set.cc line 73 at r11 (raw file):

void ConstructEmptyBoundednessProgram(MathematicalProgram* prog,
                                      const ConvexSet& s) {
  // Create a MathematicalProgram that can be used to check if a ConvexSet is

nit: GSG grammar

Suggestion:

Creates

[SeanCurtis-TRI]

Reviewable status: 4 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @cohnt)

---

geometry/optimization/convex_set.cc line 151 at r8 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

It hasn't been justified experimentally for this particular code.

It seems it would be pretty straight forward to comment out a few lines of code and re-run the program that produced the 10X performance data listed above, yes?

[cohnt]

Assuming that the documentation is all in order, and you both are happy with just checking the std::atomic<bool> each time, I think we're done?

My proposed commit message is the following. (I'll hold off on squashing until the end.)

Leverage parallelization where possible when checking boundedness of a ConvexSet.

Reviewable status: 2 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @cohnt)

a discussion (no related file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

If the function signature included the parameter (see the image below), I'd say it's a slam dunk.

However, the parameter is included in the summary:

So, the one place it's missing is the spelling under the linked bullet.

The only thing I can think of is using macros and that's probably a bridge too far.

So, let's call this good enough until we have some future epiphany. Go ahead and spread this format around to the other derived classes.

I've added documentation along these lines to all derived classes.

---

---

geometry/optimization/convex_set.cc line 151 at r8 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

It seems it would be pretty straight forward to comment out a few lines of code and re-run the program that produced the 10X performance data listed above, yes?

Current version results:

Bounded 1 threads True 4.376310110092163
Bounded 20 threads True 0.39354443550109863
Unbounded 1 threads False 4.909194231033325
Unbounded 20 threads False 0.6464524269104004

Results where we check the std::atomic<bool> every time:

Bounded 1 threads True 4.372156381607056
Bounded 20 threads True 0.4158918857574463
Unbounded 1 threads False 4.911894798278809
Unbounded 20 threads False 0.652733325958252

So it looks better in the current version. But then I ran it again:

Bounded 1 threads True 4.3535377979278564
Bounded 20 threads True 0.39914822578430176
Unbounded 1 threads False 4.901643753051758
Unbounded 20 threads False 0.6300508975982666

Looks pretty much indistinguishable, so let's just stick to keeping around a single atomic bool.

---

geometry/optimization/convex_set.cc line 158 at r8 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

Can you write in a TODO for me Tommy.

I dropped a TODO for myself (here and in the sequential one), with a link to an issue requesting the needed interface. #22131

---

geometry/optimization/convex_set.cc line 73 at r11 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

nit: GSG grammar

Done.

---

geometry/optimization/convex_set.cc line 143 at r11 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW The shorthand, RAII version of that would be:

  std::vector<VectorXd> objective_vectors(
      parallelism.num_threads(), VectorXd::Zero(s.ambient_dimension()));

Done.

[SeanCurtis-TRI]

One last thing on the documentation.

And a question about the precision/correctness of the MinkowskiSum documentation.

Reviewed 13 of 13 files at r12, all commit messages.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @cohnt)

---

geometry/optimization/affine_ball.h line 123 at r12 (raw file):

  /** Every AffineBall is bounded by construction.
  @param parallelism Ignored -- no parallelization is used.
  @note See ConvexSet::IsBounded for documentation of the docstring. It does not

nit: I love the back reference to the base class. However,I'm going to push you away from implementation and more towards ideas. We really don't want to be talking about doxygen, rendering issues and "doc strings" in the rendered documentation.

  @note See @ref ConvexSet::IsBounded "parent class's documentation" for more
  details. */

Looks like this:

---

geometry/optimization/convex_set.cc line 151 at r8 (raw file):

Previously, cohnt (Thomas Cohn) wrote…

Current version results:

Bounded 1 threads True 4.376310110092163
Bounded 20 threads True 0.39354443550109863
Unbounded 1 threads False 4.909194231033325
Unbounded 20 threads False 0.6464524269104004

Results where we check the std::atomic<bool> every time:

Bounded 1 threads True 4.372156381607056
Bounded 20 threads True 0.4158918857574463
Unbounded 1 threads False 4.911894798278809
Unbounded 20 threads False 0.652733325958252

So it looks better in the current version. But then I ran it again:

Bounded 1 threads True 4.3535377979278564
Bounded 20 threads True 0.39914822578430176
Unbounded 1 threads False 4.901643753051758
Unbounded 20 threads False 0.6300508975982666

Looks pretty much indistinguishable, so let's just stick to keeping around a single atomic bool.

Looks like a wash to me. Hooray for simpler code.

---

geometry/optimization/minkowski_sum.h line 67 at r12 (raw file):

  /** A MinkowskiSum is bounded if all its constituent sets are bounded. If any
  are unbounded, the generic method for checking boundedness is used. This class

nit: the statement that "unbounded" implies "generic method" isn't true. We can have a member set that is unbounded but it won't use the generic method.

Furthermore, I'd assume that the presence of a single unbounded set would imply the minkowski sum is likewise unbounded. What have I missed?

THere's a possible additional weirdness. If I take the Minkowski sum with an empty set, the result is empty, yes? In that case, I can take the minkowski sum of an unbounded set and an empty set and end up with a bounded set.

I think we can/should be a bit more careful with this language.

Code quote:

  /** A MinkowskiSum is bounded if all its constituent sets are bounded. If any
  are unbounded, the generic method for checking boundedness is used. This class

[cohnt]

Reviewable status: 1 unresolved discussion, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @cohnt)

---

geometry/optimization/affine_ball.h line 123 at r12 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: I love the back reference to the base class. However,I'm going to push you away from implementation and more towards ideas. We really don't want to be talking about doxygen, rendering issues and "doc strings" in the rendered documentation.

  @note See @ref ConvexSet::IsBounded "parent class's documentation" for more
  details. */

Looks like this:

Done.

---

geometry/optimization/minkowski_sum.h line 67 at r12 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: the statement that "unbounded" implies "generic method" isn't true. We can have a member set that is unbounded but it won't use the generic method.

Furthermore, I'd assume that the presence of a single unbounded set would imply the minkowski sum is likewise unbounded. What have I missed?

THere's a possible additional weirdness. If I take the Minkowski sum with an empty set, the result is empty, yes? In that case, I can take the minkowski sum of an unbounded set and an empty set and end up with a bounded set.

I think we can/should be a bit more careful with this language.

Looks like I had copied this documentation over from intersection by accident. I've fixed it.

As for the case of an empty MinkowskiSum which has unbounded components, this is an existing bug in drake -- good catch! I've added a test case revealing the bug, along with implementing the fix (and the documentation to note this edge case).

[SeanCurtis-TRI]

Outstanding. As soon as CI is done, you're good to go.

Reviewed 14 of 14 files at r13, all commit messages.
Reviewable status: LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @cohnt)

[SeanCurtis-TRI]

+(release notes: fix) (For the MinkowskiSum::IsEmpty() correction.)

Reviewable status: LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @cohnt)

[SeanCurtis-TRI]

Reviewable status: labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @cohnt)