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

Add Generic Binding<Cost> and Binding<Constraint> to vertices and edges within a GcsTrajectoryOptimization::Subgraph #22179

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

Conversation

cohnt
Copy link
Contributor

@cohnt cohnt commented Nov 14, 2024

Towards #21981.

The use of templates gets a little messy, but allows a ton of code reuse. Note that in the test cases, we use solvers::internal::ParseCost and solvers::internal::ParseConstraint to convert Expression and Formula to Binding<Cost> and Binding<Constraint>, respectively.

+@RussTedrake for feature review, please! cc @wrangelvid


This change is Reviewable

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 3 of 3 files at r1, all commit messages.
Reviewable status: 2 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 340 at r1 (raw file):

    or exclusively using the SolveConvexRestriction method. */
    template <typename T>
    void AddVertexCost(

i would recommend that the public API has the two specific entry points (with Expression and Binding) and no template -- but you can certainly delegate via a single line to a private (or cc only method) that uses templates to keep the code clean. Then the types are explicit in the signature, and don't have to be described in the docstring.


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

template void Subgraph::AddVertexCost<Binding<Cost>>(
    const Binding<Cost>& e,
    const std::unordered_set<Transcription>& transcriptions);

these would go away when you explicitly declare the correct entry points in the class. (also all of the other explicit instantiations below)

Copy link
Contributor Author

@cohnt cohnt left a comment

Choose a reason for hiding this comment

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

I went ahead and added the python bindings while I'm here, since we had to change the old bindings to use py::overload_cast.

Also, we were using "used_in_transcription" throughout, which doesn't match GCS (it has "use_in_transcription"). I've gone through and fixed all instances -- since this is tagged @experimental, we should be able to get away without the standard deprecation timeline? I'll be more careful about matching naming conventions in the future.

Reviewable status: 2 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 340 at r1 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

i would recommend that the public API has the two specific entry points (with Expression and Binding) and no template -- but you can certainly delegate via a single line to a private (or cc only method) that uses templates to keep the code clean. Then the types are explicit in the signature, and don't have to be described in the docstring.

Done.


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

Previously, RussTedrake (Russ Tedrake) wrote…

these would go away when you explicitly declare the correct entry points in the class. (also all of the other explicit instantiations below)

I found that I still got errors if I removed the explicit instantiations, but maybe I did something slightly different than what you were thinking.

…ded an invalid variable, instead of properly catching the bug. This fixes that bug, and updates the error message we check for.
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
Status: In Progress
Development

Successfully merging this pull request may close these issues.

2 participants