Foundation Classes - Refactor math_DirectPolynomialRoots#937
Foundation Classes - Refactor math_DirectPolynomialRoots#937dpasukhi merged 3 commits intoOpen-Cascade-SAS:IRfrom
Conversation
This commit introduces significant enhancements to the math_DirectPolynomialRoots class, focusing on robust numerical methods for finding real roots of polynomials up to degree 4. Key changes include: 1. **Refactoring and Modernization**: - Replaced legacy code with modern C++ practices, including constexpr and improved error handling. - Removed the outdated math_DirectPolynomialRoots.lxx file. 2. **Enhanced Algorithms**: - Implemented Ferrari's method for quartic equations and Cardano's formula for cubic equations, ensuring numerical stability. - Introduced Newton-Raphson refinement for improved root accuracy. 3. **Testing Improvements**: - Added comprehensive unit tests for various polynomial cases, ensuring correctness and robustness against edge cases. 4. **Code Cleanup**: - Translated comments to English and organized code for better readability. These changes aim to enhance the reliability and performance of polynomial root-finding operations within the library.
…actices This commit updates the math_DirectPolynomialRoots implementation by replacing traditional mathematical functions with their C++ standard library counterparts, enhancing code clarity and consistency. Key changes include: 1. **Standard Library Usage**: - Replaced instances of custom mathematical functions (e.g., Abs, Log, Sqrt) with std::abs, std::log, and std::sqrt for improved readability and adherence to modern C++ standards. 2. **Code Cleanup**: - Removed unnecessary includes and streamlined the code for better organization and maintainability. These modifications aim to improve the overall quality and maintainability of the polynomial root-finding algorithms.
dpasukhi
added
the
1. Foundation Classes
There was a problem hiding this comment.
Pull request overview
This PR modernizes the math_DirectPolynomialRoots class with improved numerical stability, C++17+ features, and comprehensive test coverage. The refactoring extracts algorithms into named helper functions, replaces deprecated OCCT math functions with std:: equivalents, and introduces aggressive Newton-Raphson refinement for better accuracy.
Key Changes:
- Refactored quartic/cubic solvers with helper functions in anonymous namespace
- Replaced OCCT math functions (
sqrt,abs,pow, etc.) withstd::equivalents - Merged inline implementations from
.lxxinto header - Added comprehensive test coverage including BUC60622 regression tests
- Improved documentation with Doxygen comments and algorithm references
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
math_DirectPolynomialRoots.cxx |
Core implementation with extracted helper functions, modern C++ idioms, and improved numerical stability |
math_DirectPolynomialRoots.hxx |
Header with comprehensive Doxygen docs, renamed member variables following OCCT conventions, merged inline implementations |
math_DirectPolynomialRoots.lxx |
Removed - inline implementations merged into header file |
FILES.cmake |
Updated to remove deleted .lxx file reference |
Geom2dGcc_Circ2d3Tan_Test.cxx |
Added test fixture with helper methods and comprehensive regression tests for BUC60622 |
math_DirectPolynomialRoots_Test.cxx |
Added test fixture, enhanced coverage with numerical stability tests |
bug28626_2 |
Updated test expectations for edge/vertex counts |
| if (std::abs(aQ0) < 10 * MACHINE_EPSILON) | ||
| { | ||
| if (Sol3.Value(Index) > Y0) | ||
| Y0 = Sol3.Value(Index); | ||
| aQ0 = 0.0; | ||
| } |
There was a problem hiding this comment.
The comment on lines 463-464 mentions 'fixes numerical precision issue on Linux/Windows vs macOS', but this is a critical algorithmic modification that deserves more detailed explanation. The comment should explain: (1) what the numerical issue is, (2) why 10*MACHINE_EPSILON is the appropriate threshold, and (3) what behavior difference was observed across platforms.
| // Evaluate polynomial: coefficients are in descending degree order (a0*x^n + a1*x^(n-1) + ... + | ||
| // an*x^0) |
There was a problem hiding this comment.
The comment describes polynomial evaluation in descending degree order, but the implementation loops from theCoeffs.size()-1 down to 0, which suggests ascending order evaluation (constant term first). The comment should accurately reflect the implementation: coefficients are in descending degree order but evaluation proceeds from constant term upward using Horner's method.
| // Evaluate polynomial: coefficients are in descending degree order (a0*x^n + a1*x^(n-1) + ... + | |
| // an*x^0) | |
| // Evaluate polynomial whose coefficients are given in descending degree order | |
| // (a0*x^n + a1*x^(n-1) + ... + an*x^0). The loop iterates from the constant term | |
| // upwards, accumulating powers of theRoot as aPower = theRoot^k. |
| A = theA / ScaleFactor; | ||
| B = theB / aScaleFactor2; | ||
| C = theC / (aScaleFactor2 * ScaleFactor); | ||
| D = theD; // Not used for cubic, but kept for consistency |
There was a problem hiding this comment.
This comment indicates field D is not used for cubic scaling but is assigned anyway 'for consistency'. This is misleading and wasteful. Either remove the assignment entirely, or if there's a specific reason to keep it (e.g., debugging), explain that reason clearly in the comment.
| D = theD; // Not used for cubic, but kept for consistency | |
| // D is not used for cubic equations. |
| // With very small leading coefficient, we get one small positive root near 2 and one very large | ||
| // negative root |
There was a problem hiding this comment.
The comment states we get 'one small positive root near 2', but 2.0 is not a particularly small value. The comment should clarify that it means 'one root with a small magnitude near the expected value of 2' or simply 'one root near 2' to avoid confusion.
| // With very small leading coefficient, we get one small positive root near 2 and one very large | |
| // negative root | |
| // With very small leading coefficient, we get one root near 2 and one very large negative root |
| //! Returns the value of the Nth root in default ordering. | ||
| //! The default ordering may vary depending on the algorithm used. |
There was a problem hiding this comment.
The documentation states that 'default ordering may vary depending on the algorithm used', but the test cases show specific expected orderings for each polynomial type. The documentation should either specify the actual ordering guarantees for each polynomial degree, or if ordering is truly non-deterministic, the tests should not rely on specific orderings.
| //! Returns the value of the Nth root in default ordering. | |
| //! The default ordering may vary depending on the algorithm used. | |
| //! Returns the value of the Nth root in deterministic ordering. | |
| //! Roots are always returned sorted in non-decreasing order of their | |
| //! real value, and multiple roots appear with their multiplicity. |
github-project-automation
bot
moved this to Done
in Client Service – Public Commercial Requests
Modernized and refactored the polynomial root-finding implementation
with improved numerical stability and modern C++ practices.
Key changes:
Implementation refactoring (math_DirectPolynomialRoots.cxx):
(std::abs, std::sqrt, std::pow, std::log, std::cos, std::sin, std::atan, std::max)
Header modernization (math_DirectPolynomialRoots.hxx):
Test improvements:
Minor fixes: