-
Notifications
You must be signed in to change notification settings - Fork 45
Add Ferrari-Cardano algorithm and general Quartic Solver test suite #2010
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
base: develop
Are you sure you want to change the base?
Conversation
… required higher precision than tolerance allowed
…fixed backwards iterator, fixed a couple of arithmetic bugs
…ss an implementation of that
Test summary 5 750 files 9 236 suites 18m 11s ⏱️ Results for commit 58bf4c5. ♻️ This comment has been updated with latest results. |
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.
Pull Request Overview
This pull request adds a Ferrari-Cardano method implementation for solving quartic polynomial equations and a comprehensive test suite. The implementation is designed to find positive, real, non-zero roots for quartic functions used in geometric surface calculations.
Key changes include:
- Implementation of the Ferrari-Cardano algorithm in
FerrariSolverclass - Comprehensive test framework for quartic solvers with abstract base class design
- Addition of CMake preset configurations for development workflows
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
src/orange/surf/detail/FerrariSolver.hh |
Core implementation of Ferrari-Cardano quartic solver with cubic and quadratic utility functions |
test/orange/surf/detail/QuarticSolver.test.cc |
Abstract test framework and specific Ferrari solver tests covering various root scenarios |
test/orange/CMakeLists.txt |
Registration of new quartic solver test |
scripts/cmake-presets/.json |
CMake preset configurations for development builds |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2010 +/- ##
===========================================
+ Coverage 84.89% 84.92% +0.02%
===========================================
Files 1273 1274 +1
Lines 44667 44787 +120
Branches 16765 16638 -127
===========================================
+ Hits 37922 38035 +113
+ Misses 4933 4763 -170
- Partials 1812 1989 +177
🚀 New features to boost your workflow:
|
|
For posterity, @osanstrong and I decided that it's better to switch to using the Numerical Recipes cubic solver because 1) it's more citable, 2) it's whats used with Algorithm 1010, 3) it's pretty similar to what we originally had, so it's not a big change. Owen has already updated this MR accordingly. @osanstrong your new citation key is |
…urface-start calculations
…g as precision robustness test
…ested out-of-scope degenerate cases (multiple roots on surface, degenerate torii)
elliottbiondo
left a comment
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 think this is basically ready to merge: just a few minor comments
sethrj
left a comment
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.
Looks great Owen! Just a few final comments based on the changes.
…ly use the Cardano method as its cubic rootfinder
…nd made normal operator call the surface operator when surface-start is specified
…ll be that surface solve will simply call four coefficient operator
|
@elliottbiondo I think this is ready to merge! |
|
@osanstrong @elliottbiondo Since we've made the solver more generic, I've moved it to its rightful place in |
This reverts commit 303a17b.
|
@elliottbiondo @osanstrong Please take a quick look at my changes and then we can merge: |
elliottbiondo
left a comment
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.
These changes look good to me. Let's wait for @osanstrong to take a look before merging.
This pull request adds FerrariSolver, a class implementing the Ferrari-Cardano method to solve quartic polynomials of the form
ax⁴ + bx³ + cx² + dx + e = 0, and QuarticSolver.test, a class with tests for quartic solvers.The Ferrari-Cardano method solves a related cubic equation to find a factor which breaks the quartic equation into two quadratic equations, which are then solved for the final roots.
The QuarticSolver test file contains a brief abstract framework for testing quartic functions, and a specific subclass FerrariSolverTest which plugs in FerrariSolver for those tests. Most of the tests can then be easily reused to test an alternative quartic solver, with a different subclass.