Replace in-house quartic solver with 1010 algorithm#235
Conversation
|
@CodeRabbit review |
WalkthroughThis PR removes a pluggable Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 similar comment
✅ Actions performedReview triggered.
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #235 +/- ##
==========================================
+ Coverage 44.22% 44.84% +0.61%
==========================================
Files 127 141 +14
Lines 8554 9767 +1213
==========================================
+ Hits 3783 4380 +597
- Misses 4771 5387 +616
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pooltool/ptmath/roots/quartic.py (1)
12-30: Handle degenerate polynomials before calling the Numba solverThe new
_solverreturns[0, 0, 0, 0]when the leading quartic term is zero, which propagates throughget_real_positive_smallest_rootsas “no positive root”. Any legitimate solution of the remaining cubic/quadratic therefore gets discarded, and downstream collision code skips real events. The old pluggable solvers fell back to lower-degree formulas, so this is a regression.We can keep the fast path for true quartics while routing degenerate rows through
np.roots(or similar) before taking real-positive minima:def solve_quartics(ps: NDArray[np.float64]) -> NDArray[np.float64]: - """Returns the smallest positive and real root for each quartic polynomial. - - Args: - ps: - A mx5 array of polynomial coefficients, where m is the number of equations. - The columns are in the order a, b, c, d, e, where these coefficients make up - the quartic polynomial equation at^4 + bt^3 + ct^2 + dt + e = 0. - solver: - The method used to calculate the roots. See - pooltool.ptmath.roots.quartic.QuarticSolver. - - Returns: - roots: - An array of shape m. Each value is the smallest root that is real and - positive. If no such root exists (e.g. all roots have complex), then - `np.inf` is returned. - """ - return get_real_positive_smallest_roots(_solver(ps)) + """Returns the smallest positive and real root for each polynomial.""" + ps = np.asarray(ps, dtype=np.float64) + ps = np.atleast_2d(ps) + + leading_zero = np.isclose(ps[:, 0], 0.0) + if not leading_zero.any(): + return get_real_positive_smallest_roots(_solver(ps)) + + all_roots = np.full((ps.shape[0], 4), np.nan + 0j, dtype=np.complex128) + + if (~leading_zero).any(): + all_roots[~leading_zero] = _solver(ps[~leading_zero]) + + for idx in np.nonzero(leading_zero)[0]: + trimmed = np.trim_zeros(ps[idx], trim="f") + if trimmed.size <= 1: + continue # constant polynomial -> no finite roots + degenerates = np.roots(trimmed) + all_roots[idx, : degenerates.size] = degenerates + + return get_real_positive_smallest_roots(all_roots)This preserves the fast path while restoring correct behaviour when the quartic term cancels.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
pooltool/evolution/event_based/introspection.py(0 hunks)pooltool/evolution/event_based/simulate.py(7 hunks)pooltool/ptmath/roots/_quartic_numba.py(1 hunks)pooltool/ptmath/roots/quartic.py(2 hunks)tests/evolution/event_based/test_simulate.py(11 hunks)tests/ptmath/roots/README.md(1 hunks)tests/ptmath/roots/_generate_quartic_coeffs.py(1 hunks)tests/ptmath/roots/test_quartic.py(1 hunks)
💤 Files with no reviewable changes (1)
- pooltool/evolution/event_based/introspection.py
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
tests/ptmath/roots/README.md
19-19: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test (3.13, 1.8.4, windows-latest)
- GitHub Check: test (3.13, 1.8.4, ubuntu-latest)
- GitHub Check: test (3.10, 1.8.4, windows-latest)
- GitHub Check: test (3.10, 1.8.4, ubuntu-latest)
| if coeff[0] == 0.0: | ||
| return roots | ||
|
|
||
| a = coeff[1] / coeff[0] | ||
| b = coeff[2] / coeff[0] | ||
| c = coeff[3] / coeff[0] | ||
| d = coeff[4] / coeff[0] |
There was a problem hiding this comment.
Do not suppress lower-degree solutions when the quartic term vanishes
When coeff[0] == 0, solve immediately returns an array of zeros. Downstream, get_real_positive_smallest_roots treats that as “no positive root”, so any real solution of the remaining cubic/quadratic (e.g., grazing collisions where the t⁴ term cancels) gets lost and the simulation under-detects events. The legacy solvers handled this by falling back to lower-degree formulas; we need the same behaviour here instead of returning zeros.
| - `quartic_coeffs.npy` - Quartic polynomial coefficients (standard cases) | ||
| - `quartic_coeffcoeffs.roots.npy` - Ground truth roots for standard cases | ||
| - `hard_quartic_coeffs.npy` - Quartic polynomial coefficients (pathological cases) | ||
| - `hard_quartic_coeffs.roots.npy` - Ground truth roots for pathological cases | ||
| - `hard_quartic_coeffs.cases.npy` - Case labels indicating which pathological scenario each coefficient set represents |
There was a problem hiding this comment.
Correct the hard coefficients file name
The second bullet currently reads quartic_coeffcoeffs.roots.npy, but the generated artifact (and tests) use quartic_coeffs.roots.npy. Please fix the typo so the README matches the actual files.
-- `quartic_coeffcoeffs.roots.npy` - Ground truth roots for standard cases
+- `quartic_coeffs.roots.npy` - Ground truth roots for standard cases📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - `quartic_coeffs.npy` - Quartic polynomial coefficients (standard cases) | |
| - `quartic_coeffcoeffs.roots.npy` - Ground truth roots for standard cases | |
| - `hard_quartic_coeffs.npy` - Quartic polynomial coefficients (pathological cases) | |
| - `hard_quartic_coeffs.roots.npy` - Ground truth roots for pathological cases | |
| - `hard_quartic_coeffs.cases.npy` - Case labels indicating which pathological scenario each coefficient set represents | |
| - `quartic_coeffs.npy` - Quartic polynomial coefficients (standard cases) | |
| - `quartic_coeffs.roots.npy` - Ground truth roots for standard cases | |
| - `hard_quartic_coeffs.npy` - Quartic polynomial coefficients (pathological cases) | |
| - `hard_quartic_coeffs.roots.npy` - Ground truth roots for pathological cases | |
| - `hard_quartic_coeffs.cases.npy` - Case labels indicating which pathological scenario each coefficient set represents |
🤖 Prompt for AI Agents
In tests/ptmath/roots/README.md around lines 9 to 13, there is a typo in the
second bullet: it reads `quartic_coeffcoeffs.roots.npy` but should match the
actual artifact `quartic_coeffs.roots.npy`; update that bullet to
`quartic_coeffs.roots.npy` so the README matches the generated files and tests.
- Times are computed on-the-fly - This is around 30% faster and well, it just makes sense
1 participant
Improvement in accuracy
Figure: Relative error of roots
(abs(roots) - abs(true_roots)) / abs(true_roots)for 100,000 "difficult" coefficients.Legend:
PR = polynomial-roots
OQS = 1010-algoritm
pooltool = the old solver for pooltool
Numba = the new solver for pooltool
Improvement in speed
Old: 336.566 ms / 100000 solves
New: 241.622 ms / 100000 solves
Additional speed tests:
~/Software/pooltool_ml/pooltool/tests/ptmath/roots/data/README.md:Main takeaways here:
Summary by CodeRabbit
Refactor
quartic_solverparameter from simulation and event detection APIs; solver behavior is now internally unified.Tests