-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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)
There was a problem hiding this 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.
f836abc
to
ce4ebb6
Compare
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
andsolvers::internal::ParseConstraint
to convertExpression
andFormula
toBinding<Cost>
andBinding<Constraint>
, respectively.+@RussTedrake for feature review, please! cc @wrangelvid
This change is