-
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
Improve GCS graphviz #21530
Improve GCS graphviz #21530
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, needs platform reviewer assigned, 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 483 at r1 (raw file):
graphviz << ", style=\"dashed\""; graphviz << "];\n"; }
we need to confirm this functionality in the GraphViz test in graph_of_convex_sets_test
bindings/pydrake/geometry/geometry_py_optimization.cc
line 882 at r1 (raw file):
py::arg("precision") = 3, py::arg("scientific") = false, py::arg("show_vars") = true, py::arg("show_costs") = true, py::arg("show_flows") = true, py::arg("active_path") = std::nullopt,
we need the py test to call these values (to make sure they are spelled correctly).
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.
Assigning +@RussTedrake on this since he's already commenting here. However, Russ is out for the next week; if you would like a quicker turnaround we can assign a different reviewer.
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), missing label for release notes (waiting on @bernhardpg)
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.
One note on argument design below.
Reviewed 3 of 3 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: 3 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), missing label for release notes (waiting on @bernhardpg)
geometry/optimization/graph_of_convex_sets.h
line 594 at r2 (raw file):
bool show_vars = true, bool show_costs = true, bool show_flows = true, const std::optional<std::vector<const Edge*>>& active_path = std::nullopt) const;
minor: With the large number of optional boolean arguments here, combined with C++'s lack of keyword arguments, the likelihood of user error -- setting the wrong parameter -- is very high (the /*foo=*/
comments in the test are indicative of this).
Per https://drake.mit.edu/styleguide/cppguide.html#Function_Argument_Comments , "Consider changing the function signature to replace a bool
argument with an enum
argument" or "For functions that have several configuration options, consider defining a single class or struct to hold all the options"
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.
pending CI. I'll wait for you to respond to Grant's suggestion before passing to platform review.
Reviewed 2 of 2 files at r2, 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), missing label for release notes (waiting on @bernhardpg)
a042e2e
to
ad0ee7b
Compare
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.
All comments addressed.
@RussTedrake Two tests are currently failing. One test is failing because of linting issues. How do you typically go about debugging this?
The other test is failing because of what I believe is a bug somewhere in GcsTrajectoryOptimization
. We are getting the following error:
Traceback (most recent call last):
File "/tmp/bazel-working-directory/drake/bazel-out/k8-opt/bin/bindings/pydrake/planning/py/trajectory\_optimization\_test.runfiles/drake/bindings/pydrake/planning/test/trajectory\_optimization\_test.py", line 589, in test\_gcs\_trajectory\_optimization\_2d
gcs.GetGraphvizString(result=result,
ValueError: GetVariableValue: v\_ell0 is not captured by the variable\_index map.
which is caused by this line, where are we are now also showing the vertex costs (which we were not before).
If this is indeed a bug with GcsTrajectoryOptimization
I suggest changing the failing test with graphviz_options.show_costs = False
so we can merge this PR and put the bug fix (which is not really related to this PR) in its own PR. Alternatively, we can wait with merging this PR until the bug is addressed in a separate PR.
Reviewable status: 1 unresolved discussion, 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.h
line 594 at r2 (raw file):
Previously, ggould-tri wrote…
minor: With the large number of optional boolean arguments here, combined with C++'s lack of keyword arguments, the likelihood of user error -- setting the wrong parameter -- is very high (the
/*foo=*/
comments in the test are indicative of this).Per https://drake.mit.edu/styleguide/cppguide.html#Function_Argument_Comments , "Consider changing the function signature to replace a
bool
argument with anenum
argument" or "For functions that have several configuration options, consider defining a single class or struct to hold all the options"
Agreed. Done.
geometry/optimization/graph_of_convex_sets.cc
line 483 at r1 (raw file):
Previously, RussTedrake (Russ Tedrake) wrote…
we need to confirm this functionality in the GraphViz test in graph_of_convex_sets_test
Done.
bindings/pydrake/geometry/geometry_py_optimization.cc
line 882 at r1 (raw file):
Previously, RussTedrake (Russ Tedrake) wrote…
we need the py test to call these values (to make sure they are spelled correctly).
Done.
You should be running the tests locally. Taking a look at the getvariable error |
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.
Reviewable status: 2 unresolved discussions, 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.h
line 594 at r2 (raw file):
Previously, bernhardpg wrote…
Agreed. Done.
Ah. This is now a breaking change. Technically GCS is still marked as experimental, so we can make breaking changes to the API. But it would be kinder/better to also keep the original function signature, but implement that as a one-liner that uses this entry point. Then you can deprecate the original entry point for eventual removal.
geometry/optimization/graph_of_convex_sets.cc
line 976 at r3 (raw file):
} auto IncludesCurrentTranscription =
you've got some strange commit history here. this was introduced in a commit which landed last night.
Previously, RussTedrake (Russ Tedrake) wrote…
https://drake.mit.edu/doxygen_cxx/drake__deprecated_8h.html#a847d827d084f28e4ae1788a0ae75be96 |
actually there is a way to accept the struct fields as arguments here. if we did that, then python users wouldn't have to change their code. in that case, I think we could avoid the deprecation completely (I suspect almost all users of the GetGraphVizString are doing that from python). |
consider the use of ParamInit (e.g. as used in the ContactVisualizerParams). I know the other structs here don't use them yet, but perhaps they should...? But i think it does not actually make the original identical syntax go through. So we probably should deprecate the python bindings, too. I would be ok with deprecating only the python bindings, and letting the c++ be a breaking change. |
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.
Addressed all your comments @SeanCurtis-TRI , except for the discussion commit message which is still unresolved.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
What you propose is one solution. I'd be equally content if the author would write their own take on the message and post it in this discussion. I'd be equally content if the feature reviewer were to do so. I'd be happy to use it in squashing, but I'd rather not have to come up with the message myself.
That said, it seems reasonable that the current PR title and summary seems consistent with what the commit message should be. (Possibly with the added note about C++ API breakage).
So what should I do here?
I think the commit message should be something like:
Extend the GraphOfConvexSets Graphviz functionality
.
Should the commit also include a detailed commit body?
geometry/optimization/graph_of_convex_sets.h
line 130 at r4 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
BTW On all of these comments, the line wrapping is incredibly odd. Typically, lines fill out as much as they can before wrapping. This isn't strictly a defect, but it will almost definitely be touched by someone else in the future. Perhaps do it now?
Done.
geometry/optimization/graph_of_convex_sets.h
line 130 at r4 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: You might consider making this serializable (it'll make it easier to bind and allow us to include this set of options in yaml files). It would look something like:
#include "drake/common/name_value.h" struct GcsGraphvizOptions { template <typename Archive> void Serialize(Archive* a) { a->Visit(DRAKE_NVP(show_slacks)); a->Visit(DRAKE_NVP(show_vars)); ... etc } ... };You can find lots of examples in the structs defined in any *_config.h file.
Done.
geometry/optimization/graph_of_convex_sets.h
line 141 at r4 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
BTW Are you worried about an expected edge not appearing because it has zero flow? Might it not confuse a user? An edge not being drawn can mean two things introducing ambiguity.
Good point. Solved by showing edges with zero flow as almost fully transparent instead of invisible.
geometry/optimization/test/graph_of_convex_sets_test.cc
line 2970 at r4 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
BTW This test is fine, but it would be a bit stronger if you showed that there were no "color" if you don't pass a path and that, for the options values, it is only adding the path that introduces "color=".
Done.
bindings/pydrake/geometry/geometry_py_optimization.cc
line 721 at r4 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
BTW If you added the ability to serialize
GcsGraphvixOptions
then you can use this paradigm:{ using Class = GcsGraphvizOptions; constexpr auto& cls_doc = doc.GcsGraphvizOptions; py::class_<Class> cls(m, "GcsGraphvizOptions", cls_doc.doc); cls.def(ParamInit<Class>()); DefAttributesUsingSerialize(&cls, cls_doc); DefReprUsingSerialize(&cls); DefCopyAndDeepCopy(&cls); }This also future proofs it against new properties.
Done.
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.
@SeanCurtis-TRI -- this one is waiting for you, too.
Reviewed 3 of 5 files at r5, 2 of 2 files at r6, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @bernhardpg)
Previously, bernhardpg wrote…
So what should I do here?
I think the commit message should be something like:
Extend the GraphOfConvexSets Graphviz functionality
.Should the commit also include a detailed commit body?
Normally a few sentences of description would be appropriate for the commit body, if they help to explain.
But perhaps a good guiding principle is thinking about the fact that the monthly release notes author needs to be able to crawl through and understand/catalog all of the commits to produce e.g.: https://drake.mit.edu/release_notes/v1.30.0.html .
That doesn't require much text, but does require clarity.
Cleaning documentation and struct ordering.
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.
+(release notes: feature)
(Note: in addition to the breaking change, this also includes a new feature.)
Reviewed 3 of 5 files at r5, 2 of 2 files at r6.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @bernhardpg)
Previously, RussTedrake (Russ Tedrake) wrote…
Normally a few sentences of description would be appropriate for the commit body, if they help to explain.
But perhaps a good guiding principle is thinking about the fact that the monthly release notes author needs to be able to crawl through and understand/catalog all of the commits to produce e.g.: https://drake.mit.edu/release_notes/v1.30.0.html .
That doesn't require much text, but does require clarity.
@bernhardpg I'd vote that we simply use the PR title and summary as the commit message. If you agree with that, I can copy and paste it when we squash merge thing.
If you want to take action, you can squash it all locally and edit the commit message yourself before force pushing it again.
geometry/optimization/graph_of_convex_sets.h
line 130 at r4 (raw file):
Previously, bernhardpg wrote…
Done.
The pattern used elsewhere (and required by the style guide) would have the Serialize()
function come before the declaration of the members.
I recognize that you've matched the previous struct in this file, but, in this case, that's a bad example.
geometry/optimization/graph_of_convex_sets.h
line 136 at r6 (raw file):
/** Determines whether the solution values for decision variables in each set * are shown. */
nit: The styleguide requests consistent formatting of comments in a file. I'm betting your IDE injected leading *
on these comments.
geometry/optimization/graph_of_convex_sets.h
line 670 at r6 (raw file):
void ClearAllPhiConstraints(); /**
nit: New line injected here.
geometry/optimization/test/graph_of_convex_sets_test.cc
line 2905 at r6 (raw file):
GTEST_TEST(GcsGraphvizOptionsTest, Serialize) { GcsGraphvizOptions options;
FYI You may not be aware of designated initializers, but they are fun:
const GcsGraphvizOptions options {
.show_slacks = false,
.show_vars = false,
.show_flows = false,
.show_costs = false,
.scientific = true,
.precision = 5};
Gives you the benefit of defining values with clear semantics at the same time you construct without needing to leave a mutable instance flapping around.
(This is just informational, what you have here isn't defective.)
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 1 of 1 files at r7, 1 of 1 files at r8.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @bernhardpg)
geometry/optimization/graph_of_convex_sets.h
line 136 at r6 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: The styleguide requests consistent formatting of comments in a file. I'm betting your IDE injected leading
*
on these comments.
I submitted a commit to fix this.
geometry/optimization/graph_of_convex_sets.h
line 670 at r6 (raw file):
Previously, SeanCurtis-TRI (Sean Curtis) wrote…
nit: New line injected here.
I submitted a commit to fix this.
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 all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @bernhardpg)
@drake-jenkins-bot mac-arm-ventura-clang-bazel-experimental-release please |
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 1 of 1 files at r9, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @bernhardpg)
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 1 of 1 files at r7, 1 of 1 files at r9, all commit messages.
Reviewable status: LGTM missing from assignee SeanCurtis-TRI(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @bernhardpg)
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.
@bernhardpg This has been ready to merge for a bit. I'm going to squash it with your PR summary as the commit message. This is your last change to weigh in.
Reviewable status: commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @bernhardpg)
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.
Sorry for the slow response. That sounds good to me!
Reviewable status: commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @bernhardpg)
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.
No worries. You weren't alone. :)
Reviewable status: commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @bernhardpg)
+(status: squashing now) |
… restriction **This is a breaking change** (Note that GCS is still marked experimental) During the review of RobotLocomotion#21530, we realized that cost slack variables were not being written to the MathematicalProgramResult in a consistent way between SolveShortestPath and SolveConvexRestriction. This PR addresses this by: 1) The "ell" slack variables associated with the AddCost methods are not returned to the user. Users should call vertex->GetSolutionCost() or edge->GetSolutionCost() to retrieve them. SolveConvexRestriction doesn't use ell, and it's an implementation detail that shouldn't have ever been exposed to the user. 2) GetSolutionCost and GetSolution are now more robust... if the placeholder variables are not in the MathematicalProgramResult, they return the implied values. 3) SolveConvexRestriction no longer tries to write NaN values for the unused vertices on every solve. The more robust GetSolution methods make this reasonable, and now that we've been generating large graphs this has become more important. Users may need to change calls from e.g. `result->GetSolution(v->x())` to `v->GetSolution(result)`. 4) The artificial `graphviz_options.show_costs = False` that was needed in `trajectory_optimization_test.py` to cover up this issue is now safely removed. Resolves RobotLocomotion#20443.
… restriction **This is a breaking change** (Note that GCS is still marked experimental) During the review of RobotLocomotion#21530, we realized that cost slack variables were not being written to the MathematicalProgramResult in a consistent way between SolveShortestPath and SolveConvexRestriction. This PR addresses this by: 1) The "ell" slack variables associated with the AddCost methods are not returned to the user. Users should call vertex->GetSolutionCost() or edge->GetSolutionCost() to retrieve them. SolveConvexRestriction doesn't use ell, and it's an implementation detail that shouldn't have ever been exposed to the user. 2) GetSolutionCost and GetSolution are now more robust... if the placeholder variables are not in the MathematicalProgramResult, they return the implied values. 3) SolveConvexRestriction no longer tries to write NaN values for the unused vertices on every solve. The more robust GetSolution methods make this reasonable, and now that we've been generating large graphs this has become more important. Users may need to change calls from e.g. `result->GetSolution(v->x())` to `v->GetSolution(result)`. 4) The artificial `graphviz_options.show_costs = False` that was needed in `trajectory_optimization_test.py` to cover up this issue is now safely removed. Resolves RobotLocomotion#20443.
… restriction **This is a breaking change** (Note that GCS is still marked experimental) During the review of RobotLocomotion#21530, we realized that cost slack variables were not being written to the MathematicalProgramResult in a consistent way between SolveShortestPath and SolveConvexRestriction. This PR addresses this by: 1) The "ell" slack variables associated with the AddCost methods are not returned to the user. Users should call vertex->GetSolutionCost() or edge->GetSolutionCost() to retrieve them. SolveConvexRestriction doesn't use ell, and it's an implementation detail that shouldn't have ever been exposed to the user. 2) GetSolutionCost and GetSolution are now more robust... if the placeholder variables are not in the MathematicalProgramResult, they return the implied values. 3) SolveConvexRestriction no longer tries to write NaN values for the unused vertices on every solve. The more robust GetSolution methods make this reasonable, and now that we've been generating large graphs this has become more important. Users may need to change calls from e.g. `result->GetSolution(v->x())` to `v->GetSolution(result)`. 4) The artificial `graphviz_options.show_costs = False` that was needed in `trajectory_optimization_test.py` to cover up this issue is now safely removed. Resolves RobotLocomotion#20443.
… restriction (#21833) **This is a breaking change** (Note that GCS is still marked experimental) During the review of #21530, we realized that cost slack variables were not being written to the MathematicalProgramResult in a consistent way between SolveShortestPath and SolveConvexRestriction. This PR addresses this by: 1) The "ell" slack variables associated with the AddCost methods are not returned to the user. Users should call vertex->GetSolutionCost() or edge->GetSolutionCost() to retrieve them. SolveConvexRestriction doesn't use ell, and it's an implementation detail that shouldn't have ever been exposed to the user. 2) GetSolutionCost and GetSolution are now more robust... if the placeholder variables are not in the MathematicalProgramResult, they return the implied values. 3) SolveConvexRestriction no longer tries to write NaN values for the unused vertices on every solve. The more robust GetSolution methods make this reasonable, and now that we've been generating large graphs this has become more important. Users may need to change calls from e.g. `result->GetSolution(v->x())` to `v->GetSolution(result)`. 4) The artificial `graphviz_options.show_costs = False` that was needed in `trajectory_optimization_test.py` to cover up this issue is now safely removed. Resolves #20443.
Adds a few features to the GCS Graphviz code when a
result
is passed, and additionally make it possible to turn on/off the visualization of different quantities.jChanges:
show_vars
: Decide to show vertex var resultsshow_costs
: Decide to show costs AND also plot vertex costs (these were not currently shown)show_flows
: Plots the flow values as\phi = ..
and also dims the edge based on the flow valueactive_path
: If provided, the graph will show the path in red on the graph. This is useful when debugging workflows with multiple different paths through the graph, to quickly be able to show a path in the graph.This change is