Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create a Single Entrypoint for the Various IRIS Algorithms #21852

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

cohnt
Copy link
Contributor

@cohnt cohnt commented Aug 27, 2024

This is the beginning of a collective effort to resolve #21821.

For now, we're just creating that single entrypoint function, and showing that it can call IrisInConfigurationSpace properly. This isn't really close to being ready to merge into Drake in its current state, but hopefully I can start getting comments and suggestions from anyone who's involved with the various IRIS variants, and/or is a potential user. CC @rhjiang @wernerpe @RussTedrake @AlexandreAmice @sadraddini


This change is Reviewable

@cohnt cohnt added status: do not review release notes: feature This pull request contains a new feature labels Aug 27, 2024
Copy link
Contributor

@rhjiang rhjiang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


planning/iris/iris_entrypoint.h line 20 at r1 (raw file):

  NP,      ///< The IRIS-NP algorithm, which uses nonlinear programming to grow
           ///< regions in configuration space.
  NP2,     ///< The IRIS-NP2 algorithm, an improved version of IRIS-NP.

perhaps "The IRIS-NP2 algorithm, which uses sampling to improve nonlinear search to grow regions in configuration space"


planning/iris/iris_entrypoint.h line 99 at r1 (raw file):

  /** IRIS will terminate if the change in the *volume* of the hyperellipsoid
  between iterations is less that this threshold. This termination condition can

typo: less than


planning/iris/iris_entrypoint.h line 230 at r1 (raw file):

  \b Algorithms: NP, NP2, ZO */
  int mixing_steps{10};

I think 50 would be better!

Copy link
Contributor

@AlexandreAmice AlexandreAmice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am going to advocate for a very different design. Each algorithm should be a class

class IrisRegionBuilder:
public:
  IrisRegionBuilder(const CollisionChecker& checker);
  HPolyhedron BuildRegion();

protected:
  HPolyhedron DoBuildRegion();

I think the differences between the algorithms are too large to be a monolith.

  1. Some algorithms take an initial seed and ellipse to start. CliqueInflation take a collection of points. C-Iris takes an initial polytope.
  2. There are too many options distinct to each algorithm that just the option file will get very large and unwieldy to navigate.
  3. Doing all the necessary pre-condition checking for each algorithm is also going to bloat the single entry point.
  4. Most of the time we want to build multiple regions. Having a class-based interface avoids having to run the setup code for each individual call to the build region function.l

Having the single base class however is useful since downstream code which doesn't care how a region is built can just consume an IrisRegionBuilder.

The main design question I have in this setup is finding a clean way to tell different algorithms where to build the next region since this input is different across different algorithms.

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


planning/iris/iris_entrypoint.h line 11 at r1 (raw file):

#include "drake/geometry/optimization/hyperellipsoid.h"
#include "drake/planning/collision_checker.h"

Tag @experimental so that we can update without deprecation.


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

enum class IrisAlgorithm {
  Unset,   ///< Default value -- must be changed by the user.

Do you need to have an unset enum?


planning/iris/iris_entrypoint.h line 68 at r1 (raw file):

/**
 */
struct IrisOptions {

I don't like this monolithic IrisOptions object. The approach of solver options is to have a map from options to values that each solver individually can make sense of. Some common options are handled by a specific enum.

My preference is to makeIrisOptions a base class that each algorithm extends. The IrisOptionsImplementation subclasses should each have a virtual method Run() which actually computes the region.

The way one flags which algorithm they which to run in those cases is determined entirely by which set of options they use.


planning/iris/iris_entrypoint.h line 73 at r1 (raw file):

  /** TODO(cohnt): Document */
  IrisRegionSpace region_space{IrisRegionSpace::Unset};

I would put these in the function handle of the entry point. Users must set these so they are not optional kwargs.

Code quote:

  /** TODO(cohnt): Document */
  IrisAlgorithm algorithm{IrisAlgorithm::Unset};

  /** TODO(cohnt): Document */
  IrisRegionSpace region_space{IrisRegionSpace::Unset};

planning/iris/iris_entrypoint.h line 91 at r1 (raw file):

  \b Algorithms: All */
  bool require_sample_point_is_contained{false};

Some algorithsm (C-IRIS, CliqueInflation) can force this condition explicitly so this documentation isn't quite right.

Code quote:

  /** The polytope produced by the first iteration of each algorithm is
  guaranteed to contain the seed point, as long as that point is collision-free.
  However, subsequent iterations do not make this guarantee, so the IRIS paper
  recommends that if containment is a requirement, then the algorithm should
  simply terminate early if alternations would ever cause the set to not contain
  the point.

  \b Algorithms: All */
  bool require_sample_point_is_contained{false};

planning/iris/iris_entrypoint.h line 137 at r1 (raw file):

  \b Algorithms: All */
  geometry::optimization::ConvexSets configuration_obstacles{};

C-Iris currently does not handle Cspace obstacles.

Code quote:

  \b Algorithms: All */
  geometry::optimization::ConvexSets configuration_obstacles{};

planning/iris/iris_entrypoint.h line 144 at r1 (raw file):

  \b Algorithms: All */
  std::optional<geometry::optimization::Hyperellipsoid> starting_ellipse{};

C-Iris does not use an initial ellipse but an initial polytope.

Code quote:

  \b Algorithms: All */
  std::optional<geometry::optimization::Hyperellipsoid> starting_ellipse{};

planning/iris/iris_entrypoint.h line 249 at r1 (raw file):

geometry::optimization::HPolyhedron GrowIrisRegion(
    const CollisionChecker& checker, const IrisOptions& options,
    const Eigen::VectorXd seed);

This isn't exactly what you want. The initial point for C-Iris is actually a region not a seed point. For CliqueInflation it would be collection of points.

Code quote:

    const Eigen::VectorXd seed);

planning/iris/iris_entrypoint.cc line 27 at r1 (raw file):

  } else if (options.algorithm == IrisAlgorithm::Certified) {
    throw std::runtime_error("IrisAlgorithm::Certified is not supported yet.");
  } else if (options.algorithm == IrisAlgorithm::NP) {

This is going to become a massive and messy file as we add more algorithms. It would be good to have a mechanism for separating the different implementations across files.

Code quote:

  if (options.algorithm == IrisAlgorithm::Convex) {
    throw std::runtime_error("IrisAlgorithm::Convex is not supported yet.");
  } else if (options.algorithm == IrisAlgorithm::NP2) {
    throw std::runtime_error("IrisAlgorithm::NP2 is not supported yet.");
  } else if (options.algorithm == IrisAlgorithm::ZO) {
    throw std::runtime_error("IrisAlgorithm::ZO is not supported yet.");
  } else if (options.algorithm == IrisAlgorithm::Certified) {
    throw std::runtime_error("IrisAlgorithm::Certified is not supported yet.");
  } else if (options.algorithm == IrisAlgorithm::NP) {

@AlexandreAmice
Copy link
Contributor

One more complicating factor is what should the return type be?

Currently, IrisInConfigurationSpaceFromCliqueCover crashes if IrisInConfigurationSpace fails to return a region. The PR #21376 is stalled since we are not allowed to catch the errors from IrisInConfigurationSpace.

I am not sure what the best procedure for this is.

@AlexandreAmice
Copy link
Contributor

See #21879 for an alternative design based on classes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: feature This pull request contains a new feature status: do not review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor IRIS variants (making a single IRIS entry point)
3 participants