Foundation Classes - Enhance MathRoot and MathSys with new utilities#954
Foundation Classes - Enhance MathRoot and MathSys with new utilities#954dpasukhi merged 3 commits intoOpen-Cascade-SAS:IRfrom
Conversation
dpasukhi
commented
Dec 25, 2025
dpasukhi
commented
- Added MathRoot_Trig_Test for comprehensive testing of trigonometric root finding.
- Introduced MathSys_Newton2D for optimized 2D Newton-Raphson solver, including bounds checking and gradient descent fallback.
- Updated FILES.cmake to include new test and utility files.
- Added MathUtils_Domain for parameter domain management in curves and surfaces, enhancing domain analysis capabilities.
- Added MathRoot_Trig_Test for comprehensive testing of trigonometric root finding. - Introduced MathSys_Newton2D for optimized 2D Newton-Raphson solver, including bounds checking and gradient descent fallback. - Updated FILES.cmake to include new test and utility files. - Added MathUtils_Domain for parameter domain management in curves and surfaces, enhancing domain analysis capabilities.
dpasukhi
added
2. Bug
There was a problem hiding this comment.
Pull request overview
This PR enhances the Foundation Classes math libraries by introducing new utilities for root finding and domain management. The additions include comprehensive test coverage for trigonometric equation solving, an optimized 2D Newton-Raphson solver for common geometric problems, and lightweight domain management types for parameter bounds handling.
Key Changes:
- Added comprehensive testing infrastructure for trigonometric root finding with multiple edge cases
- Introduced optimized 2D Newton solver with gradient descent fallback and bounds checking
- Created domain management utilities for 1D/2D parameter spaces
- Fixed critical bugs in trigonometric root finding algorithm
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| MathUtils_Domain.hxx | New lightweight domain types (Domain1D/Domain2D) for parameter space management |
| MathUtils/FILES.cmake | Added MathUtils_Domain.hxx to build configuration |
| MathSys/README.md | Documentation for new Newton2D solver capabilities and usage |
| MathSys_Newton2D.hxx | Optimized 2D Newton-Raphson solver with Cramer's rule and symmetric Jacobian variant |
| MathSys/FILES.cmake | Added MathSys_Newton2D.hxx to build configuration |
| MathRoot_Trig.hxx | Fixed quadratic coefficient ordering and improved root refinement with Halley's method |
| MathRoot_Trig_Test.cxx | Comprehensive test suite covering linear, quadratic, general cases, and edge cases |
| GTests/FILES.cmake | Added MathRoot_Trig_Test.cxx to test build configuration |
| { | ||
| // c*cos(x) + d*sin(x) + e = 0 | ||
| // Using t = tan(x/2): (e-c) + 2dt + (e+c)t^2 = 0 | ||
| // Using t = tan(x/2): (e-c)*t^2 + 2d*t + (e+c) = 0 |
There was a problem hiding this comment.
The comment incorrectly describes the quadratic equation coefficients. The equation should be (e-c)t² + 2dt + (e+c) = 0, but the code correctly implements it as at² + bt + c where a=(e-c), b=2d, c=(e+c). The comment's notation '(e-c)*t^2' is misleading as it doesn't match the standard quadratic form ax² + bx + c = 0 that MathPoly::Quadratic expects.
| // Using t = tan(x/2): (e-c)*t^2 + 2d*t + (e+c) = 0 | |
| // Using t = tan(x/2): (e - c)*t^2 + 2*d*t + (e + c) = 0, | |
| // i.e. a quadratic a*t^2 + b*t + c = 0 with a = (e - c), b = 2*d, c = (e + c). |
| constexpr int THE_MAX_ITER = 20; | ||
| constexpr double THE_TOL = 1.0e-14; |
There was a problem hiding this comment.
The constants THE_MAX_ITER and THE_TOL use global-style naming (all caps with THE_ prefix) but are local constexpr variables within a lambda. According to OCCT conventions, local variables should use aVariableName style. These should be renamed to aMaxIter and aTol respectively.
| constexpr double THE_MAX_STEP = 0.5; | ||
| if (std::abs(aDelta) > THE_MAX_STEP) | ||
| { | ||
| aDelta = (aDelta > 0) ? THE_MAX_STEP : -THE_MAX_STEP; |
There was a problem hiding this comment.
The constant THE_MAX_STEP uses global-style naming but is a local constexpr variable. According to OCCT conventions, it should be renamed to aMaxStep to follow local variable naming patterns.
| constexpr double THE_MAX_STEP = 0.5; | |
| if (std::abs(aDelta) > THE_MAX_STEP) | |
| { | |
| aDelta = (aDelta > 0) ? THE_MAX_STEP : -THE_MAX_STEP; | |
| constexpr double aMaxStep = 0.5; | |
| if (std::abs(aDelta) > aMaxStep) | |
| { | |
| aDelta = (aDelta > 0) ? aMaxStep : -aMaxStep; |
…t notation for cosine values (± to +/-) and update mathematical expressions for clarity (cos² to cos^2).
