Repository navigation
Polynomial roots - #1452
Polynomial roots#1452
Conversation
There was a problem hiding this comment.
I have some questions and remarks, so, per void check_residuals in test/polynomial_roots_test.cpp:
- This could be
const auto&forfor(auto root : roots).
per void fft in polynomial_roots.hpp:
- This could benefit of some comments, and more testing as well due to the nested loops.
|
Just wondering naming, since there are so many algorithms for finding polynomial roots; you wouldn't call it |
|
@jeremy-murphy : I believe this algorithm finishes polynomial rootfinding as it is nlogn and stable. |
Cool, that's pretty exciting! Do you have any results on how accurate it is compared to the existing algorithms we have or against some optimal answer? |
Not yet-I think Aurentz's 2015 work (which is O(n^2)) is probably the reference for stability. I do not believe this algorithm is stable in quite the same sense. |
| assert(std::count(zero_roots.begin(), zero_roots.end(), std::complex<double>(0, 0)) == 2); | ||
|
|
||
| std::mt19937_64 generator(0x5eed); | ||
| std::uniform_real_distribution<double> distribution(-2, 2); |
There was a problem hiding this comment.
Have you tested how accurate this method is with multiprecision types like boost::multiprecision::cpp_bin_float?
| using std::abs; | ||
| using std::log2; |
There was a problem hiding this comment.
Would adding BOOST_MATH_STD_USING here and below allow multiprecision types?
There was a problem hiding this comment.
@JacobHass8 : BOOST_MATH_STD_USING just brings in all math functions. using std::abs allows ADL of abs.
| try { polynomial_roots(std::vector<double>{0, 0}); } | ||
| catch(std::domain_error const&) { threw = true; } | ||
| assert(threw); |
There was a problem hiding this comment.
Should all tests here use boost test functions like BOOST_CHECK_THROW? Additionally, is there any benefit in adding policies to polynomial_roots?
There was a problem hiding this comment.
Not sure why BOOST_CHECK_THROW would matter here, what's your rationale for it?
There was a problem hiding this comment.
@JacobHass8 : I eliminated Boost.Test from the library because it takes forever to compile and its error messages are not actionable. We'll use math_unit_test.hpp throughout.
Looks like it needs a CHECK_THROW macro though.
There was a problem hiding this comment.
math_unit_test.hpp does have CHECK_THROW now; the test uses it for the zero polynomial and non-finite coefficients.
424a4ca to
553e1b9
Compare
553e1b9 to
6854237
Compare
7e12e54 to
ef22186
Compare
6854237 to
e03a9c7
Compare
bcb2e29 to
31fbee6
Compare
e03a9c7 to
f96a5cc
Compare
f96a5cc to
d93034b
Compare
d93034b to
29df02a
Compare
f374f17 to
79f1c61
Compare
29df02a to
21f5308
Compare
c3754ee to
4981897
Compare
d6da86c to
2b9c7bc
Compare
|
|
||
| `` | ||
| #include <boost/math/tools/polynomial_roots.hpp> | ||
| #include <boost/math/tools/amvw_roots.hpp> |
There was a problem hiding this comment.
@claude : I don't think amvw_roots will be scrutable to anyone. We can also do multiple implementations in a single file and dispatch to a detail, rather than have multiple files.
There was a problem hiding this comment.
Done: both are in polynomial_roots.hpp now (amvw_roots.hpp is gone). The public functions are polynomial_roots(c) and polynomial_roots_companion_matrix(c), each a thin wrapper over polynomial_roots_detail::piecewise / polynomial_roots_detail::companion_qr.
| `` | ||
|
|
||
| template <class Real> | ||
| std::vector<std::complex<Real> > |
There was a problem hiding this comment.
These should be templated on a RandomAccessContainer, and the return type deduced.
It should work with std::array, std::span, and std::vector.
There was a problem hiding this comment.
Done: both take RandomAccessContainer const& (anything with value_type, begin and end) and return auto, a std::vector of std::complex<Real> for built-in types or the matching Boost.Multiprecision complex type. The test covers std::vector, std::array, and std::span under C++20.
| template <class Real> | ||
| std::vector<std::complex<Real> > | ||
| polynomial_roots(std::vector<Real> coefficients, | ||
| unsigned precision = std::numeric_limits<Real>::digits); |
There was a problem hiding this comment.
Precision is not a useful parameter here-how would a user set it?
There was a problem hiding this comment.
Agreed, removed. The local expansion order is now fixed by std::numeric_limits<Real>::digits, which is what the default was.
9f2e824 to
ea3689a
Compare
polynomial_roots: the Imbach-Moroz piecewise root finder (arXiv:2302.06244), whose sector DFTs use tools::fft; polynomial_roots_companion_matrix: the Aurentz-Mach-Vandebril-Watkins companion QR, O(n^2) and normwise backward stable. Both take any random access container of built-in or Boost.Multiprecision reals, with Python bindings. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ea3689a to
fadcc22
Compare
polynomial_roots: the Imbach-Moroz piecewise root finder (arXiv:2302.06244), whose sector DFTs use tools::fft; polynomial_roots_companion_matrix: the Aurentz-Mach-Vandebril-Watkins companion QR, O(n^2) and normwise backward stable. Both take any random access container of built-in or Boost.Multiprecision reals, with Python bindings.
Max root error / (κ ε), κ the coefficientwise condition number:
polynomial_rootspolynomial_roots_companion_matrix🤖 Generated with Claude Code