-
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
Implements ExpressionCost #19405
Implements ExpressionCost #19405
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.
See RussTedrake/drake@expression_cost...jwnimmer-tri:drake:expression_cost for a suggested change. (No need to copy-paste the implementation.)
Reviewed 2 of 6 files at r1, all commit messages.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers (waiting on @RussTedrake)
a discussion (no related file):
The docs for this function on mathematical_program.h
are out of date now:
/**
* Adds a cost in the symbolic form.
* Note that the constant part of the cost is ignored. So if you set
* `e = x + 2`, then only the cost on `x` is added, the constant term 2 is
* ignored.
* @param e The linear or quadratic expression of the cost.
* @pre `e` is linear or `e` is quadratic. Otherwise throws a runtime error.
* @return The newly created cost, together with the bound variables.
*
* @exclude_from_pydrake_mkdoc{Not bound in pydrake.}
*/
Binding<Cost> AddCost(const symbolic::Expression& e);
Both in terms of the preconditions, and the ignoring of the constant factor.
When resolving it, having the constant factor be sometimes-ignored and sometimes-obeyed seems too confusing on first impression, so we will need to figure out how to do it all one way or the other, and the test cases should reflect whichever contract we choose.
solvers/BUILD.bazel
line 210 at r1 (raw file):
], deps = [ "//common/symbolic:polynomial",
nit Unused new deps symbolic:polynomial
?
Oops, I pasted the wrong URL. I meant to link to RussTedrake#49. |
c728e0b
to
e339f52
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.
i considered doing it this way, but didn't think we would want cost to depend on constraint at the root implementation level (plus it was only a few lines).
Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @jwnimmer-tri and @RussTedrake)
a discussion (no related file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
The docs for this function on
mathematical_program.h
are out of date now:/** * Adds a cost in the symbolic form. * Note that the constant part of the cost is ignored. So if you set * `e = x + 2`, then only the cost on `x` is added, the constant term 2 is * ignored. * @param e The linear or quadratic expression of the cost. * @pre `e` is linear or `e` is quadratic. Otherwise throws a runtime error. * @return The newly created cost, together with the bound variables. * * @exclude_from_pydrake_mkdoc{Not bound in pydrake.} */ Binding<Cost> AddCost(const symbolic::Expression& e);Both in terms of the preconditions, and the ignoring of the constant factor.
When resolving it, having the constant factor be sometimes-ignored and sometimes-obeyed seems too confusing on first impression, so we will need to figure out how to do it all one way or the other, and the test cases should reflect whichever contract we choose.
Done. Good catch. It turns out the the AddQuadraticCost docstring was a lie, too. (The existing tests proved it, but I've added a narrower test to confirm). And the python binding was bizarre, but is now fixed.
I don't see any other places where it claims that the constant is dropped.
fyi @hongkai-dai
solvers/BUILD.bazel
line 210 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
nit Unused new deps
symbolic:polynomial
?
Done. (it was providing the decompose.h, but that was also not needed anymore)
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 should have also clarified -- my main motivation in de-duplication was to avoiding duplicating the non-threadsafe environment_
member field into a second place.
+@xuchenhan-tri for platform review per schedule, please. (I'm on-call today.)
Reviewed 1 of 6 files at r1, 6 of 6 files at r2, all commit messages.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee xuchenhan-tri(platform) (waiting on @xuchenhan-tri)
solvers/cost.h
line 8 at r2 (raw file):
#include <optional> #include <string> #include <unordered_map>
nit Unused <unordered_map>
. (I thought I'd removed this line in my PR but maybe I forgot to push that edit to github.)
solvers/test/cost_test.cc
line 772 at r2 (raw file):
std::ostringstream os; cost.Display(os, x_e); EXPECT_EQ(fmt::format("{}", os.str()), "ExpressionCost (x * sin(y))");
nit No need for format, as best I can tell.
Suggestion:
EXPECT_EQ(os.str(), "ExpressionCost (x * sin(y))");
Most importantly, now AddCost(Expression) will succeed, even if it is passed in an Expression that is non-Polynomial. Resolves RobotLocomotion#17897.
e339f52
to
39f7654
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.
Reviewable status: LGTM missing from assignee xuchenhan-tri(platform) (waiting on @xuchenhan-tri)
solvers/cost.h
line 8 at r2 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
nit Unused
<unordered_map>
. (I thought I'd removed this line in my PR but maybe I forgot to push that edit to github.)
Done. (I thought I'd put it back in to satisfy clang-format, but 🤷♂️ )
solvers/test/cost_test.cc
line 772 at r2 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
nit No need for format, as best I can tell.
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.
Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: LGTM missing from assignee xuchenhan-tri(platform) (waiting on @xuchenhan-tri)
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 6 files at r1, 5 of 6 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: complete! all discussions resolved, LGTM from assignees jwnimmer-tri(platform),xuchenhan-tri(platform)
Oops. I meant to remove the issue number from the commit message on merge. I should have left a Maybe at some point we should configure reviewable to help us remember. |
Most importantly, now AddCost(Expression) will succeed, even if it is passed in an Expression that is non-Polynomial.
Resolves #17897.
+@jwnimmer-tri for feature review, please?
This change is