Implements symbolic::Polynomial::Roots() #20161
Conversation
There was a problem hiding this comment.
Reviewed 4 of 6 files at r1.
Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes (waiting on @RussTedrake)
common/symbolic/polynomial.h line 349 at r1 (raw file):
/// Returns the roots of a _univariate_ polynomial with constant coefficients /// as a column vector. ///
nit. Maybe add a note that we don't guarantee a specific ordering of the roots in the returned value?
There was a problem hiding this comment.
Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes (waiting on @RussTedrake)
common/symbolic/polynomial.h line 349 at r1 (raw file):
Previously, hongkai-dai (Hongkai Dai) wrote…
nit. Maybe add a note that we don't guarantee a specific ordering of the roots in the returned value?
Done.
There was a problem hiding this comment.
Reviewed 3 of 3 files at r2.
Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes (waiting on @RussTedrake)
jwnimmer-tri
added
the
release notes: feature
There was a problem hiding this comment.
See RussTedrake#55 for some fixes.
Reviewed 3 of 6 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: 3 unresolved discussions
a discussion (no related file):
BTW The macOS CI failure is real (tolerance failure). Be sure macOS is happy before merging this PR.
common/symbolic/polynomial.cc line 1061 at r2 (raw file):
throw std::runtime_error(fmt::format( "{} is not a univariate polynomial; it has indeterminates {}.", ToExpression().to_string(), indeterminates().to_string()));
nit The formatting has a lot more syntax than necessary. See my PR.
common/symbolic/test/polynomial_test.cc line 1515 at r2 (raw file):
// Note: the order of the roots is not guaranteed. Sort them here by real, // then imaginary. auto sort = [](const Eigen::VectorXcd& v) {
nit Verb vs adjective confusion. (Fixed in my PR.)
Suggestion:
sorted
There was a problem hiding this comment.
Reviewable status:
complete! all discussions resolved, LGTM from assignees jwnimmer-tri(platform),hongkai-dai
a discussion (no related file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
BTW The macOS CI failure is real (tolerance failure). Be sure macOS is happy before merging this PR.
Done.
For finding the roots of univariate polynomials. Towards RobotLocomotion#20159.
There was a problem hiding this comment.
Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status:
For finding the roots of univariate polynomials.
Towards #20159.
+@hongkai-dai for feature review, please.
This change is