Convex hulls of multiple convex sets #21594

sadraddini · 2024-06-20T02:50:13Z

A new derived class of ConvexSet: the implementation for convex hulls of multiple convex sets.

This change is

ggould-tri

This PR needs a feature reviewer; +@RussTedrake do you want to take review on this one? Or reassign it elsewhere?

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

AlexandreAmice

-@RussTedrake +@AlexandreAmice

Reviewable status: 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 @sadraddini)

AlexandreAmice

Checkpoint. I'll do the rest of the tests later.

PTAL

Reviewable status: 31 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 @sadraddini)

geometry/optimization/convex_hull.h line 5 at r2 (raw file):

#include <memory>
#include <optional>
#include <set>

nit: I don't think you need to include set?

Code quote:

#include <set>

geometry/optimization/convex_hull.h line 24 at r2 (raw file):

  DRAKE_DEFAULT_COPY_AND_MOVE_AND_ASSIGN(ConvexHull)

  /** Constructs the convex hull from a list of convex sets. */

nit:

Suggestion:

vector

geometry/optimization/convex_hull.h line 35 at r2 (raw file):

  const ConvexSet& element(int index) const;

  /** Returns the number of convex sets constructing the convex hull. */

nit: grammar

Suggestion:

/** Returns the number of convex sets defining the convex hull. */

geometry/optimization/convex_hull.cc line 39 at r2 (raw file):

}

std::vector<symbolic::Variable> AddNewVariables(

This needs documentation.

geometry/optimization/convex_hull.cc line 70 at r2 (raw file):

const ConvexSet& ConvexHull::element(int index) const {
  DRAKE_THROW_UNLESS(0 <= index && index < std::ssize(sets_));
  return *sets_[index];

Why not just use sets_.at(index) which does bounds checking?

Code quote:

  DRAKE_THROW_UNLESS(0 <= index && index < std::ssize(sets_));
  return *sets_[index];

geometry/optimization/convex_hull.cc line 85 at r2 (raw file):

    }
  }
  return true;

I don't think this is the best way to implement this. In the worst case, this solves 2*ambient_dimension()*sets.size() programs rather than 2*ambient_dimension(). So this is definitely not a shortcut.

Ideally, what this method should do is the below (which you can't do to the protected/private nature of DoIsBoundedShortcut). If we can't figure out a way to make use of DoIsBoundedShortcutthen this should just return nullopt.

Suggestion:

  for (const auto& set : sets_) {
    std::optional<bool> bounded = set->DoIsBoundedShortcut()
    if(!bounded.has_value()) {
      return std::nullopt;
    }
    else if (!bounded.value()) {
      return false;
    }
  }
  return true;

geometry/optimization/convex_hull.cc line 97 at r2 (raw file):

      return true;
    }
  }

Again, this solves more programs than necessary if the sets themselves require solving convex programs to check emptiness (e.g. polytopes).

Code quote:

  // The convex hull is empty if one of the participating sets is empty.
  for (const auto& set : sets_) {
    if (set->IsEmpty()) {
      return true;
    }
  }

geometry/optimization/convex_hull.cc line 113 at r2 (raw file):

  }
  return std::nullopt;
}

The ConvexHull of two Point sets is a line. This is contradiction to the promise

If this set trivially contains exactly one point, returns the value of
  that point.

and so in this case this function should return nullopt

Suggestion:

std::optional<VectorXd> ConvexHull::DoMaybeGetPoint() const {
  if(ssize(sets_) == 1) {
    return sets_->MaybeGetPoint();
  }
  return std::nullopt;
}

geometry/optimization/convex_hull.cc line 118 at r2 (raw file):

                              double tol) const {
  // Check the feasibility of |x - ∑ᵢ xᵢ| <= tol*1, xᵢ ∈ 𝛼ᵢXᵢ, 𝛼ᵢ ≥ 0, ∑ᵢ 𝛼ᵢ =
  // 1, where 1 is a vector of ones and |.| is interpreted element-wise.

Any particular reason for measuring the tolerance in the infinity norm?

Suggestion:

  // Check the feasibility of |x - ∑ᵢ xᵢ| <= tol, xᵢ ∈ 𝛼ᵢXᵢ, 𝛼ᵢ ≥ 0, ∑ᵢ 𝛼ᵢ =
  // 1, where 1 is a vector of ones and |.| is interpreted element-wise.

geometry/optimization/convex_hull.cc line 123 at r2 (raw file):

  const int d = ambient_dimension();
  auto xz = prog.NewContinuousVariables(n + 1, d, "x");
  auto alpha = prog.NewContinuousVariables(n, "alpha");

nit:
I would honestly prefer if z was declared separately if you are going to keep it around, which I don't think you should (see below).

Suggestion:

  // The iᵗʰ row of xz corresponds to a point in the iᵗʰ set. 
  // The last row is a slack variable for the jᵗʰ dimension
  const auto xz = prog.NewContinuousVariables(n + 1, d, "x");
  const auto alpha = prog.NewContinuousVariables(n, "alpha");

geometry/optimization/convex_hull.cc line 124 at r2 (raw file):

  auto xz = prog.NewContinuousVariables(n + 1, d, "x");
  auto alpha = prog.NewContinuousVariables(n, "alpha");
  // constraint I: x - ∑ᵢ xᵢ <= z -->  ∑ᵢ xᵢ + z >= x

Why do you need z?

You can just write
x - ∑ᵢ xᵢ ≤ tol ⟹ ∑ᵢ xᵢ ≥ x - tol

similarly for the other side of the inequality.

Also please fix the unicode.

Code quote:

 // constraint I: x - ∑ᵢ xᵢ <= z -->  ∑ᵢ xᵢ + z >= x

geometry/optimization/convex_hull.cc line 129 at r2 (raw file):

  Eigen::VectorXd a_2{a_1};
  a_2(n) = -1;
  // constraint I: inf >= A_1 * xz >= x

I think this line is spurious. Regardless I think you can write this as one constraint with just the x-tol bound.

Code quote:

  // constraint I: inf >= A_1 * xz >= x

geometry/optimization/convex_hull.cc line 138 at r2 (raw file):

  prog.AddLinearEqualityConstraint(Eigen::MatrixXd::Ones(1, n),
                                   VectorXd::Ones(1), alpha);
  // constraint: 1 > αᵢ >= 0

nit

Suggestion:

1 ≥ αᵢ ≥ 0

geometry/optimization/convex_hull.cc line 161 at r2 (raw file):

  const int n = std::ssize(sets_);
  const int d = ambient_dimension();
  // The new variable is n * d, each row is a variable for a convex set

FWIW: we usually declare things in column-major.

Code quote:

// The new variable is n * d, each row is a variable for a convex set

geometry/optimization/convex_hull.cc line 165 at r2 (raw file):

  auto alpha = prog->NewContinuousVariables(n, "alpha");
  prog->AddBoundingBoxConstraint(0, 1, alpha);
  std::vector<solvers::Binding<solvers::Constraint>> new_bindings;

This binding needs to be added to new_bindings

Suggestion:

   std::vector<solvers::Binding<solvers::Constraint>> new_bindings;
   new_bindings.push_back(prog->AddBoundingBoxConstraint(0, 1, alpha));

geometry/optimization/convex_hull.cc line 166 at r2 (raw file):

  prog->AddBoundingBoxConstraint(0, 1, alpha);
  std::vector<solvers::Binding<solvers::Constraint>> new_bindings;
  // constraint: x - ∑ᵢ xᵢ == 0

Suggestion:

vars

geometry/optimization/convex_hull.cc line 173 at r2 (raw file):

    Eigen::Ref<const solvers::VectorXDecisionVariable> vars_i =
        vars.segment(i, 1);
    solvers::VariableRefList vars_list{x.col(i), vars_i};

Does this not work? It reads better.

Suggestion:

    solvers::VariableRefList vars_list{x.col(i), vars(i)};

geometry/optimization/convex_hull.cc line 182 at r2 (raw file):

  auto new_vars = std::vector<symbolic::Variable>(alpha.data(),
                                                  alpha.data() + alpha.size());
  new_vars.insert(new_vars.end(), x.data(), x.data() + x.size());

Could you move this up to where the variables are actually first declared. It is quite scattered right now.

Code quote:

  // alpha and x should already be added
  auto new_vars = std::vector<symbolic::Variable>(alpha.data(),
                                                  alpha.data() + alpha.size());
  new_vars.insert(new_vars.end(), x.data(), x.data() + x.size());

geometry/optimization/convex_hull.cc line 188 at r2 (raw file):

        prog, x.row(i), alpha(i));
    new_bindings.insert(new_bindings.end(), cons.begin(), cons.end());
    AddNewVariables(cons, &new_vars);

Based on reading AddNewVars. I think you should make new_vars a Variables object and then convert it to a vector. Right now, you iterate over the vector far too many times. Using a set would be much more efficient.

Code quote:

AddNewVariables(cons, &new_vars);

geometry/optimization/convex_hull.cc line 198 at r2 (raw file):

ConvexHull::DoAddPointInNonnegativeScalingConstraints(
    solvers::MathematicalProgram* prog,
    const Eigen::Ref<const solvers::VectorXDecisionVariable>& vars,

nit: This is called x in ConvexSet

Suggestion:

const Eigen::Ref<const solvers::VectorXDecisionVariable>& x

geometry/optimization/convex_hull.cc line 217 at r2 (raw file):

    solvers::VariableRefList vars_list{x.col(i), vars_i};
    new_bindings.push_back(prog->AddLinearEqualityConstraint(a, 0, vars_list));
  }

This is the third repetition of (very) minor variants of this idiom. I think you could likely refactor this into an anonymous function.

Code quote:

  const int n = std::ssize(sets_);
  const int d = ambient_dimension();
  // The new variable is n * d, each row is a variable for a convex set
  auto x = prog->NewContinuousVariables(n, d, "x");
  auto alpha = prog->NewContinuousVariables(n, "alpha");
  const double inf = std::numeric_limits<double>::infinity();
  std::vector<solvers::Binding<solvers::Constraint>> new_bindings;
  // Constraint I: -x + ∑ᵢ xᵢ == 0
  Eigen::VectorXd a(n + 1);
  a.head(n) = Eigen::VectorXd::Ones(n);
  a(n) = -1;
  for (int i = 0; i < d; ++i) {
    Eigen::Ref<const solvers::VectorXDecisionVariable> vars_i =
        vars.segment(i, 1);
    solvers::VariableRefList vars_list{x.col(i), vars_i};
    new_bindings.push_back(prog->AddLinearEqualityConstraint(a, 0, vars_list));
  }

geometry/optimization/convex_hull.cc line 249 at r2 (raw file):

  auto s = prog->NewContinuousVariables(n, dim, "s");
  auto alpha = prog->NewContinuousVariables(n, "alpha");
  const double inf = std::numeric_limits<double>::infinity();

nit: We usually declare const double kInf = std::numeric_limits<double>::infinty(); in an anonymous namespace to avoid having to repeat this line.

Code quote:

const double inf = std::numeric_limits<double>::infinity();

geometry/optimization/convex_hull.cc line 252 at r2 (raw file):

  std::vector<solvers::Binding<solvers::Constraint>> new_bindings;
  // constraint: A_x * x - ∑ᵢ sᵢ == -b_x
  DRAKE_DEMAND(A_x.rows() == dim);

No need for this. The preconditions are checked in the NVI

Code quote:

DRAKE_DEMAND(A_x.rows() == dim);

geometry/optimization/convex_hull.cc line 254 at r2 (raw file):

  DRAKE_DEMAND(A_x.rows() == dim);
  for (int i = 0; i < dim; ++i) {
    Eigen::Ref<const solvers::VectorXDecisionVariable> s_i = s.col(i);

If you are trying to avoid a copy here, you need to make this. Eigen::Ref is not actually a reference, but a template class which avoids a copy when using Matrix types as templates. Ditto throughout.

On the other hand

Suggestion:

const solvers::VectorXDecisionVariable& s_i = s.col(i);

geometry/optimization/convex_hull.cc line 277 at r2 (raw file):

  // c't + d and alpha should be positive
  new_bindings.push_back(prog->AddBoundingBoxConstraint(0, inf, alpha));
  new_bindings.push_back(prog->AddLinearConstraint(c.transpose(), -d, inf, t));

Note that subclasses do not need to add the constraint c * t + d ≥ 0
as it is already added.

geometry/optimization/test/convex_hull_test.cc line 23 at r2 (raw file):

  ConvexHull hull(MakeConvexSets(point, rectangle));
  EXPECT_EQ(hull.sets().size(), 2);
  // It is not empty

nit: your comments need periods.

Code quote:

  // It is not empty

geometry/optimization/test/convex_hull_test.cc line 28 at r2 (raw file):

  EXPECT_TRUE(hull.IsBounded());
  // A point in the convex hull exists
  EXPECT_TRUE(hull.MaybeGetPoint().has_value());

This function should not be returning a value.

Code quote:

  // A point in the convex hull exists
  EXPECT_TRUE(hull.MaybeGetPoint().has_value());

geometry/optimization/test/convex_hull_test.cc line 29 at r2 (raw file):

  // A point in the convex hull exists
  EXPECT_TRUE(hull.MaybeGetPoint().has_value());
  // Make a HPolyhedron that is empty

nit

Suggestion:

  // Make an HPolyhedron that is empty

geometry/optimization/test/convex_hull_test.cc line 46 at r2 (raw file):

  EXPECT_THROW(ConvexHull(MakeConvexSets(point, point_3d)), std::runtime_error);
}

Point in set needs one example where sets_ > ambient dimension and one with sets_ < ambient_dimension. This ensures that your allocation of the variables doesn't go out of bounds.

geometry/optimization/test/convex_hull_test.cc line 80 at r2 (raw file):

  // solve a mathematical program that finds a point in the convex hull with
  // least L2 distance to (0.8,0) The result should be (0.4, 0.4), easy to see
  // from the geometry

Suggestion:

  // Solves a mathematical program that finds a point in the convex hull with
  // least L2 distance to (0.8,0) The result should be (0.4, 0.4).

geometry/optimization/test/convex_hull_test.cc line 93 at r2 (raw file):

GTEST_TEST(ConvexHullTest, AddPointInSetConstraints3) {
  // Make convex hull from a point and another convex hull. Calls
  // AddPointInNonnegativeScalingConstraints for the second convex hull.

Suggestion:

  // Makes convex hull from a point and another convex hull. Calls
  // AddPointInNonnegativeScalingConstraints for the second convex hull.

sadraddini

Thanks for the comprehensive review!

Reviewable status: 24 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)

geometry/optimization/convex_hull.h line 5 at r2 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

nit: I don't think you need to include set?

Done

geometry/optimization/convex_hull.h line 24 at r2 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

nit:

Done

geometry/optimization/convex_hull.h line 35 at r2 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

nit: grammar

Done

geometry/optimization/convex_hull.cc line 39 at r2 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

This needs documentation.

Done it is a not in the public API though

geometry/optimization/convex_hull.cc line 70 at r2 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

Why not just use sets_.at(index) which does bounds checking?

Because it throws a message that is more understandable. The same pattern happens in term in MinkowskiSum

geometry/optimization/convex_hull.cc line 85 at r2 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

I don't think this is the best way to implement this. In the worst case, this solves 2*ambient_dimension()*sets.size() programs rather than 2*ambient_dimension(). So this is definitely not a shortcut.

Ideally, what this method should do is the below (which you can't do to the protected/private nature of DoIsBoundedShortcut). If we can't figure out a way to make use of DoIsBoundedShortcutthen this should just return nullopt.

Done. Great point!

geometry/optimization/convex_hull.cc line 97 at r2 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

Again, this solves more programs than necessary if the sets themselves require solving convex programs to check emptiness (e.g. polytopes).

Why? What is the alternative? It is not meant to be a shortcut.

geometry/optimization/convex_hull.cc line 113 at r2 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

The ConvexHull of two Point sets is a line. This is contradiction to the promise
If this set trivially contains exactly one point, returns the value of
  that point.
and so in this case this function should return nullopt

Done I see. I thought one point is just a trivial point, not that the set contains one single point. Good catch.

geometry/optimization/convex_hull.cc line 118 at r2 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

Any particular reason for measuring the tolerance in the infinity norm?

1- The same is in VPolytope from is_approx_equal_abstol 2- It "felt" simpler. L2 norm leads to feasibility checking of a SOCP or checking the cost of a QP. Note: the tol in the documentation is not properly defined as what exactly it means.

geometry/optimization/convex_hull.cc line 123 at r2 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

nit:
I would honestly prefer if z was declared separately if you are going to keep it around, which I don't think you should (see below).

See below

geometry/optimization/convex_hull.cc line 124 at r2 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

Why do you need z?

You can just write
x - ∑ᵢ xᵢ ≤ tol ⟹ ∑ᵢ xᵢ ≥ x - tol

similarly for the other side of the inequality.

Also please fix the unicode.

Done You are right. Changed the formulation to the simpler form.

geometry/optimization/convex_hull.cc line 129 at r2 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

I think this line is spurious. Regardless I think you can write this as one constraint with just the x-tol bound.

Done

geometry/optimization/convex_hull.cc line 138 at r2 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

nit

Done

geometry/optimization/convex_hull.cc line 161 at r2 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

FWIW: we usually declare things in column-major.

Done

geometry/optimization/convex_hull.cc line 165 at r2 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

This binding needs to be added to new_bindings

Done

geometry/optimization/convex_hull.cc line 166 at r2 (raw file):

  prog->AddBoundingBoxConstraint(0, 1, alpha);
  std::vector<solvers::Binding<solvers::Constraint>> new_bindings;
  // constraint: x - ∑ᵢ xᵢ == 0

Done

geometry/optimization/convex_hull.cc line 173 at r2 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

Does this not work? It reads better.

Done vars(i) does not compile (it has to be a vector). Used segment to preserve the type.

geometry/optimization/convex_hull.cc line 182 at r2 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

Could you move this up to where the variables are actually first declared. It is quite scattered right now.

Done

geometry/optimization/convex_hull.cc line 198 at r2 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

nit: This is called x in ConvexSet

Done

geometry/optimization/convex_hull.cc line 217 at r2 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

This is the third repetition of (very) minor variants of this idiom. I think you could likely refactor this into an anonymous function.

Given the variants, I could make a function that led to saving very few lines of code. It decreases the readability IMO. So no action taken.

geometry/optimization/convex_hull.cc line 249 at r2 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

nit: We usually declare const double kInf = std::numeric_limits<double>::infinty(); in an anonymous namespace to avoid having to repeat this line.

It is repeated twice now - not yet at 3-times rule.

geometry/optimization/convex_hull.cc line 252 at r2 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

No need for this. The preconditions are checked in the NVI

Done removed

geometry/optimization/convex_hull.cc line 254 at r2 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

If you are trying to avoid a copy here, you need to make this. Eigen::Ref is not actually a reference, but a template class which avoids a copy when using Matrix types as templates. Ditto throughout.

On the other hand

Done

geometry/optimization/convex_hull.cc line 277 at r2 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

Note that subclasses do not need to add the constraint c * t + d ≥ 0
as it is already added.

Done

geometry/optimization/test/convex_hull_test.cc line 23 at r2 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

nit: your comments need periods.

Done

geometry/optimization/test/convex_hull_test.cc line 28 at r2 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

This function should not be returning a value.

Done

geometry/optimization/test/convex_hull_test.cc line 29 at r2 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

nit

Done

geometry/optimization/test/convex_hull_test.cc line 46 at r2 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

Point in set needs one example where sets_ > ambient dimension and one with sets_ < ambient_dimension. This ensures that your allocation of the variables doesn't go out of bounds.

Done. Added two tests.

geometry/optimization/test/convex_hull_test.cc line 80 at r2 (raw file):

  // solve a mathematical program that finds a point in the convex hull with
  // least L2 distance to (0.8,0) The result should be (0.4, 0.4), easy to see
  // from the geometry

Done

geometry/optimization/test/convex_hull_test.cc line 93 at r2 (raw file):

GTEST_TEST(ConvexHullTest, AddPointInSetConstraints3) {
  // Make convex hull from a point and another convex hull. Calls
  // AddPointInNonnegativeScalingConstraints for the second convex hull.

Done

sadraddini

I yet to have the requested change about AddNewVariables.

Reviewable status: 24 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)

AlexandreAmice

Reviewed 1 of 4 files at r1, 3 of 3 files at r3, all commit messages.
Reviewable status: 4 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 @sadraddini)

geometry/optimization/convex_hull.cc line 39 at r2 (raw file):

Previously, sadraddini (Sadra Sadraddini) wrote…

Done it is a not in the public API though

I still think this should be a Variables object.

geometry/optimization/convex_hull.cc line 97 at r2 (raw file):

Previously, sadraddini (Sadra Sadraddini) wrote…

Why? What is the alternative? It is not meant to be a shortcut.

// The convex hull is empty if one of the participating sets is empty. Actually this isn't true. The convex hull is empty is all the sets are empty. If one set is non-empty, then the empty set does not contain it and therefore the convex hull cannot be the empty set since a convex hull must contain all the sets.

DoIsEmpty is implemented in ConvexSet and requires solving a single convex feasibility program. This currently solves n (admittedly smaller) convex programs. The current implementation is only faster if DoIsEmpty() can be checked without solving a convex program.

I guess this is true Point, Hyperrectangle, Hyperellispoid, VPolytope, AffineSubspace, but not HPolyhedron and Spectrahedron.

I personally think that either this method should use the DoIsEmpty from convex set, or should be smart about whether or not it should solve a big program.

geometry/optimization/convex_hull.cc line 118 at r2 (raw file):

Previously, sadraddini (Sadra Sadraddini) wrote…

1- The same is in VPolytope from is_approx_equal_abstol 2- It "felt" simpler. L2 norm leads to feasibility checking of a SOCP or checking the cost of a QP. Note: the tol in the documentation is not properly defined as what exactly it means.

nit Could you remove the "1"?

sadraddini

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

geometry/optimization/convex_hull.cc line 97 at r2 (raw file):
I see your point about a big program vs multiple small programs. I will go with a big program to be generic.

Actually this isn't true. The convex hull is empty is all the sets are empty

I believe if one participating set is empty, then the convex hull becomes undefined {∑ᵢ λᵢ xᵢ | xᵢ ∈ Xᵢ, λᵢ ≥ 0, ∑ᵢ λᵢ = 1}. There basically would not exist any xᵢ ∈ Xᵢ.

For the sake of this implementation in this class, one set being empty leads to infeasibility in all operations because xᵢ ∈ λᵢ Xᵢ becomes infeasible. Another approach is to find empty sets and remove them from the participating sets to make the convex hull and the operations in this class well-defined.

We have the same issue in Minkowski sums and there we declare the set empty if one participating set is empty.

I favor declaring convex hull empty and explain this in the documentation.

What do you think?

geometry/optimization/convex_hull.cc line 118 at r2 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

nit Could you remove the "1"?

I've put it to emphasize the vector form. Is this against our style?

AlexandreAmice

Reviewable status: 4 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 @sadraddini)

geometry/optimization/convex_hull.cc line 97 at r2 (raw file):

Previously, sadraddini (Sadra Sadraddini) wrote…

I see your point about a big program vs multiple small programs. I will go with a big program to be generic.

Actually this isn't true. The convex hull is empty is all the sets are empty

I believe if one participating set is empty, then the convex hull becomes undefined {∑ᵢ λᵢ xᵢ | xᵢ ∈ Xᵢ, λᵢ ≥ 0, ∑ᵢ λᵢ = 1}. There basically would not exist any xᵢ ∈ Xᵢ.

For the sake of this implementation in this class, one set being empty leads to infeasibility in all operations because xᵢ ∈ λᵢ Xᵢ becomes infeasible. Another approach is to find empty sets and remove them from the participating sets to make the convex hull and the operations in this class well-defined.

We have the same issue in Minkowski sums and there we declare the set empty if one participating set is empty.

I favor declaring convex hull empty and explain this in the documentation.

What do you think?

If one defines the convex hull as the smallest convex set which contains the union of all the component sets, then there is no problem with defining the convex hull between a set and the empty set.

This is different from the case of the Minkowski sum since there the addition is undefined as there is no vector.

Perhaps we should have the invariant in this class that all sets be non-empty and we enforce this at construction time. This would avoid any subtletites.

geometry/optimization/convex_hull.cc line 118 at r2 (raw file):

Previously, sadraddini (Sadra Sadraddini) wrote…

I've put it to emphasize the vector form. Is this against our style?

The left hand side of |x - ∑ᵢ xᵢ| <= tol*1 is a scalar no?

sadraddini

Reviewable status: 4 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 @sadraddini)

geometry/optimization/convex_hull.cc line 39 at r2 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

I still think this should be a Variables object.

Done see below

geometry/optimization/convex_hull.cc line 97 at r2 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

If one defines the convex hull as the smallest convex set which contains the union of all the component sets, then there is no problem with defining the convex hull between a set and the empty set.

This is different from the case of the Minkowski sum since there the addition is undefined as there is no vector.

Perhaps we should have the invariant in this class that all sets be non-empty and we enforce this at construction time. This would avoid any subtletites.

Checking the emptiness at construction is costly. I prefer the ctor to be fast. I added the following:

Code snippet:

@note If any of the participating sets, Xᵢ = ∅, is empty, then the convex hull
is also considered empty. This interpretation arises because mathematical
problems involving constraints such as xᵢ ∈ Xᵢ or variations thereof become
infeasible. It is advisable to verify the emptiness of the participating sets
before attempting to construct the convex hull.
@ingroup geometry_optimization */

geometry/optimization/convex_hull.cc line 118 at r2 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

The left hand side of |x - ∑ᵢ xᵢ| <= tol*1 is a scalar no?

No. It's a vector. The |.| operation (absolute value) is interpreted element-wise. Look at the next sentence.

geometry/optimization/convex_hull.cc line 188 at r2 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

Based on reading AddNewVars. I think you should make new_vars a Variables object and then convert it to a vector. Right now, you iterate over the vector far too many times. Using a set would be much more efficient.

Done although this is different than similar method in other convex sets. However, convex hull introduces more new variables and in more complicated way.

AlexandreAmice

Reviewed 1 of 4 files at r4.
Reviewable status: 5 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 @sadraddini)

geometry/optimization/convex_hull.cc line 97 at r2 (raw file):

Previously, sadraddini (Sadra Sadraddini) wrote…

Checking the emptiness at construction is costly. I prefer the ctor to be fast. I added the following:

I think we might want a second opinion here. If any of the sets in convex hull are empty, then the output of almost every function here is wrong. Therefore, I think we need to enforce that none of the sets are empty.

I think there are two ways to address this.

Add an additional argument to the constructor sets_known_non_empty = False where if this is set to true, then we don't check whether the sets are empty
Add a member field std::vector<bool> set_known_non_empty where the value of set_known_non_empty.at(i) is True if we know set.at(i) is non-empty. Then methods just need to check whether all the values of set_known_non_empty are True before proceeding.

geometry/optimization/convex_hull.cc line 114 at r4 (raw file):

                                                      alpha(i));
  }
  // Check the feasibility.

nit:

Suggestion:

// Check feasibility.

geometry/optimization/convex_hull.cc line 147 at r4 (raw file):

  new_bindings.push_back(prog->AddLinearEqualityConstraint(
      Eigen::MatrixXd::Ones(1, n), VectorXd::Ones(1), alpha));
  auto new_vars = drake::symbolic::Variables();

You are now missing alpha in these variables.

Code quote:

auto new_vars = drake::symbolic::Variables();

geometry/optimization/convex_hull.cc line 241 at r4 (raw file):

  new_bindings.push_back(
      prog->AddLinearEqualityConstraint(a_con, d, alpha_t_vec));
  // c't + d and alpha should be positive.

nit: I think you can remove this.

Code quote:

// c't + d and alpha should be positive.

geometry/optimization/test/convex_hull_test.cc line 96 at r4 (raw file):

  // How many new variables are added?
  // 2 alphas, 2*2 x variables. Total 6.
  EXPECT_EQ(new_vars.size(), 6);

Not sure how this is passing if I am right that your current code forgets to add alpha to the new variables? Maybe I misunderstood?

Code quote:

  // How many new variables are added?
  // 2 alphas, 2*2 x variables. Total 6.
  EXPECT_EQ(new_vars.size(), 6);

sadraddini

Need a resolution on empty convex hulls.

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 @sadraddini)

geometry/optimization/convex_hull.cc line 97 at r2 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

I think we might want a second opinion here. If any of the sets in convex hull are empty, then the output of almost every function here is wrong. Therefore, I think we need to enforce that none of the sets are empty.

I think there are two ways to address this.

Add an additional argument to the constructor sets_known_non_empty = False where if this is set to true, then we don't check whether the sets are empty

Add a member field std::vector<bool> set_known_non_empty where the value of set_known_non_empty.at(i) is True if we know set.at(i) is non-empty. Then methods just need to check whether all the values of set_known_non_empty are True before proceeding.

I did an implementation of (1) and did not like it. I made the constructor throw an error, but that does not feel right. It is much better to ask the user to check IsEmpty. (2) is more useful if the constructor removes the empty sets. But that also feels that must happen outside of the constructor. And what if one of the flags in the vector is wrong?

The "right" solution is checking the emptiness of all the participating sets, and removing those that are empty. We may have a participating convex set that itself is a convex hull and this ensures empty sets never get propagated.

However, code-wise, I believe it is better to follow other methods in Drake. For example, empty sets are allowed to participate in the Minkowski-sum, but the sums become undefined and will make everything infeasible.

At the end, we have defined the convex hull as {∑ᵢ λᵢ xᵢ | xᵢ ∈ Xᵢ, λᵢ ≥ 0, ∑ᵢ λᵢ = 1}. and not the classic definition of the smallest convex set that contains all the sets. The former only follows from the latter if all sets are non-empty convex sets.

I am happy to see what other people think cc @RussTedrake @TobiaMarcucci @wrangelvid

geometry/optimization/convex_hull.cc line 114 at r4 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

nit:

Done

geometry/optimization/convex_hull.cc line 147 at r4 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

You are now missing alpha in these variables.

No worries as alpha and x_sets get added in the loop form the bindings.

geometry/optimization/convex_hull.cc line 241 at r4 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

nit: I think you can remove this.

Done you are right, they are added in convex_set.cc

geometry/optimization/test/convex_hull_test.cc line 96 at r4 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

Not sure how this is passing if I am right that your current code forgets to add alpha to the new variables? Maybe I misunderstood?

See the comment above. They get added in AddNewVariables.

BetsyMcPhail · 2024-07-12T18:31:04Z

@drake-jenkins-bot linux-jammy-clang-bazel-experimental-leak-sanitizer please

sadraddini · 2024-07-22T17:22:11Z

@AlexandreAmice any new thoughts?

AlexandreAmice

I have no new thoughts. I really don't like the idea of allowing users to get completely non-sense answers if they do something as benign as taking the convex hull with the empty set. This is quite easy to do, so there needs to be some sort of mechanism to protect the user in my opinion.

Reviewed 1 of 3 files at r5.
Reviewable status: 1 unresolved discussion, 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 @sadraddini)

sadraddini · 2024-07-31T18:43:49Z

The only nonsense, to use your term, answer is infeasibility, and that is warned in the formulation and documentation. Users can check IsEmpty when in doubt.

I could not find any "mechanism" to prevent this as they are worse options. Either a costly check has to be in the constructor, or the user should pass args that prevent the costly check. The latter case is worse, because if the user wrongly says a set is known to be empty/non-empty, the answers would be more nonsense.

RussTedrake

Reviewable status: 1 unresolved discussion, 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 @sadraddini)

geometry/optimization/convex_hull.cc line 97 at r2 (raw file):

Previously, sadraddini (Sadra Sadraddini) wrote…

I did an implementation of (1) and did not like it. I made the constructor throw an error, but that does not feel right. It is much better to ask the user to check IsEmpty. (2) is more useful if the constructor removes the empty sets. But that also feels that must happen outside of the constructor. And what if one of the flags in the vector is wrong?

The "right" solution is checking the emptiness of all the participating sets, and removing those that are empty. We may have a participating convex set that itself is a convex hull and this ensures empty sets never get propagated.

However, code-wise, I believe it is better to follow other methods in Drake. For example, empty sets are allowed to participate in the Minkowski-sum, but the sums become undefined and will make everything infeasible.

At the end, we have defined the convex hull as {∑ᵢ λᵢ xᵢ | xᵢ ∈ Xᵢ, λᵢ ≥ 0, ∑ᵢ λᵢ = 1}. and not the classic definition of the smallest convex set that contains all the sets. The former only follows from the latter if all sets are non-empty convex sets.

I am happy to see what other people think cc @RussTedrake @TobiaMarcucci @wrangelvid

I believe everyone was happy with the decision from the stand-up yesterday: empty sets are discarded (or marked as "ignore") using a potentially expensive check in the constructor to check emptiness. And we can add a new optional argument in the constructor which opts out of the expensive checks with "i'm sure my sets are not empty, and understand that the code won't work correctly if i'm wrong"

ggould-tri

Reviewed 1 of 4 files at r4, 3 of 3 files at r5, all commit messages.
Reviewable status: 1 unresolved discussion, 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 @sadraddini)

sadraddini

Reviewable status: 1 unresolved discussion, 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 @sadraddini)

geometry/optimization/convex_hull.cc line 97 at r2 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

I believe everyone was happy with the decision from the stand-up yesterday: empty sets are discarded (or marked as "ignore") using a potentially expensive check in the constructor to check emptiness. And we can add a new optional argument in the constructor which opts out of the expensive checks with "i'm sure my sets are not empty, and understand that the code won't work correctly if i'm wrong"

Done I implemented a version of this and updated the unit tests

btw I realized it would usually not lead not to infeasibility as x ∈ 𝛼X with empty X is feasible with 𝛼 = 0, but the problem is that non-zero x can also become feasible. I have included an example in the documentation.

AlexandreAmice

mostly there just one more small change. Thanks for your patience!

Reviewed 4 of 4 files at r6, all commit messages.
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) (waiting on @sadraddini)

geometry/optimization/convex_hull.h line 39 at r6 (raw file):

    constructed incorrectly with remove_empty_sets=false), the remove_empty_sets
    will *not* remove it but will remove the whole convex hull itself.
    */

I would make this documentation a bit more succinct and bring the warning higher up so that it is not missed. If you think the example is important enough to keep, I would move it to the bottom of the documentation.

BTW: can we avoid this "unintended" behavior by checking the values of alpha after solving the programs to see if the set actually participated?

Suggestion:

  /** Constructs the convex hull from a vector of convex sets.
    @param sets A vector of convex sets that define the convex hull.
    @param remove_empty_sets If true, the constructor will check if any of the
    sets are empty and will not consider them. If false, the constructor will
    not check if any of the sets are empty.
    @warning If remove_empty_sets is set to false, but some of the sets are in fact empty,
    then unexpected and incorrect results may occur. Only set this flag to false if you are
    sure that your sets are non-empty and performance in the constructor is critical.
    */

geometry/optimization/convex_hull.cc line 88 at r6 (raw file):

  // if non_empty_sets_ does not have value, then we reconstruct the
  // non_empty_sets_ and check if it is empty.
  return ConvexHull(sets_, true).IsEmpty();

I think if you're going to implement this in this manner, you should then update non_empty_sets to have value so that subsequent calls to IsEmpty() can be cheap.

Code quote:

  // if non_empty_sets_ does not have value, then we reconstruct the
  // non_empty_sets_ and check if it is empty.
  return ConvexHull(sets_, true).IsEmpty();

geometry/optimization/convex_hull.cc line 179 at r6 (raw file):

    const symbolic::Variable& t) const {
  // Add the constraint that t >= 0, x = ∑ᵢ xᵢ, xᵢ ∈ 𝛼ᵢXᵢ, 𝛼ᵢ ≥ 0, ∑ᵢ 𝛼ᵢ = t.
  const auto& participating_sets = non_empty_sets_.value_or(sets_);

Is it safer to just make this a function? To avoid the repeated calls to value_or?

Code quote:

const auto& participating_sets = non_empty_sets_.value_or(sets_);

geometry/optimization/test/convex_hull_test.cc line 66 at r6 (raw file):

  EXPECT_TRUE(hull_2.PointInSet(Eigen::Vector2d(0.0, 3.0), 1e-6));
  // If only the empty_hpolyhedron is added, the convex hull should be empty.
  ConvexHull hull_3(MakeConvexSets(empty_hpolyhedron));

For platform reviewer. Do we need to check the unintended behavior?

Code quote:

  // Unexpected behavior because the check was bypassed.
  // [0, 3] is not in the convex hull, but it says it is because
  // 0*empty_hpolyhedron contains the line.
  EXPECT_TRUE(hull_2.PointInSet(Eigen::Vector2d(0.0, 3.0), 1e-6));
  // If only the empty_hpolyhedron is added, the convex hull should be empty.
  ConvexHull hull_3(MakeConvexSets(empty_hpolyhedron));

sadraddini

Thanks! Made a small change and replaced the optional ConvexSets with a sets and a boolean flag instead.

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) (waiting on @sadraddini)

geometry/optimization/convex_hull.h line 39 at r6 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

I would make this documentation a bit more succinct and bring the warning higher up so that it is not missed. If you think the example is important enough to keep, I would move it to the bottom of the documentation.

BTW: can we avoid this "unintended" behavior by checking the values of alpha after solving the programs to see if the set actually participated?

Done Thank you for the better does

for empty sets, I believe always alpha = 0, unless the user manually imposes some constraints on it.

A good thing about removing empty sets is performance as it decreases the number of optimization vars.

geometry/optimization/convex_hull.cc line 88 at r6 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

I think if you're going to implement this in this manner, you should then update non_empty_sets to have value so that subsequent calls to IsEmpty() can be cheap.

It is a const method. Can't update. Made a note in the documentation

geometry/optimization/convex_hull.cc line 179 at r6 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

Is it safer to just make this a function? To avoid the repeated calls to value_or?

Made it a private variable and changed the other one.

geometry/optimization/test/convex_hull_test.cc line 66 at r6 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

For platform reviewer. Do we need to check the unintended behavior?

@ggould-tri

sadraddini · 2024-08-12T18:50:23Z

+@ggould-tri for platform review per the original plan. Please feel free to reassign.

ggould-tri

; trivial notes and obnoxious quibbles below. Glad to see this finally coming over the finish line.

Reviewed 1 of 4 files at r6, 3 of 3 files at r7, all commit messages.
Reviewable status: 13 unresolved discussions

geometry/optimization/convex_hull.h line 48 at r7 (raw file):

  /** Returns true if the constructor checked for empty sets and removed them,
  if any. */

nit: I found this "if any" phrasing a bit awkward.

Suggestion:

  /** Returns true iff the constructor checked for empty sets and removed at least one. */

geometry/optimization/convex_hull.h line 63 at r7 (raw file):

  remove_empty_sets=true. Therefore, it is recommended to call this function
  only once. */
  using ConvexSet::IsEmpty;

minor: The set-of-sets can be empty, either before or after removal, and that case needs to be documented here because the emptiness of sets with ambient dimension zero is called out in the superclass as a per-class property (I think you're making the opposite decision here as it does?).

Also ConvexHull({ConvexSet(0)}).IsEmpty() == False, but ConvexHull({}).IsEmpty() == True, I think; is this intended?

geometry/optimization/convex_hull.h line 107 at r7 (raw file):

  ConvexSets sets_{};
  ConvexSets participating_sets_{};
  bool empty_sets_removed_;

minor: I believe some or all of these can be const.

Suggestion:

  const ConvexSets sets_{};
  const ConvexSets participating_sets_{};
  const bool empty_sets_removed_;

geometry/optimization/convex_hull.cc line 94 at r7 (raw file):

  }
  // if empty_sets_removed_ is false, then we reconstruct the
  // participating_sets_ and check if it is empty.

typo Capitalize complete sentence.

Suggestion:

  // If empty_sets_removed_ is false, then we reconstruct the
  // participating_sets_ and check if it is empty.

geometry/optimization/convex_hull.cc line 162 at r7 (raw file):

      Eigen::MatrixXd::Ones(1, n), VectorXd::Ones(1), alpha));
  auto new_vars = drake::symbolic::Variables();
  // add the constraints for each convex set

nit: Capitalize and punctuate complete sentence.

Suggestion:

  // Add the constraints for each convex set.

geometry/optimization/test/convex_hull_test.cc line 66 at r6 (raw file):

Previously, sadraddini (Sadra Sadraddini) wrote…

@ggould-tri

The check is not necessary -- you've warned that incorrect behaviour is possible and have not said that it's guaranteed -- but IMO it is valuable as an explanation and example of how things go wrong. I didn't understand how the check could go wrong until I read this code.

geometry/optimization/test/convex_hull_test.cc line 78 at r7 (raw file):

  EXPECT_TRUE(hull.PointInSet(Eigen::Vector2d(0.0, 1.0), 1e-6));
  EXPECT_FALSE(hull.PointInSet(Eigen::Vector2d(1.0, 1.1), 1e-6));
  // test tolerance.

nit: Inconsistent capitalization w.r.t other comments in this file

Suggestion:

  // Test tolerance.

geometry/optimization/test/convex_hull_test.cc line 139 at r7 (raw file):

  ConvexHull hull1(MakeConvexSets(point1, rectangle));
  ConvexHull hull2(MakeConvexSets(hull1, point2));
  // We know that (0.5,0) to (1.0, 1.0) becomes a face

nit: Inconsistent whitespace.

Suggestion:

 (0.5, 0)

geometry/optimization/test/convex_hull_test.cc line 159 at r7 (raw file):

  HPolyhedron empty_hpolyhedron(A, b);
  ConvexHull hull(MakeConvexSets(point, rectangle, empty_hpolyhedron));
  // ConvexHull hull(MakeConvexSets(point, rectangle));

nit: Commented-out code.

Suggestion:

  ConvexHull hull(MakeConvexSets(point, rectangle, empty_hpolyhedron));

geometry/optimization/test/convex_hull_test.cc line 196 at r7 (raw file):

  Eigen::Vector2d b(0.0, 2.0);
  // Select a 3d vector c = [1, 2, -1] and d = 5.0, just to make the problem.
  // more interesting.

typo

Suggestion:

  // Select a 3d vector c = [1, 2, -1] and d = 5.0, just to make the problem
  // more interesting.

sadraddini

Thanks for the review.

Reviewable status: 7 unresolved discussions

geometry/optimization/convex_hull.h line 48 at r7 (raw file):

Previously, ggould-tri wrote…

nit: I found this "if any" phrasing a bit awkward.

This is not what is intended. It returns true even if it did not remove any set. Just only if ctor actually checked, independent of the outcome. Reworded to make it clear.

geometry/optimization/convex_hull.h line 63 at r7 (raw file):

Also ConvexHull({ConvexSet(0)}).IsEmpty() == False, but ConvexHull({}).IsEmpty() == True, I think; is this intended?

Your statement is correct (I just tested it). This is actually following the documentation. Where is the opposite decision? Maybe I am missing something.

geometry/optimization/convex_hull.h line 107 at r7 (raw file):

Previously, ggould-tri wrote…

minor: I believe some or all of these can be const.

It would not compile this way, I think due to move semantics.

geometry/optimization/convex_hull.cc line 94 at r7 (raw file):

Previously, ggould-tri wrote…

typo Capitalize complete sentence.

Done

geometry/optimization/convex_hull.cc line 162 at r7 (raw file):

Previously, ggould-tri wrote…

nit: Capitalize and punctuate complete sentence.

Done

geometry/optimization/test/convex_hull_test.cc line 66 at r6 (raw file):

Previously, ggould-tri wrote…

The check is not necessary -- you've warned that incorrect behaviour is possible and have not said that it's guaranteed -- but IMO it is valuable as an explanation and example of how things go wrong. I didn't understand how the check could go wrong until I read this code.

I am still keeping it unless there is a big objection.

geometry/optimization/test/convex_hull_test.cc line 78 at r7 (raw file):

Previously, ggould-tri wrote…

nit: Inconsistent capitalization w.r.t other comments in this file

Done

geometry/optimization/test/convex_hull_test.cc line 139 at r7 (raw file):

Previously, ggould-tri wrote…

nit: Inconsistent whitespace.

Done

geometry/optimization/test/convex_hull_test.cc line 159 at r7 (raw file):

Previously, ggould-tri wrote…

nit: Commented-out code.

Done

geometry/optimization/test/convex_hull_test.cc line 196 at r7 (raw file):

Previously, ggould-tri wrote…

typo

Done

ggould-tri

Reviewed 1 of 3 files at r8, all commit messages.
Reviewable status: 4 unresolved discussions

geometry/optimization/convex_hull.h line 63 at r7 (raw file):

Previously, sadraddini (Sadra Sadraddini) wrote…

Also ConvexHull({ConvexSet(0)}).IsEmpty() == False, but ConvexHull({}).IsEmpty() == True, I think; is this intended?

Your statement is correct (I just tested it). This is actually following the documentation. Where is the opposite decision? Maybe I am missing something.

As long as it's intended, I don't feel strongly about it; the documentation confuses me every time I read it anyway :-/

geometry/optimization/convex_hull.h line 107 at r7 (raw file):

Previously, sadraddini (Sadra Sadraddini) wrote…

It would not compile this way, I think due to move semantics.

Huh. Okay; it's not worth too much concern.

AlexandreAmice

Reviewed 2 of 3 files at r8, all commit messages.
Reviewable status: 1 unresolved discussion

geometry/optimization/convex_hull.cc line 52 at r8 (raw file):

// Given a vector of convex sets, return a vector of non-empty convex sets if
// the remove_empty_sets is true, otherwise return the original vector.

nit: grammar

Suggestion:

// Given a vector of convex sets, return a vector of non-empty convex sets if
// remove_empty_sets is true, otherwise return the original vector.

geometry/optimization/test/convex_hull_test.cc line 66 at r6 (raw file):

Previously, sadraddini (Sadra Sadraddini) wrote…

I am still keeping it unless there is a big objection.

I am happy to keep it.

sadraddini

Reviewable status: complete! all discussions resolved, LGTM from assignees ggould-tri(platform),AlexandreAmice

geometry/optimization/convex_hull.cc line 52 at r8 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

nit: grammar

Done

ggould-tri

Reviewed 1 of 3 files at r8, 1 of 1 files at r9, all commit messages.
Reviewable status: complete! all discussions resolved, LGTM from assignees ggould-tri(platform),AlexandreAmice

* Convex hulls of multiple convex sets * fix typo * remove unnecessary headers * remove std::move * address Alex's comments * address comments * augment test and address comments * empty tests * last form * remove optional and address comments * address Grant's comments * grammar fix

Convex hulls of multiple convex sets

ecdc29a

sadraddini added the status: do not review label Jun 20, 2024

fix typo

ec67c95

sadraddini added release notes: feature This pull request contains a new feature and removed status: do not review labels Jun 20, 2024

sadraddini added 2 commits June 24, 2024 16:31

remove unnecessary headers

5711a22

remove std::move

9e24e29

ggould-tri assigned RussTedrake Jun 25, 2024

ggould-tri reviewed Jun 25, 2024

View reviewed changes

AlexandreAmice assigned AlexandreAmice and unassigned RussTedrake Jun 25, 2024

AlexandreAmice reviewed Jun 25, 2024

View reviewed changes

AlexandreAmice reviewed Jun 26, 2024

View reviewed changes

address Alex's comments

06cee22

sadraddini commented Jun 27, 2024

View reviewed changes

AlexandreAmice reviewed Jun 28, 2024

View reviewed changes

sadraddini commented Jul 1, 2024

View reviewed changes

AlexandreAmice reviewed Jul 3, 2024

View reviewed changes

sadraddini added 2 commits July 9, 2024 18:14

Merge branch 'master' into ss.convex_hull

de65de3

address comments

be310be

sadraddini commented Jul 9, 2024

View reviewed changes

AlexandreAmice reviewed Jul 12, 2024

View reviewed changes

sadraddini added 2 commits July 12, 2024 16:29

Merge branch 'master' into ss.convex_hull

8075673

augment test and address comments

8b378e2

sadraddini commented Jul 12, 2024

View reviewed changes

AlexandreAmice reviewed Jul 31, 2024

View reviewed changes

RussTedrake reviewed Aug 2, 2024

View reviewed changes

ggould-tri reviewed Aug 7, 2024

View reviewed changes

sadraddini added 3 commits August 8, 2024 15:17

Merge branch 'master' into ss.convex_hull

e1ca9d8

empty tests

9ff4cb9

last form

558307f

sadraddini commented Aug 9, 2024

View reviewed changes

AlexandreAmice reviewed Aug 9, 2024

View reviewed changes

remove optional and address comments

f0475da

sadraddini commented Aug 12, 2024

View reviewed changes

sadraddini assigned ggould-tri Aug 12, 2024

sadraddini added the status: squashing now https://drake.mit.edu/reviewable.html#curated-commits label Aug 12, 2024

ggould-tri reviewed Aug 12, 2024

View reviewed changes

sadraddini added 2 commits August 13, 2024 17:09

Merge branch 'master' into ss.convex_hull

ee10ee0

address Grant's comments

3482fa4

sadraddini commented Aug 13, 2024

View reviewed changes

ggould-tri reviewed Aug 13, 2024

View reviewed changes

AlexandreAmice reviewed Aug 13, 2024

View reviewed changes

grammar fix

23addce

sadraddini commented Aug 13, 2024

View reviewed changes

ggould-tri reviewed Aug 13, 2024

View reviewed changes

sadraddini merged commit 75fc21f into RobotLocomotion:master Aug 13, 2024
9 checks passed

cohnt mentioned this pull request Aug 21, 2024

Python Bindings for ConvexHull #21835

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convex hulls of multiple convex sets #21594

Convex hulls of multiple convex sets #21594

sadraddini commented Jun 20, 2024 •

edited by sherm1

Loading

ggould-tri left a comment

AlexandreAmice left a comment

AlexandreAmice left a comment

sadraddini left a comment

sadraddini left a comment

AlexandreAmice left a comment

sadraddini left a comment

AlexandreAmice left a comment

sadraddini left a comment

AlexandreAmice left a comment

sadraddini left a comment

BetsyMcPhail commented Jul 12, 2024

sadraddini commented Jul 22, 2024

AlexandreAmice left a comment

sadraddini commented Jul 31, 2024

RussTedrake left a comment

ggould-tri left a comment

sadraddini left a comment

AlexandreAmice left a comment

sadraddini left a comment

sadraddini commented Aug 12, 2024 •

edited

Loading

ggould-tri left a comment

sadraddini left a comment

ggould-tri left a comment

AlexandreAmice left a comment

sadraddini left a comment

ggould-tri left a comment

Convex hulls of multiple convex sets #21594

Convex hulls of multiple convex sets #21594

Conversation

sadraddini commented Jun 20, 2024 • edited by sherm1 Loading

ggould-tri left a comment

Choose a reason for hiding this comment

AlexandreAmice left a comment

Choose a reason for hiding this comment

AlexandreAmice left a comment

Choose a reason for hiding this comment

sadraddini left a comment

Choose a reason for hiding this comment

sadraddini left a comment

Choose a reason for hiding this comment

AlexandreAmice left a comment

Choose a reason for hiding this comment

sadraddini left a comment

Choose a reason for hiding this comment

AlexandreAmice left a comment

Choose a reason for hiding this comment

sadraddini left a comment

Choose a reason for hiding this comment

AlexandreAmice left a comment

Choose a reason for hiding this comment

sadraddini left a comment

Choose a reason for hiding this comment

BetsyMcPhail commented Jul 12, 2024

sadraddini commented Jul 22, 2024

AlexandreAmice left a comment

Choose a reason for hiding this comment

sadraddini commented Jul 31, 2024

RussTedrake left a comment

Choose a reason for hiding this comment

ggould-tri left a comment

Choose a reason for hiding this comment

sadraddini left a comment

Choose a reason for hiding this comment

AlexandreAmice left a comment

Choose a reason for hiding this comment

sadraddini left a comment

Choose a reason for hiding this comment

sadraddini commented Aug 12, 2024 • edited Loading

ggould-tri left a comment

Choose a reason for hiding this comment

sadraddini left a comment

Choose a reason for hiding this comment

ggould-tri left a comment

Choose a reason for hiding this comment

AlexandreAmice left a comment

Choose a reason for hiding this comment

sadraddini left a comment

Choose a reason for hiding this comment

ggould-tri left a comment

Choose a reason for hiding this comment

sadraddini commented Jun 20, 2024 •

edited by sherm1

Loading

sadraddini commented Aug 12, 2024 •

edited

Loading