Add bindings for ConvexSet::CalcVolumeViaSampling

[AlexandreAmice]

This change is 

[AlexandreAmice]

+@cohnt for feature review.

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

[cohnt]

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee cohnt, needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @AlexandreAmice)

---

bindings/pydrake/geometry/test/optimization_test.py line 382 at r1 (raw file):

        self.assertGreaterEqual(sampled_volume.rel_accuracy,
                                desired_rel_accuracy)
        self.assertEqual(rect.SampledVolume(), max_num_samples)

Is this a typo? Seems like the left hand side should be sampled_volume.max_num_samples.

---

bindings/pydrake/geometry/test/optimization_test.py line 414 at r1 (raw file):

                                desired_rel_accuracy)
        self.assertEqual(rect.SampledVolume(), max_num_samples)

Looks like the test was duplicated?

[AlexandreAmice]

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

---

bindings/pydrake/geometry/test/optimization_test.py line 382 at r1 (raw file):

Previously, cohnt (Thomas Cohn) wrote…

Is this a typo? Seems like the left hand side should be sampled_volume.max_num_samples.

Done.

---

bindings/pydrake/geometry/test/optimization_test.py line 414 at r1 (raw file):

Previously, cohnt (Thomas Cohn) wrote…

Looks like the test was duplicated?

This was done intentionally. The block in test_hyperrectangle ensures that Hyperrectangle has access to the method. The block in test_sampled_volume chooses an arbitrary convex set to check that ConvexSet has the binding. I think that all the subclasses of convex set should be testing the bindings inheritted from ConvexSet, but they do not, so I don't want someone to accidentally erase the Hyperrectangle tests and no longer be testing this binding.

[cohnt]

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @AlexandreAmice)

---

bindings/pydrake/geometry/test/optimization_test.py line 414 at r1 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

This was done intentionally. The block in test_hyperrectangle ensures that Hyperrectangle has access to the method. The block in test_sampled_volume chooses an arbitrary convex set to check that ConvexSet has the binding. I think that all the subclasses of convex set should be testing the bindings inheritted from ConvexSet, but they do not, so I don't want someone to accidentally erase the Hyperrectangle tests and no longer be testing this binding.

Understood!

[jwnimmer-tri]

+@jwnimmer-tri for platform.

+(release notes: fix)

Reviewed 1 of 2 files at r1, all commit messages.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @AlexandreAmice)

---

bindings/pydrake/geometry/geometry_py_optimization.cc line 131 at r2 (raw file):

            cls_doc.ToShapeWithPose.doc)
        .def("CalcVolume", &ConvexSet::CalcVolume, cls_doc.CalcVolume.doc)
        .def("CalcVolumeViaSampling", &ConvexSet::CalcVolumeViaSampling,

Bindings need to be declared in topological dependency order. In other words, SampledVolume needs to be bound before ConvexSet is bound. Otherwise, the type signatures come out wrong (note the C++ spelling of the return type name with no hyperlink; it should be the Python spelling with a hyperlink):

---

bindings/pydrake/geometry/test/optimization_test.py line 414 at r1 (raw file):
Duplication is harmful to upkeep over time.

I don't want someone to accidentally erase the Hyperrectangle tests and no longer be testing this binding.

In that case, keep test_sampled_volume and undo the changes in test_hyperrectangle? (Maybe rename it to test_calc_volume_via_sampling for clarity?)

There's no real risk that someone moves the CalcVolume... binding(s) from the ConvexSet base class to Hyperrectangle subclass and it goes unnoticed during code review. We don't need a unit test to catch that kind of mistake.

[jwnimmer-tri]

If you like, just let me know and I can push the fixes for my comments.

[AlexandreAmice]

Sorry for the delay.

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

---

bindings/pydrake/geometry/geometry_py_optimization.cc line 131 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Bindings need to be declared in topological dependency order. In other words, SampledVolume needs to be bound before ConvexSet is bound. Otherwise, the type signatures come out wrong (note the C++ spelling of the return type name with no hyperlink; it should be the Python spelling with a hyperlink):

Done.

---

bindings/pydrake/geometry/test/optimization_test.py line 414 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Duplication is harmful to upkeep over time.

I don't want someone to accidentally erase the Hyperrectangle tests and no longer be testing this binding.

In that case, keep test_sampled_volume and undo the changes in test_hyperrectangle? (Maybe rename it to test_calc_volume_via_sampling for clarity?)

There's no real risk that someone moves the CalcVolume... binding(s) from the ConvexSet base class to Hyperrectangle subclass and it goes unnoticed during code review. We don't need a unit test to catch that kind of mistake.

Sounds good. Perhaps in a later PR, we have a method which tests all the ConvexSet bindings using one of the concrete classes. Then every other subclass of ConvexSet would only need to check one of the methods in ConvexSet to know that it has all the other ones?

[jwnimmer-tri]

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: LGTM missing from assignee jwnimmer-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @AlexandreAmice)

---

bindings/pydrake/geometry/test/optimization_test.py line 414 at r1 (raw file):
Having refreshed my memory on the existing test code, I'd offer a few suggestions to improve overall:

(1) Make a new dedicated file convex_set_test.py that tests ConvexSex and all of its subclasses in one place, but not the other geometry.optimization bindings code e.g. IRIS. This will help focus the purpose of the file and make it easier to keep it clean.

(2) As you say, de-duplicate the testing code for "Methods inherited from ConvexSet" that's somewhat copy-pasted all over the place currently.

That could happen via one dedicated test on a representative class as you say, or it could be a helper function like ...

def _check_convex_set_base(self, dut):
        self.assertIsInstance(dut.ambient_dimension(), int)
        self.assertIsInstance(dut.IntersectsWith(dut), bool)
        self.assertIsInstance(dut.IsBounded(), bool)
        self.assertIsInstance(dut.IsEmpty(), bool)

... that each subclass calls ...

def test_hyperrectangle(self):
    rect = mut.Hyperrectangle()
    self._check_convex_set_base(rect)
    rect = mut.Hyperrectangle(lb=-self.b, ub=self.b)
    np.testing.assert_array_equal(rect.lb(), -self.b)
    ... more Hyperrectangle-unique bindings tests ...

Then every other subclass of ConvexSet would only need to check one of the methods in ConvexSet to know that it has all the other ones?

(3) Even that is more work than necessary. To test the py::class_<Hyperrectangle, ConvexSet>(m, "Hyperrectangle", cls_doc.doc) line of bindings code, all we need is self.assertIsInstance(rect, ConvexSet) somewhere, we don't actually need to call any bound functions.

[jwnimmer-tri]

Oops, I forgot the stamp.