Consolidate quadratic solving#296
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR migrates the quadratic root solver from a dual real-only/complex interface to a single unified complex-capable solver, and updates all physics collision and motion code to extract real parts and filter NaN values from the new complex return type. ChangesQuadratic Solver Complex Domain Migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #296 +/- ##
==========================================
+ Coverage 47.41% 47.45% +0.03%
==========================================
Files 153 153
Lines 10509 10493 -16
==========================================
- Hits 4983 4979 -4
+ Misses 5526 5514 -12
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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pooltool/physics/utils.py`:
- Around line 63-64: The code takes the .real part of roots from quadratic.solve
and can return a non-physical touchdown time when roots are complex; instead,
after calling quadratic.solve (variables t1, t2), check whether the roots are
real (e.g., test abs(t1.imag) and abs(t2.imag) against a tiny tolerance or use
numpy.isreal); if both are real return max(t1.real, t2.real) as before,
otherwise preserve the prior “no real root” behavior by returning None (or the
module’s sentinel for “no touchdown”) rather than coercing complex values to
real.
In `@tests/evolution/event_based/test_simulate.py`:
- Around line 372-374: The loop that uses quadratic.solve to pick a
collision_time currently reads t.real without ensuring the root is effectively
real; modify the loop that iterates over quadratic.solve(...) and only consider
roots whose imaginary part is negligible (e.g., abs(t.imag) < a small tolerance
like 1e-9) before using t.real, and ignore complex roots with significant
imaginary components so collision_time is set only from physically meaningful
real roots.
In `@tests/ptmath/roots/test_quadratic.py`:
- Line 69: The test currently attempts to sort complex roots with "solutions =
sorted([u1, u2])" which raises TypeError; update this to use a deterministic key
that compares real then imaginary parts like the other tests (e.g., sorted([u1,
u2], key=lambda z: (z.real, z.imag))) so the complex NDArray values returned by
solve() can be ordered; modify the line referencing u1 and u2 accordingly to
match existing tests' sorting approach.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e923af71-8c2b-4f08-893a-40faec28c561
📒 Files selected for processing (7)
pooltool/physics/motion/solve.pypooltool/physics/resolve/ball_ball/core.pypooltool/physics/resolve/ball_cushion/core.pypooltool/physics/utils.pypooltool/ptmath/roots/quadratic.pytests/evolution/event_based/test_simulate.pytests/ptmath/roots/test_quadratic.py
| t1, t2 = quadratic.solve(-0.5 * g, rvw[1, 2], rvw[0, 2] - R) | ||
|
|
||
| if math.isnan(t1): | ||
| return np.inf | ||
|
|
||
| return max(t1, t2) | ||
| return max(t1.real, t2.real) |
There was a problem hiding this comment.
Preserve no-real-root handling for airborne touchdown time.
Line 64 currently returns a finite time from complex roots by taking .real, which can schedule a non-physical touchdown when there is no real intersection.
Proposed fix
- t1, t2 = quadratic.solve(-0.5 * g, rvw[1, 2], rvw[0, 2] - R)
- return max(t1.real, t2.real)
+ t1, t2 = quadratic.solve(-0.5 * g, rvw[1, 2], rvw[0, 2] - R)
+ t1_is_real = np.abs(t1.imag) <= const.EPS
+ t2_is_real = np.abs(t2.imag) <= const.EPS
+
+ if not t1_is_real and not t2_is_real:
+ return np.inf
+
+ r1 = t1.real if t1_is_real else -np.inf
+ r2 = t2.real if t2_is_real else -np.inf
+ return max(r1, r2)📝 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.
| t1, t2 = quadratic.solve(-0.5 * g, rvw[1, 2], rvw[0, 2] - R) | |
| if math.isnan(t1): | |
| return np.inf | |
| return max(t1, t2) | |
| return max(t1.real, t2.real) | |
| t1, t2 = quadratic.solve(-0.5 * g, rvw[1, 2], rvw[0, 2] - R) | |
| t1_is_real = np.abs(t1.imag) <= const.EPS | |
| t2_is_real = np.abs(t2.imag) <= const.EPS | |
| if not t1_is_real and not t2_is_real: | |
| return np.inf | |
| r1 = t1.real if t1_is_real else -np.inf | |
| r2 = t2.real if t2_is_real else -np.inf | |
| return max(r1, r2) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pooltool/physics/utils.py` around lines 63 - 64, The code takes the .real
part of roots from quadratic.solve and can return a non-physical touchdown time
when roots are complex; instead, after calling quadratic.solve (variables t1,
t2), check whether the roots are real (e.g., test abs(t1.imag) and abs(t2.imag)
against a tiny tolerance or use numpy.isreal); if both are real return
max(t1.real, t2.real) as before, otherwise preserve the prior “no real root”
behavior by returning None (or the module’s sentinel for “no touchdown”) rather
than coercing complex values to real.
| for t in quadratic.solve(0.5 * mu_r * g, -V0, eps): | ||
| if t >= 0 and t < collision_time: | ||
| collision_time = t | ||
| if t.real >= 0 and t.real < collision_time: | ||
| collision_time = t.real |
There was a problem hiding this comment.
Validate that roots are real before using their real component.
The code extracts t.real without verifying that the imaginary part is negligible. For a collision-time calculation, only real-valued roots are physically meaningful. If the solver returns a root with a significant imaginary component (e.g., 2.0 + 0.1j), the current code would incorrectly use 2.0 as a valid collision time.
Since this helper computes the ground-truth collision time for test validation, it should be as strict as production code about root validity.
🛡️ Proposed fix to filter complex roots
collision_time = np.inf
for t in quadratic.solve(0.5 * mu_r * g, -V0, eps):
- if t.real >= 0 and t.real < collision_time:
+ if not np.isnan(t) and np.isclose(t.imag, 0.0, atol=1e-9) and t.real >= 0 and t.real < collision_time:
collision_time = t.real
return collision_timeAlternatively, to match the pattern in other physics modules, check the imaginary part separately:
collision_time = np.inf
for t in quadratic.solve(0.5 * mu_r * g, -V0, eps):
- if t.real >= 0 and t.real < collision_time:
+ if np.isnan(t) or not np.isclose(t.imag, 0.0, atol=1e-9):
+ continue
+ if t.real >= 0 and t.real < collision_time:
collision_time = t.real
return collision_time📝 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.
| for t in quadratic.solve(0.5 * mu_r * g, -V0, eps): | |
| if t >= 0 and t < collision_time: | |
| collision_time = t | |
| if t.real >= 0 and t.real < collision_time: | |
| collision_time = t.real | |
| for t in quadratic.solve(0.5 * mu_r * g, -V0, eps): | |
| if not np.isnan(t) and np.isclose(t.imag, 0.0, atol=1e-9) and t.real >= 0 and t.real < collision_time: | |
| collision_time = t.real |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/evolution/event_based/test_simulate.py` around lines 372 - 374, The
loop that uses quadratic.solve to pick a collision_time currently reads t.real
without ensuring the root is effectively real; modify the loop that iterates
over quadratic.solve(...) and only consider roots whose imaginary part is
negligible (e.g., abs(t.imag) < a small tolerance like 1e-9) before using
t.real, and ignore complex roots with significant imaginary components so
collision_time is set only from physically meaningful real roots.
| # This should give one very large and one very small solution. | ||
| a, b, c = 1.0, -1e7, 1.0 | ||
| u1, u2 = solve(a, b, c) | ||
| solutions = sorted([u1, u2]) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Run the specific test to confirm it fails with TypeError
pytest tests/ptmath/roots/test_quadratic.py::test_solve_large_values -vRepository: ekiefl/pooltool
Length of output: 106
🏁 Script executed:
cat -n tests/ptmath/roots/test_quadratic.py | sed -n '60,80p'Repository: ekiefl/pooltool
Length of output: 897
🏁 Script executed:
# Check if file exists and get basic info
wc -l tests/ptmath/roots/test_quadratic.pyRepository: ekiefl/pooltool
Length of output: 100
🏁 Script executed:
# Search for similar uses of sorted() in the test file to understand patterns
rg "sorted\(" tests/ptmath/roots/test_quadratic.py -B 2 -A 2Repository: ekiefl/pooltool
Length of output: 1251
🏁 Script executed:
# Check the quadratic solver implementation to understand what types it returns
fd test_quadratic.pyRepository: ekiefl/pooltool
Length of output: 96
🏁 Script executed:
# Find and examine the solve() function implementation
fd -t f "*.py" | xargs rg "def solve" | grep -i quadraticRepository: ekiefl/pooltool
Length of output: 463
🏁 Script executed:
# Check the imports and module structure
head -30 tests/ptmath/roots/test_quadratic.pyRepository: ekiefl/pooltool
Length of output: 946
🏁 Script executed:
# Verify Python's behavior with sorting complex numbers
python3 << 'EOF'
try:
result = sorted([1+2j, 3+4j])
print("Success:", result)
except TypeError as e:
print("TypeError:", e)
EOFRepository: ekiefl/pooltool
Length of output: 133
Complex numbers cannot be sorted without a key function.
Line 69 attempts to sort complex numbers directly, which will raise a TypeError at runtime because complex numbers do not support comparison operators (<, >). The solve() function returns NDArray[np.complex128], making both u1 and u2 complex128 values. This is inconsistent with all other tests in the file, which use sorted([u1, u2], key=lambda z: (z.real, z.imag)).
🐛 Proposed fix
- solutions = sorted([u1, u2])
+ solutions = sorted([u1, u2], key=lambda z: (z.real, z.imag))📝 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.
| solutions = sorted([u1, u2]) | |
| solutions = sorted([u1, u2], key=lambda z: (z.real, z.imag)) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/ptmath/roots/test_quadratic.py` at line 69, The test currently attempts
to sort complex roots with "solutions = sorted([u1, u2])" which raises
TypeError; update this to use a deterministic key that compares real then
imaginary parts like the other tests (e.g., sorted([u1, u2], key=lambda z:
(z.real, z.imag))) so the complex NDArray values returned by solve() can be
ordered; modify the line referencing u1 and u2 accordingly to match existing
tests' sorting approach.
1 participant
Consolidate quadratic solvers
Summary
pooltool/ptmath/roots/quadratic.pypreviously carried two solvers:solve(a, b, c) -> tuple[float, float]— simple Cardano formula. Real-valued only; returns(nan, nan)for negative discriminant.solve_complex(a, b, c) -> NDArray[np.complex128]— robust complex-valued implementation with a sign-trick and product-identity fallback to avoid catastrophic cancellation.The
3dbranch already collapsed these into a singlesolvethat does what main'ssolve_complexdoes. Main carried a# TODOcomment for this exact migration. This PR completes it.After this PR: one solver, named
solve, returning a length-2 complex array. The five legacy callers and one test fixture are migrated.What's in this PR
Solver consolidation (
pooltool/ptmath/roots/quadratic.py):solve.solve_complex→solve.# TODO: In the branch 3d, ... this function has replaced solvecomment.Caller migrations (5 files):
pooltool/physics/utils.py::get_airborne_time—math.isnan→np.isnan; explicit.imagcheck;max(t1.real, t2.real)for the descending-leg root.import mathdropped.pooltool/physics/motion/solve.py::ball_linear_cushion_collision_time—from math import acos, isnan→from math import acos;isnan(root)→np.isnan(root); passroot.real(not the complexroot) intoevolve_ball_motion.pooltool/physics/resolve/ball_ball/core.py—quadratic.solve_complex(...)→quadratic.solve(...).pooltool/physics/resolve/ball_cushion/core.py— same.tests/evolution/event_based/test_simulate.py::true_time_to_collision— skip nan/complex roots, take.real.Tests added (
tests/ptmath/roots/test_quadratic.py):Why this matters (and the risk that was managed)
The new solver is more numerically robust than the old. For typical inputs the difference is below machine epsilon, but in catastrophic-cancellation cases (one root much smaller than the other) the new solver can produce roots that differ in the last 5–7 digits from the old one. Cushion-collision detection (
ball_linear_cushion_collision_time) is in the hot path for every shot, and event-detection results are sensitive to root precision — small shifts can cascade into different event ordering and different shot trajectories.What all tests passing proves and doesn't prove
test_case1,test_case2,test_case3,test_grazing_ball_ball_collision,test_high_speed_grazing_collisions,test_newtons_cradle_*, layouts, etc.) all produce event times within their existingpytest.approxtolerances after migration.Summary by CodeRabbit
Release Notes
Bug Fixes
Tests