Common Iris Entrypoint Via Abstract Classes

[AlexandreAmice]

This is a design proposal towards resolving #21821. In this implementation, we have an IrisInterface which is very flexible. This class based implementation will hopefully facilitate code reuse, testing, and overhead when constructing multiple regions.

---

This change is 

[AlexandreAmice]

+(status: do not review)

CC @wernerpe @cohnt @RussTedrake @rhjiang @sadraddini for feedback.

Currently, I left most stubs unimplemented, but hopefully it shouldn't take too much imagination to see how the implementations would go. Holding off on writing tests and full-fledged working code until I get feedback on the design.

The idea is that IrisInterface is a templated, abstract class. Every implementation of IrisInterface can define its own option struct which must inherit from IrisInterfaceOptions. This way, one always has access to the IrisInterfaceOptions, but I can't pass a C_IrisOptions to IrisNp2.

Happy to quibble about naming conventions if we think this is a good design. The names I used are not that good.

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)

[AlexandreAmice]

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), missing label for release notes (waiting on @AlexandreAmice)

---

planning/iris/c_iris.h line 22 at r1 (raw file):

      binary_search_options;

  // Options for running binary search during DoSetup which expands the seed

Bilinear alternations

Code quote:

binary search

[wernerpe]

Reviewable status: 2 unresolved discussions, 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)

---

planning/iris/iris_interface.h line 32 at r1 (raw file):

        options.domain.value_or(geometry::optimization::HPolyhedron::MakeBox(
            checker_->plant().GetPositionLowerLimits(),
            checker_->plant().GetPositionUpperLimits()));

I am a bit confused here. I would have expected the seed configuration to show up here as an argument. Furthermore, how are we going to handle the vanilla Iris case here. The domain will likely be nonsensical in this case. Should we maybe pass that in as an argument?

Code quote:

  geometry::optimization::HPolyhedron BuildSet(
      const IrisInterfaceOptionsSubclass& options) {
    geometry::optimization::HPolyhedron set =
        options.domain.value_or(geometry::optimization::HPolyhedron::MakeBox(
            checker_->plant().GetPositionLowerLimits(),
            checker_->plant().GetPositionUpperLimits()));

[AlexandreAmice]

Reviewable status: 2 unresolved discussions, 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)

---

planning/iris/iris_interface.h line 32 at r1 (raw file):

Previously, wernerpe (Peter Werner) wrote…

I am a bit confused here. I would have expected the seed configuration to show up here as an argument. Furthermore, how are we going to handle the vanilla Iris case here. The domain will likely be nonsensical in this case. Should we maybe pass that in as an argument?

Why would the seed configuration be part of the initial domain? This is just the bounding box that all polytopes grow in. For the vanilla iris case, I would say that the method should throw during Setup if the domain is not set.

[jwnimmer-tri]

I'm not sure if you're looking for C++ advice here yet, but I posted two comments below. In short, I think the options handling will need some rework. If the goal here is supposed to be the IRIS algorithm spellings, rather than the mechanics of options passing, then it's fine to ignore/dismiss my comments.

Reviewable status: 4 unresolved discussions, 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)

---

planning/iris/iris_interface.h line 17 at r1 (raw file):

          typename = std::enable_if_t<std::is_base_of<
              IrisInterfaceOptions, IrisInterfaceOptionsSubclass>::value>>
class IrisInterface {

As a rule of thumb, mixing templates with inheritance is generally a major headache over the long term. If at all possible, virtual base classes should never be templated, or at least not templated with an open-ended type (as compared to, e.g., a closed set of scalar types double/autodiff/symbolic).

---

planning/iris/iris_via_collisions_and_ellipsoid_interface_options.h line 16 at r1 (raw file):

struct IrisViaCollisionsAndEllipsoidInterfaceOptions
    : public IrisInterfaceOptions {

Inheritance of options structs is generally impossible.

At the very least, the base class must have a virtual destructor per GSG, but there are piles of other problems with it in the long run.

Approaches that work better are:

(1) has-a instead of has-a: the IrisViaCollisionsAndEllipsoidInterfaceOptions could have a field IrisInterfaceOptions base_options, instead of inheriting from it

(2) Put all of the options into one base struct (IrisInterfaceOptions) and have concrete implementations throw when meaningful options (like cspace obstacles) have been set but the implementation can't handle it.

(3) Pass options using a helper class along the lines of SolverOptions that freely maps name => value in a container. The option values would be stored as either a variant (like solver options) or type-erased Value<T>).

(4) Have whatever set of struct IrisOptionsFoo and struct IrisOptionsBar and etc. you want, all written down separately without inheritance. In the common interface, the functions which accept options take std::variant<IrisOptionsFoo, IrisOptionsBar, ...>, listing all available flavors. If the user passes an unsupported flavor to a concrete iris that can't handle it, they get an exception.

(5) Have a single common IrisOptions struct that is passed as an argument. Any options which a specific to the concrete iris subclass as passed to its constructor anr/or setters, and are not part of the base virtual interface.

[wernerpe]

Reviewable status: 4 unresolved discussions, 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)

---

planning/iris/iris_interface.h line 32 at r1 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

Why would the seed configuration be part of the initial domain? This is just the bounding box that all polytopes grow in. For the vanilla iris case, I would say that the method should throw during Setup if the domain is not set.

No I meant the seed point should be passed as an argument and not hidden away in the options struct.

The point about the domain is a different one. I am not sure why the descision was made in IrisNp to implicitly set it to the joint limits. I think the domain should be passed because it is something you should actively be thinking about when you grow iris regions.

[AlexandreAmice]

C++ advice is very much appreciated. To me that is the crux of resolving #21821. Would you be available sometime to discuss designing this interface?

Reviewable status: 4 unresolved discussions, 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

[AlexandreAmice]

Reviewable status: 4 unresolved discussions, 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

---

planning/iris/iris_interface.h line 32 at r1 (raw file):

Previously, wernerpe (Peter Werner) wrote…

No I meant the seed point should be passed as an argument and not hidden away in the options struct.

The point about the domain is a different one. I am not sure why the descision was made in IrisNp to implicitly set it to the joint limits. I think the domain should be passed because it is something you should actively be thinking about when you grow iris regions.

I disagree. The input for Clique Inflation and C-Iris are not a seed point, and so I don't think BuildSet should take that as a mandatory input when it might be meaningless

---

planning/iris/iris_via_collisions_and_ellipsoid_interface_options.h line 16 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Inheritance of options structs is generally impossible.

At the very least, the base class must have a virtual destructor per GSG, but there are piles of other problems with it in the long run.

Approaches that work better are:

(1) has-a instead of has-a: the IrisViaCollisionsAndEllipsoidInterfaceOptions could have a field IrisInterfaceOptions base_options, instead of inheriting from it

(2) Put all of the options into one base struct (IrisInterfaceOptions) and have concrete implementations throw when meaningful options (like cspace obstacles) have been set but the implementation can't handle it.

(3) Pass options using a helper class along the lines of SolverOptions that freely maps name => value in a container. The option values would be stored as either a variant (like solver options) or type-erased Value<T>).

(4) Have whatever set of struct IrisOptionsFoo and struct IrisOptionsBar and etc. you want, all written down separately without inheritance. In the common interface, the functions which accept options take std::variant<IrisOptionsFoo, IrisOptionsBar, ...>, listing all available flavors. If the user passes an unsupported flavor to a concrete iris that can't handle it, they get an exception.

(5) Have a single common IrisOptions struct that is passed as an argument. Any options which a specific to the concrete iris subclass as passed to its constructor anr/or setters, and are not part of the base virtual interface.

1. A possible option except it makes calling base options quite long.
2. I disprefer this as there are too many permutations of options that are valid/invalid I think it could rapidly become a headache.
3. A possible solution.
4. I like this a lot.
5. I disprefer this as it splits the way options are set into two different fields that may be confusing to keep track of.

Thoughts cc @cohnt @wernerpe

[jwnimmer-tri]

Best would be if we could discuss it online here.

I hadn't (re-)read the issue before posting. After having done so now ... from the issue:

... a single IRIS entry point which dispatches to the proper variant based on the IrisOptions.

This screams an obvious answer to me:

(1) struct IrisOptions has all of the config knobs merged in one place (the union of all possible options).

(2) A new (non-virtual) class IrisThing entrypoint has functions (verbs along the lines of what is seen here) that accept struct IrisOptions and do the work. The function looks at the options, decides which variation should be delegated to, and calls it. In other words, this wrapper entrypoint depends (in bazel) on all of Drake's IRIS implementations, the calls the right one.

I do not understand what problem any inheritance is trying to solve.

[AlexandreAmice]

(1) struct IrisOptions has all of the config knobs merged in one place (the union of all possible options).

A variant of this design is in #21852. I dislike the solution here as many of the options are conflicting. For example, some algorithms take as input an initial point and an initial ellipse. Other algorithms take as input an initial polytope. Another takes as input a set of points.

Also the behavior of certain options changes depending on the algorithm. For example require_sample_point_contained might mean "terminate the loop and immediately return the region if you can't guarantee this" or it might mean "this algorithm can enforce this condition at every step if you ask it to. If you don't ask it to, you might end up with a different region"

The other think I dislike about this solution is that the option struct is going to become so large that it will be almost impossible to navigate.

(2) A new (non-virtual) class IrisThing entrypoint has functions (verbs along the lines of what is seen here) that accept struct IrisOptions and do the work. The function looks at the options, decides which variation should be delegated to, and calls it. In other words, this wrapper entrypoint depends (in bazel) on all of Drake's IRIS implementations, the calls the right one.

Different algorithms require holding onto very different information. For example C_Iris which is a reimplementation of CspaceFreePolytope requires holding onto all of the rational forward kinematics objects, separating planes, etc. On the other hand IrisNp which is a reimplementation of IrisInConfigurationSpace should only really store the information that is computed in lines 553-596 of the file iris.h. To me this means making a single class that represents all the algorithms somewhat challenging.

I do not understand what problem any inheritance is trying to solve.
My points above emphasize the differences between the algorithms, but there are a lot of similarities at least at the concept level. All the algorithms need to do an initial setup phase, then iterate between improving the polytope and improving the metric. I am trying to avoid each particular algorithm having to reimplement every step themselves. Of course this could be fixed by judicious use of helper functions, but I was hoping that having an inheritence based solution could help users navigate the differences and similarities between the algorithms.

Reviewable status: 4 unresolved discussions, 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

[jwnimmer-tri]

... [single options struct] ...

If making one struct is too confusing, that's fine.

In that case, make enough different structs so that they are not confusing, and the common entry point accepts a variant<Struct1, Struct2, ...> options.

In cases where there is commonality in the options, I think copy-paste between the structs is probably a good thing (it is most clear for the users), but if you really want code reuse for options then use "has-a" with e.g. IrisBaseOptions base_options as a member of the structs.

Possibly this common entry point should have a SetOptions(options) or SetUp(options) function, instead of passing the same thing to every different verb method.

Also remember that functions can take named arguments. The options struct is good for setting up long-lived data that will be shared across many operations. When a function just needs a parameter, pass it as a one-time parameter, don't put it into an Options struct. Part of the disease in existing GCS-adjacent code lately is taking things that should be function parameters and wrapping them into a struct. Structs are for things like CollisionCheckerParams with the "which robot are we going to be planning for" at construction-time, which is long-lived option data. Arguments that change every time you call this function should be parameters, not options structs.

To me this means making a single class that represents all the algorithms somewhat challenging.

Those other algorithms would still be their own classes. The common entry point merely composes them (e.g., with a variant), it does not actually have lots of code.

[AlexandreAmice]

Those other algorithms would still be their own classes. The common entry point merely composes them (e.g., with a variant), it does not actually have lots of code.

Is this a time when RTTI is considered acceptable then? Or does the variant avoid the need to do RTTI?

Reviewable status: 4 unresolved discussions, 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

[jwnimmer-tri]

The verboten use of runtime types is branching on them, as with dynamic_cast. I don't foresee anywhere we'd need to do that as part of the proposed design.

[AlexandreAmice]

Those other algorithms would still be their own classes. The common entry point merely composes them (e.g., with a variant), it does not actually have lots of code.

Sorry I am not sure I understand. If every class implements its own algorithm but is called by a common method wouldn't the method need to construct the class? In which case we lose out on the advantage of the class?

Reviewable status: 4 unresolved discussions, 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

[jwnimmer-tri]

No problem, I'll write up a more specific idea with a code sample to help explain -- but first let me check one thing, so I can match my proposal with the what's needed.

I see that this proposal has IrisInterface::BuildSet method, which steps through calling Setup then CheckTermination, etc. sensibly. As given here, those lower-level operations (setup, check, improve, metric, etc.) are exposed to public users. Is that on purpose, or just a shortcut for this illustration? In other words, is it a design goal to let users call those lower-level operations themselves? Maybe they would want to call them in a slightly different sequence, or with different logging or visualization, something like that?

Assuming they are intended to be public helpers, why is geometry::optimization::HPolyhedron* set passed by rote to all of them, instead of being a member field? Having it as an argument means that the implementation cannot rely on any assumptions about it. It needs to validate / preprocess / sanity check the input argument on every single call, and it can't cache any intermediate conclusions for the next call to improve / check.

[AlexandreAmice]

Initially I had done away completely with Setup, CheckTermination, etc. and only had the protected Do* variants. I don't think having them publically exposed is necessary, but I think others should weigh in.

Reviewable status: 4 unresolved discussions, 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

[cohnt]

I think having them hidden from the user should be fine -- they generally don't make sense out-of-sequence. The only case that comes to mind where we would want the intermediate values is if we were doing some sort of logging or inspection of the information within the algorithm. But I feel like there are more elegant ways of accomplishing that (like logging or storing the statistics somewhere).