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

[planning] Add velocity bounds to subgraphs #19334

Merged

Conversation

wrangelvid
Copy link
Contributor

@wrangelvid wrangelvid commented May 2, 2023

This adds velocity bounds to the subgraphs, which enables useful minimum time planning.


This change is Reviewable

Copy link
Contributor Author

@wrangelvid wrangelvid left a comment

Choose a reason for hiding this comment

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

+@RussTedrake for feature review.

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

Copy link
Contributor

@RussTedrake RussTedrake left a comment

Choose a reason for hiding this comment

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

awesome! i like the test/example. :lgtm: with a few small questions/corrections.

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: 8 unresolved discussions, needs at least two assigned reviewers, missing label for release notes (waiting on @wrangelvid)


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

    @param ub is the upper bound of the velocity.

    The subgraph order must be greater than zero, since the velocity is defined

You should write e.g.

throws std::exception if the subgraph order is zero...

you also throw if the sizes of lb/ub are wrong.


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

  forming the derivative of the Bézier curve, this constraint will only added to
  all subgraphs with order greater than zero. Note that this constraint will be
  applied even to subgraphs added in the future.

report that you throw if lb and ub are the wrong size.


planning/trajectory_optimization/gcs_trajectory_optimization.cc line 51 at r1 (raw file):

using EdgeId = GraphOfConvexSets::EdgeId;

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

inf => kInf.


planning/trajectory_optimization/gcs_trajectory_optimization.cc line 217 at r1 (raw file):

  // We have q̇(t) = drds * dsdt = ṙ(s) / h, and h >= 0, so we
  // use duration * lb <= ṙ(s) <= duration * ub. But will be reformulated as:

duration => h (twice)

nit: "But will be reformulated as" . maybe ", formulated as:"


planning/trajectory_optimization/test/gcs_trajectory_optimization_test.cc line 118 at r1 (raw file):

  // Add shortest path objective.
  gcs.AddPathLengthCost();

nit: seems inconsistent with the problem setup? add a line saying that you


planning/trajectory_optimization/test/gcs_trajectory_optimization_test.cc line 158 at r1 (raw file):

  // Now we want to find the fastest path from S to G.
  // Add minimum time objective with a very high weight to overpower the
  // shortest path objective.

nit: would be slightly cleaner to not have both in the same problem, but i realize that removing the costs would be nontrivial, and making a new gcs would be a bunch of lines...


planning/trajectory_optimization/test/gcs_trajectory_optimization_test.cc line 171 at r1 (raw file):

  // We expect the fastest path to take a detour and avoid the slow gravel,
  // going through the gravel. Thus we expect the number of segments in the

nit: redundant. "slow gravel, going through the gravel"


bindings/pydrake/planning/test/trajectory_optimization_test.py line 340 at r1 (raw file):

        # Add velocity bounds to the entire graph.
        gcs.AddVelocityBounds(-max_vel, max_vel)

lb=, ub=

@wrangelvid wrangelvid force-pushed the gcs_traj_opt/velocity_bounds branch from ad86e4f to 0c24921 Compare May 3, 2023 03:12
Copy link
Contributor Author

@wrangelvid wrangelvid 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: 5 unresolved discussions, needs at least two assigned reviewers, missing label for release notes (waiting on @wrangelvid)


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

Previously, RussTedrake (Russ Tedrake) wrote…

You should write e.g.

throws std::exception if the subgraph order is zero...

you also throw if the sizes of lb/ub are wrong.

Done.


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

Previously, RussTedrake (Russ Tedrake) wrote…

report that you throw if lb and ub are the wrong size.

Done.


planning/trajectory_optimization/gcs_trajectory_optimization.cc line 51 at r1 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

inf => kInf.

Done.


planning/trajectory_optimization/gcs_trajectory_optimization.cc line 217 at r1 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

duration => h (twice)

nit: "But will be reformulated as" . maybe ", formulated as:"

Done.


planning/trajectory_optimization/test/gcs_trajectory_optimization_test.cc line 118 at r1 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

nit: seems inconsistent with the problem setup? add a line saying that you

Done.


planning/trajectory_optimization/test/gcs_trajectory_optimization_test.cc line 158 at r1 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

nit: would be slightly cleaner to not have both in the same problem, but i realize that removing the costs would be nontrivial, and making a new gcs would be a bunch of lines...

.Clone() would be a nice addition that could get around that issue. I'll add a TODO statement.


planning/trajectory_optimization/test/gcs_trajectory_optimization_test.cc line 171 at r1 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

nit: redundant. "slow gravel, going through the gravel"

Done.


bindings/pydrake/planning/test/trajectory_optimization_test.py line 340 at r1 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

lb=, ub=

Done.d

@RussTedrake
Copy link
Contributor

I don't think you pushed any of the changes?

@wrangelvid wrangelvid force-pushed the gcs_traj_opt/velocity_bounds branch from 0c24921 to e5b1efc Compare May 3, 2023 17:17
Copy link
Contributor Author

@wrangelvid wrangelvid left a comment

Choose a reason for hiding this comment

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

fixed.

Reviewable status: 5 unresolved discussions, needs at least two assigned reviewers, missing label for release notes (waiting on @wrangelvid)

Copy link
Contributor

@RussTedrake RussTedrake left a comment

Choose a reason for hiding this comment

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

:lgtm: again.
+@xuchenhan-tri for platform review, please.

Reviewed 4 of 4 files at r2, all commit messages.
Reviewable status: LGTM missing from assignee xuchenhan-tri(platform), missing label for release notes (waiting on @wrangelvid and @xuchenhan-tri)

@xuchenhan-tri xuchenhan-tri added the release notes: feature This pull request contains a new feature label May 3, 2023
Copy link
Contributor

@xuchenhan-tri xuchenhan-tri left a comment

Choose a reason for hiding this comment

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

+(release notes: feature) :lgtm:

Reviewed 1 of 5 files at r1, 4 of 4 files at r2, all commit messages.
Reviewable status: 4 unresolved discussions


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

    void AddPathLengthCost(double weight = 1.0);

    /** Adds a linear velocity constraints to the subgraph `lb` ≤ q̇(t) ≤

nit

Suggestion:

 Adds a linear velocity constraint to the subgraph 

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

  void AddPathLengthCost(double weight = 1.0);

  /** Adds a linear velocity constraints to the entire graph `lb` ≤ q̇(t) ≤

nit

Suggestion:

Adds a linear velocity constraint to the entire graph

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

  // constraints for all t.

  const MatrixX<Expression> u_rdot_control =

nit looks like a copy isn't necessary here.

Suggestion:

const MatrixX<Expression>& u_rdot_control =

planning/trajectory_optimization/test/gcs_trajectory_optimization_test.cc line 70 at r2 (raw file):

  ░ - wall
  ▒ - grave

typo
BTW, cool example.

Suggestion:

gravel

@wrangelvid wrangelvid force-pushed the gcs_traj_opt/velocity_bounds branch from e5b1efc to 562bf96 Compare May 4, 2023 01:11
Copy link
Contributor Author

@wrangelvid wrangelvid 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: :shipit: complete! all discussions resolved, LGTM from assignees RussTedrake(platform),xuchenhan-tri(platform)


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

Previously, xuchenhan-tri wrote…

nit

Done.


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

Previously, xuchenhan-tri wrote…

nit

Done.


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

Previously, xuchenhan-tri wrote…

nit looks like a copy isn't necessary here.

The test cases are failing with &.


planning/trajectory_optimization/test/gcs_trajectory_optimization_test.cc line 70 at r2 (raw file):

Previously, xuchenhan-tri wrote…

typo
BTW, cool example.

Done.

Copy link
Contributor

@RussTedrake RussTedrake left a comment

Choose a reason for hiding this comment

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

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

@wrangelvid wrangelvid merged commit eed60c4 into RobotLocomotion:master May 4, 2023
9 checks passed
@wrangelvid wrangelvid deleted the gcs_traj_opt/velocity_bounds branch May 4, 2023 01:31
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants