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

Implement support for slack variables on edges in GraphOfConvexSets #21403

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

Conversation

bernhardpg
Copy link
Contributor

@bernhardpg bernhardpg commented May 7, 2024

This addresses #21003.

The unit tests implement the two examples from the issue above.

Note 1: I named the function NewSlackVariables. It could alternatively be called NewContinuousSlackVariables to be closer to the naming conventions of the member functions in MathematicalProgram.

Note 2: I could also add another helper function called NewSlackVariable (or NewContinuousSlackVariable) (singular, in addition to plural) if we think that would be natural. I imagine that for most use-cases, most people will only add one slack variable at a time.


This change is Reviewable

@bernhardpg
Copy link
Contributor Author

@TobiaMarcucci FYI

Copy link
Member

@sherm1 sherm1 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, please (or delegate)

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), missing label for release notes (waiting on @bernhardpg)

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 3 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee RussTedrake(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @bernhardpg)


geometry/optimization/graph_of_convex_sets.cc line 959 at r2 (raw file):

    prog.AddDecisionVariables(e->z_);
    prog.AddDecisionVariables(e->ell_);
    prog.AddDecisionVariables(e->slacks_);

we need to add this to SolveConvexRestriction, too.

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.

One question, I guess, is whether we want to implement the "vertex isa MathematicalProgram" idea first, and just give more direct access to MathematicalProgram's API instead of introducing this new method?

Reviewable status: 1 unresolved discussion, LGTM missing from assignee RussTedrake(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @bernhardpg)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants