Add Expression Costs and Formula Constraints to Subgraphs in GcsTrajectoryOptimization

[cohnt]

On the way towards completing #21981. Adds placeholder variables to subgraphs, that can be used to construct costs and constraints to each vertex or edge. Direct followups to this PR include variants that take in Binding<Constraint>, adding Python bindings to everything, and the analogous copies for EdgesBetweenSubgraphs.

+@RussTedrake for feature review on this one, please! Also cc @wrangelvid -- let me know if you have any comments or suggestions.

Note that this includes the fix #22090, since the conjunctive formula used in a test case creates an instance of this bug.

---

This change is 

[RussTedrake]

Thanks @cohnt ! Added some API suggestions/requests. PTAL.

Reviewed 2 of 5 files at r1, all commit messages.
Reviewable status: 6 unresolved discussions, 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 @cohnt)

---

planning/trajectory_optimization/gcs_trajectory_optimization.h line 284 at r1 (raw file):

    AddVertexConstraint. Passing this variable directly into
    objectives/constraints will result in an error. */
    const solvers::VectorDecisionVariable<1>& vertex_time_placeholder() const {

let's not use the _placeholder() suffix in the method names. This recipe (without the suffix) is used throughout the trajectory optimization code.

We use duration instead of time in kinematic traj opt.

---

planning/trajectory_optimization/gcs_trajectory_optimization.h line 294 at r1 (raw file):

    AddVertexConstraint. Passing this variable directly into
    objectives/constraints will result in an error. */
    const solvers::VectorXDecisionVariable& vertex_control_points_placeholder()

i would think these should be reshaped into the matrix of control points.

---

planning/trajectory_optimization/gcs_trajectory_optimization.h line 302 at r1 (raw file):

    as decision variables in the MathematicalProgram) associated with the time
    scaling of the trajectory in two sets within this subgraph that are
    connected by an edge. This variable will be substituted for real decision

can we clarify that this is an "internal" edge? here and the other related methods below?

---

planning/trajectory_optimization/gcs_trajectory_optimization.h line 307 at r1 (raw file):

    const std::pair<solvers::VectorDecisionVariable<1>,
                    solvers::VectorDecisionVariable<1>>&
    edge_time_placeholder() const {

edge_times? or perhaps two methods, edge_tu? and edge_tv?

---

planning/trajectory_optimization/gcs_trajectory_optimization.h line 319 at r1 (raw file):

    const std::pair<solvers::VectorXDecisionVariable,
                    solvers::VectorXDecisionVariable>&
    edge_control_points_placeholder() const {

again, i think edge_control_points_u and edge_control_points_v would be more ergonomic?

---

planning/trajectory_optimization/gcs_trajectory_optimization.h line 330 at r1 (raw file):

    solution for all possible costs.

    Costs which do not support the perspective operation cannot be used with

does that mean that it throws? assuming yes, let's use the doxygen for it.

i wonder if we should have an argument that is e.g. assert_convex = true or something like that, to help protect people from accidentally passing in nonconvex things?

---

planning/trajectory_optimization/gcs_trajectory_optimization.h line 469 at r1 (raw file):

    solvers::VectorDecisionVariable<1> placeholder_vertex_time_scaling_var_;
    solvers::VectorXDecisionVariable placeholder_vertex_control_points_var_;

fyi -- in kinematic trajectory optimization, this is stored even here as a matrix

solvers::MatrixXDecisionVariable control_points_;

the internal storage doesn't matter to me, but the return type above does.

[cohnt]

Reviewable status: 6 unresolved discussions, 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 @cohnt)

---

planning/trajectory_optimization/gcs_trajectory_optimization.h line 284 at r1 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

let's not use the _placeholder() suffix in the method names. This recipe (without the suffix) is used throughout the trajectory optimization code.

We use duration instead of time in kinematic traj opt.

Removed the placeholder suffix, and switched time for duration.

---

planning/trajectory_optimization/gcs_trajectory_optimization.h line 294 at r1 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

i would think these should be reshaped into the matrix of control points.

Done.

---

planning/trajectory_optimization/gcs_trajectory_optimization.h line 302 at r1 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

can we clarify that this is an "internal" edge? here and the other related methods below?

Done.

---

planning/trajectory_optimization/gcs_trajectory_optimization.h line 307 at r1 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

edge_times? or perhaps two methods, edge_tu? and edge_tv?

How about edge_durations, to match the language of using "duration" instead of "time"?

---

planning/trajectory_optimization/gcs_trajectory_optimization.h line 319 at r1 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

again, i think edge_control_points_u and edge_control_points_v would be more ergonomic?

If someone is trying to use GcsTrajOpt without having looked closely at the paper, I don't think u and v will be particularly meaningful.

---

planning/trajectory_optimization/gcs_trajectory_optimization.h line 330 at r1 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

does that mean that it throws? assuming yes, let's use the doxygen for it.

i wonder if we should have an argument that is e.g. assert_convex = true or something like that, to help protect people from accidentally passing in nonconvex things?

Currently, it throws once the user tries to call the SolveShortestPath method. It would be great if we could do the check here, but GCS doesn't currently have a great way to query whether or not a cost or constraint can be handled properly. (Right now, it's just doing some RTTI behind the scenes, and throwing if the cost/constraint is not one that has been specifically implemented.)

---

planning/trajectory_optimization/gcs_trajectory_optimization.h line 469 at r1 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

fyi -- in kinematic trajectory optimization, this is stored even here as a matrix

solvers::MatrixXDecisionVariable control_points_;

the internal storage doesn't matter to me, but the return type above does.

Gotcha. I went ahead and stored it internally as a MatrixXDecisionVariable, and updated the interfaces to match.

[RussTedrake]

a few more small comments/questions, otherwise

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

---

planning/trajectory_optimization/gcs_trajectory_optimization.h line 307 at r1 (raw file):

Previously, cohnt (Thomas Cohn) wrote…

How about edge_durations, to match the language of using "duration" instead of "time"?

I'm still worried that this is potentially confusing. The edges themselves do not have durations (nor times) associated with them.

---

planning/trajectory_optimization/gcs_trajectory_optimization.h line 319 at r1 (raw file):

Previously, cohnt (Thomas Cohn) wrote…

If someone is trying to use GcsTrajOpt without having looked closely at the paper, I don't think u and v will be particularly meaningful.

We use u() v() and xu() , xv() as the designations in GCS (after a similar naming discussion that didn't come up with something we liked better)

---

planning/trajectory_optimization/gcs_trajectory_optimization.h line 330 at r1 (raw file):

Previously, cohnt (Thomas Cohn) wrote…

Currently, it throws once the user tries to call the SolveShortestPath method. It would be great if we could do the check here, but GCS doesn't currently have a great way to query whether or not a cost or constraint can be handled properly. (Right now, it's just doing some RTTI behind the scenes, and throwing if the cost/constraint is not one that has been specifically implemented.)

Got it. I think it's not ideal, but I don't have a great better suggestion.

---

planning/trajectory_optimization/gcs_trajectory_optimization.h line 349 at r2 (raw file):

    /** Adds an arbitrary user-defined constraint to every vertex in the
    subgraph. The constraint should be defined using the placeholder control
    point variables (obtained frpom vertex_control_points_placeholder()) and the

typo ("frpom")

and stale references to the old method names here and in a number of more places just below.

---

planning/trajectory_optimization/gcs_trajectory_optimization.cc line 766 at r2 (raw file):

  // Note: the logic is identical for symbolic::Expression and
  // symbolic::Formula, but since there is no inheritance structure, we have to
  // split into cases based on which type is actually used.

then why not make an templated method (internal to this file, or even this method) that works for both expression and formula?

---

planning/trajectory_optimization/gcs_trajectory_optimization.cc line 818 at r2 (raw file):

  // Note: the logic is identical for symbolic::Expression and
  // symbolic::Formula, but since there is no inheritance structure, we have to
  // split into cases based on which type is actually used.

templated method?

[cohnt]

Dismissed @RussTedrake from a discussion.
Reviewable status: 4 unresolved discussions, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @cohnt)

---

planning/trajectory_optimization/gcs_trajectory_optimization.h line 307 at r1 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

I'm still worried that this is potentially confusing. The edges themselves do not have durations (nor times) associated with them.

I also don't have any great ideas for what we could call this. No matter what, we have to use something like "time" or "duration" in the method name, but we have to specify that it's an edge as well.

---

planning/trajectory_optimization/gcs_trajectory_optimization.h line 319 at r1 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

We use u() v() and xu() , xv() as the designations in GCS (after a similar naming discussion that didn't come up with something we liked better)

I'm wary of letting the users grab only the start or end control point placeholders, because then they might try to construct a vertex cost/constraint using them. I think that forcing them to get both at the same time reinforces the idea that these placeholders are different than the vertex placeholders, and cannot be interchanged.

---

planning/trajectory_optimization/gcs_trajectory_optimization.h line 349 at r2 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

typo ("frpom")

and stale references to the old method names here and in a number of more places just below.

Fixed both throughout.

---

planning/trajectory_optimization/gcs_trajectory_optimization.cc line 766 at r2 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

then why not make an templated method (internal to this file, or even this method) that works for both expression and formula?

Unfortunately, Expression has the GetVariables method, but the analogous method for Formula is GetFreeVariables. So I can't directly template it. I could set up an adapter or wrapper if you want me to?

---

planning/trajectory_optimization/gcs_trajectory_optimization.cc line 818 at r2 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

templated method?

(Same issue as above)

[RussTedrake]

something happened where the .cc files got reverted?

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

---

planning/trajectory_optimization/gcs_trajectory_optimization.h line 307 at r1 (raw file):

Previously, cohnt (Thomas Cohn) wrote…

I also don't have any great ideas for what we could call this. No matter what, we have to use something like "time" or "duration" in the method name, but we have to specify that it's an edge as well.

what about something like vertex_durations_by_edge()?

---

planning/trajectory_optimization/gcs_trajectory_optimization.h line 319 at r1 (raw file):

Previously, cohnt (Thomas Cohn) wrote…

I'm wary of letting the users grab only the start or end control point placeholders, because then they might try to construct a vertex cost/constraint using them. I think that forcing them to get both at the same time reinforces the idea that these placeholders are different than the vertex placeholders, and cannot be interchanged.

Ok. I hadn't appreciated that justification.

---

planning/trajectory_optimization/gcs_trajectory_optimization.cc line 766 at r2 (raw file):

Previously, cohnt (Thomas Cohn) wrote…

Unfortunately, Expression has the GetVariables method, but the analogous method for Formula is GetFreeVariables. So I can't directly template it. I could set up an adapter or wrapper if you want me to?

Then the comment is slightly wrong/misleading?
(Of course you could do the template and have one type-based branch in the middle if you think that is cleaner).

[cohnt]

I don't think they got reverted? The changes all show up for me, and on the pull request in Github itself. So probably some reviewable weirdness?

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

---

planning/trajectory_optimization/gcs_trajectory_optimization.h line 307 at r1 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

what about something like vertex_durations_by_edge()?

How about edge_constituent_vertex_durations()? And we could change the following method to edge_constituent_vertex_control_points() -- I do want to keep the method signatures matching.

---

planning/trajectory_optimization/gcs_trajectory_optimization.cc line 766 at r2 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

Then the comment is slightly wrong/misleading?
(Of course you could do the template and have one type-based branch in the middle if you think that is cleaner).

I mean, the methods do the exact same thing, they just have different names. I didn't realize you could do type-based branches in templated code -- is this what you meant? (I haven't written templated code since undergrad! 😆)

[RussTedrake]

Perhaps. My internet connection is bad and reviewable has been struggling.

Reviewed 3 of 3 files at r4, all commit messages.
Reviewable status: 4 unresolved discussions, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @cohnt)

---

planning/trajectory_optimization/gcs_trajectory_optimization.h line 307 at r1 (raw file):

Previously, cohnt (Thomas Cohn) wrote…

How about edge_constituent_vertex_durations()? And we could change the following method to edge_constituent_vertex_control_points() -- I do want to keep the method signatures matching.

that seems like a mouthful to me. but i'm aok with it.

---

planning/trajectory_optimization/gcs_trajectory_optimization.cc line 766 at r2 (raw file):

Previously, cohnt (Thomas Cohn) wrote…

I mean, the methods do the exact same thing, they just have different names. I didn't realize you could do type-based branches in templated code -- is this what you meant? (I haven't written templated code since undergrad! 😆)

Yep. Looks great.

Perhaps we should implement GetFreeVariables() as an overload for Expression that calls GetVariables().

---

planning/trajectory_optimization/gcs_trajectory_optimization.h line 445 at r4 (raw file):

    template <typename T, typename = std::enable_if_t<
                              std::is_same_v<T, symbolic::Expression> ||
                              std::is_same_v<T, symbolic::Formula>>>

i don't think you need/want the enable_if logic here. i'll push a fix.

---

planning/trajectory_optimization/test/gcs_trajectory_optimization_test.cc line 2667 at r4 (raw file):

    VectorXd x1 = segment.value(segment.end_time());
    EXPECT_LE(std::abs(x0[0] - x1[0]), 0.75);
  }

assuming you can throw on passing and edge placeholder to a vertex method above, then we could capture that here.

---

planning/trajectory_optimization/gcs_trajectory_optimization.cc line 774 at r4 (raw file):

  }

  // Substitute the control point variables.

Can we restrict that they only use placeholder_vertex_control_points_var here...? Or at least alarm if they pass the edge placeholders? (and the reciprocal for the edge variant below)

---

planning/trajectory_optimization/gcs_trajectory_optimization.cc line 802 at r4 (raw file):

  // Note: the logic is identical for Expression and Formula, except for one
  // differing method name -- Expression::GetVariables versus
  // Formula::GetFreeVariables.

this all seems very duplicative of the above. why not have a method that they both call, and pass e.g. placeholder_edge_control_points_var to it?

[RussTedrake]

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

---

planning/trajectory_optimization/gcs_trajectory_optimization.h line 445 at r4 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

i don't think you need/want the enable_if logic here. i'll push a fix.

Done.

[cohnt]

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

---

planning/trajectory_optimization/gcs_trajectory_optimization.h line 445 at r4 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

Done.

I included the enable_if logic to prevent the usage of the method for any other types. But if that's something we don't need to prevent users from, I'm fine dropping it and just having template<typename T>.

---

planning/trajectory_optimization/gcs_trajectory_optimization.cc line 766 at r2 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

Yep. Looks great.

Perhaps we should implement GetFreeVariables() as an overload for Expression that calls GetVariables().

I saw your other PR -- I'll integrate that change once it merges.

---

planning/trajectory_optimization/gcs_trajectory_optimization.cc line 774 at r4 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

Can we restrict that they only use placeholder_vertex_control_points_var here...? Or at least alarm if they pass the edge placeholders? (and the reciprocal for the edge variant below)

So the underlying GraphOfConvexSets::AddCost and GraphO fConvexSets::AddConstraint will throw if you try to give it random variables (and it'll throw as part of calling this function -- none of the business of not throwing an error until you actually try to solve). Do you want to specifically have the error at this level?

---

planning/trajectory_optimization/gcs_trajectory_optimization.cc line 802 at r4 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

this all seems very duplicative of the above. why not have a method that they both call, and pass e.g. placeholder_edge_control_points_var to it?

How about this?

---

planning/trajectory_optimization/test/gcs_trajectory_optimization_test.cc line 2667 at r4 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

assuming you can throw on passing and edge placeholder to a vertex method above, then we could capture that here.

I added a test case verifying that this throws an error.

[RussTedrake]

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

---

planning/trajectory_optimization/gcs_trajectory_optimization.h line 445 at r4 (raw file):

Previously, cohnt (Thomas Cohn) wrote…

I included the enable_if logic to prevent the usage of the method for any other types. But if that's something we don't need to prevent users from, I'm fine dropping it and just having template<typename T>.

Correct. We don't need to do that. It's private, anyhow, and because the gcs_traj_opt.cc will only be compiled with the types that are needed locally they wouldn't even be able to link to a different type if they wanted to.

---

planning/trajectory_optimization/gcs_trajectory_optimization.cc line 774 at r4 (raw file):

Previously, cohnt (Thomas Cohn) wrote…

So the underlying GraphOfConvexSets::AddCost and GraphO fConvexSets::AddConstraint will throw if you try to give it random variables (and it'll throw as part of calling this function -- none of the business of not throwing an error until you actually try to solve). Do you want to specifically have the error at this level?

oh. that's good enough, but then we should document with @throws in the new AddVertexConstraint, etc methods? The new test coverage is great.

---

planning/trajectory_optimization/gcs_trajectory_optimization.cc line 802 at r4 (raw file):

Previously, cohnt (Thomas Cohn) wrote…

How about this?

Nice.

---

planning/trajectory_optimization/test/gcs_trajectory_optimization_test.cc line 2677 at r6 (raw file):

  Formula bad_formula_1 = bad_expression_1 == Expression(0.0);
  Formula bad_formula_2 = bad_expression_2 == Expression(0.0);
  EXPECT_THROW(middle.AddVertexCost(bad_expression_1), std::exception);

btw -- ideally these are DRAKE_THROWS_MESSAGE

[cohnt]

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

---

planning/trajectory_optimization/gcs_trajectory_optimization.cc line 774 at r4 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

oh. that's good enough, but then we should document with @throws in the new AddVertexConstraint, etc methods? The new test coverage is great.

Added the documentation.

I also realized that the documentation ends up being a really long line. I'm guessing this is inherent to doxygen? But if you know an easy fix, let me know:

---

planning/trajectory_optimization/test/gcs_trajectory_optimization_test.cc line 2677 at r6 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

btw -- ideally these are DRAKE_THROWS_MESSAGE

Done.

Note that the error message is "Failure at geometry/optimization/graph_of_convex_sets.cc:387 in AddConstraint(): condition 'Variables(binding.variables()).IsSubsetOf(allowed_vars_)' failed." which isn't the clearest explanation. Do you think we need to add an additional error check at the GcsTrajOpt level, or can we let it be?

[RussTedrake]

This is great @cohnt . Thanks!

+@sammy-tri for platform review, please.

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

---

planning/trajectory_optimization/gcs_trajectory_optimization.cc line 774 at r4 (raw file):

Previously, cohnt (Thomas Cohn) wrote…

Added the documentation.

I also realized that the documentation ends up being a really long line. I'm guessing this is inherent to doxygen? But if you know an easy fix, let me know:

i agree we shouldn't have to handle that locally here.

---

planning/trajectory_optimization/test/gcs_trajectory_optimization_test.cc line 2677 at r6 (raw file):

Previously, cohnt (Thomas Cohn) wrote…

Done.

Note that the error message is "Failure at geometry/optimization/graph_of_convex_sets.cc:387 in AddConstraint(): condition 'Variables(binding.variables()).IsSubsetOf(allowed_vars_)' failed." which isn't the clearest explanation. Do you think we need to add an additional error check at the GcsTrajOpt level, or can we let it be?

Thanks for the update. I think it makes the docstrings much more clear.
Re: error message. Your call. I agree it could be a nice improvement, but I don't think it has to block this PR.

[sammy-tri]

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

---

planning/trajectory_optimization/gcs_trajectory_optimization.cc line 766 at r2 (raw file):

Previously, cohnt (Thomas Cohn) wrote…

I saw your other PR -- I'll integrate that change once it merges.

It has merged.

[cohnt]

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

---

planning/trajectory_optimization/test/gcs_trajectory_optimization_test.cc line 2677 at r6 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

Thanks for the update. I think it makes the docstrings much more clear.
Re: error message. Your call. I agree it could be a nice improvement, but I don't think it has to block this PR.

I went ahead and added an extra check specifically for swapping the edge and vertex placeholder variables -- that seems like something one could do by accident, so having a clear error message seems worth it. It's a small runtime cost on the parsing stage, which will be insignificant compared to solving the programs.

[RussTedrake]

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

[sammy-tri]

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

---

planning/trajectory_optimization/test/gcs_trajectory_optimization_test.cc line 2747 at r8 (raw file):

  middle.AddEdgeConstraint(outgoing_time_minimum && equal_time_constraint &&
                           control_point_limit);
  // middle.AddEdgeConstraint(outgoing_time_minimum && equal_time_constraint);

leftover debug code? Or is this supposed to be part of the test? (it doesn't look like it)

---

planning/trajectory_optimization/gcs_trajectory_optimization.h line 354 at r8 (raw file):

    time scaling variable (obtained from vertex_time()). This enables greater
    modeling freedom, but we cannot guarantee a feasible solution for all
    possible costs.

nit: "possible costs" instead of "possible constraints"

---

planning/trajectory_optimization/gcs_trajectory_optimization.h line 377 at r8 (raw file):

    /** Adds an arbitrary user-defined cost to every internal edge within the
    subgraph. The cost should be defined using the placeholder control point
    variables (obtained from edge_control_points()) and the placeholder time

nit: should these function names have _constituent_ in the middle? (also on the next function below)

---

planning/trajectory_optimization/gcs_trajectory_optimization.h line 405 at r8 (raw file):

    time scaling variables (obtained from edge_time()). This enables greater
    modeling freedom, but we cannot guarantee a feasible solution for all
    possible costs.

nit: "possible costs"

[cohnt]

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

---

planning/trajectory_optimization/gcs_trajectory_optimization.h line 354 at r8 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

nit: "possible costs" instead of "possible constraints"

Done.

---

planning/trajectory_optimization/gcs_trajectory_optimization.h line 377 at r8 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

nit: should these function names have _constituent_ in the middle? (also on the next function below)

Done.

---

planning/trajectory_optimization/gcs_trajectory_optimization.h line 405 at r8 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

nit: "possible costs"

Done.

---

planning/trajectory_optimization/test/gcs_trajectory_optimization_test.cc line 2747 at r8 (raw file):

Previously, sammy-tri (Sam Creasey) wrote…

leftover debug code? Or is this supposed to be part of the test? (it doesn't look like it)

Old debug code. I've removed it.

[sammy-tri]

+(status: squashing now)

Reviewed 2 of 2 files at r9, all commit messages.
Reviewable status: complete! all discussions resolved, LGTM from assignees sammy-tri(platform),RussTedrake(platform)

[RussTedrake]

Reviewed 2 of 2 files at r9, all commit messages.
Reviewable status: complete! all discussions resolved, LGTM from assignees sammy-tri(platform),RussTedrake(platform)