-
Notifications
You must be signed in to change notification settings - Fork 81
Consolidate quadratic solving #296
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -370,8 +370,8 @@ def true_time_to_collision(eps, V0, mu_r, g): | |||||||||||||||||
| """ | ||||||||||||||||||
| collision_time = np.inf | ||||||||||||||||||
| 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 | ||||||||||||||||||
|
Comment on lines
372
to
+374
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Validate that roots are real before using their real component. The code extracts 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
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||
| return collision_time | ||||||||||||||||||
|
|
||||||||||||||||||
| V0 = 2 | ||||||||||||||||||
|
|
||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,103 @@ | ||
| import numpy as np | ||
| import pytest | ||
|
|
||
| from pooltool.ptmath.roots.quadratic import solve | ||
|
|
||
|
|
||
| def test_solve_standard_quadratic(): | ||
| # x^2 - 5x + 6 = 0 | ||
| # Solutions: x = 2, x = 3 | ||
| u1, u2 = solve(1.0, -5.0, 6.0) | ||
| solutions = sorted([u1, u2], key=lambda z: (z.real, z.imag)) | ||
| # First root -> 2.0 + 0.0j | ||
| assert solutions[0].real == 2.0 | ||
| assert solutions[0].imag == 0.0 | ||
| # Second root -> 3.0 + 0.0j | ||
| assert solutions[1].real == 3.0 | ||
| assert solutions[1].imag == 0.0 | ||
|
|
||
| # x^2 - x - 2 = 0 | ||
| # Solutions: x = -1, x = 2 | ||
| u1, u2 = solve(1.0, -1.0, -2.0) | ||
| solutions = sorted([u1, u2], key=lambda z: (z.real, z.imag)) | ||
| # First root -> -1.0 + 0.0j | ||
| assert solutions[0].real == -1.0 | ||
| assert solutions[0].imag == 0.0 | ||
| # Second root -> 2.0 + 0.0j | ||
| assert solutions[1].real == 2.0 | ||
| assert solutions[1].imag == 0.0 | ||
|
|
||
| # Perfect square: x^2 - 4x + 4 = 0 | ||
| # Single repeated solution: x = 2 | ||
| u1, u2 = solve(1.0, -4.0, 4.0) | ||
| solutions = sorted([u1, u2], key=lambda z: (z.real, z.imag)) | ||
| # Both roots -> 2.0 + 0.0j | ||
| for root in solutions: | ||
| assert root.real == 2.0 | ||
| assert root.imag == 0.0 | ||
|
|
||
| # Difference of squares: x^2 - 4 = 0 | ||
| # Solutions: x = -2, x = 2 | ||
| u1, u2 = solve(1.0, 0.0, -4.0) | ||
| solutions = sorted([u1, u2], key=lambda z: (z.real, z.imag)) | ||
| # First root -> -2.0 + 0.0j | ||
| assert solutions[0].real == -2.0 | ||
| assert solutions[0].imag == 0.0 | ||
| # Second root -> 2.0 + 0.0j | ||
| assert solutions[1].real == 2.0 | ||
| assert solutions[1].imag == 0.0 | ||
|
|
||
| # Complex roots: x^2 + 1 = 0 | ||
| # Solutions: x = i, x = -i | ||
| u1, u2 = solve(1.0, 0.0, 1.0) | ||
| solutions = sorted([u1, u2], key=lambda z: (z.real, z.imag)) | ||
| # First root -> -i -> (0.0, -1.0) | ||
| assert solutions[0].real == 0.0 | ||
| assert solutions[0].imag == -1.0 | ||
| # Second root -> i -> (0.0, 1.0) | ||
| assert solutions[1].real == 0.0 | ||
| assert solutions[1].imag == 1.0 | ||
|
|
||
|
|
||
| def test_solve_large_values(): | ||
| """Test large coefficients for numerical stability.""" | ||
|
|
||
| # Equation: x^2 - 1e7*x + 1 = 0 | ||
| # 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], key=lambda z: (z.real, z.imag)) | ||
|
|
||
| # The large root should be close to 1e7, the smaller should be close to 1e-7. We're | ||
| # able to use a very small relative tolerance due to the way the solver avoids | ||
| # catastrophic cancellation. | ||
| assert pytest.approx(solutions[0].real, rel=1e-12) == 1e-7 | ||
| assert pytest.approx(solutions[1].real, rel=1e-12) == 1e7 | ||
|
|
||
| assert solutions[0].imag == 0.0 | ||
| assert solutions[1].imag == 0.0 | ||
|
|
||
|
|
||
| def test_solve_linear_equation(): | ||
| # a=0, b≠0 => linear equation b*t + c = 0 => t=-c/b | ||
| # e.g. 2t + 4 = 0 => t=-2 | ||
| r1, r2 = solve(0.0, 2.0, 4.0) | ||
| assert r1.real == -2.0 | ||
| assert r1.imag == 0.0 | ||
| assert np.isnan(r2) | ||
|
|
||
|
|
||
| def test_solve_degenerate_no_solution(): | ||
| # a=0, b=0, c≠0 => no solutions | ||
| # e.g. 0*t^2 + 0*t + 5 = 0 => no real solution | ||
| r1, r2 = solve(0.0, 0.0, 5.0) | ||
| assert np.isnan(r1) | ||
| assert np.isnan(r2) | ||
|
|
||
|
|
||
| def test_solve_degenerate_infinite_solutions(): | ||
| # a=0, b=0, c=0 => infinite solutions | ||
| # e.g. 0*t^2 + 0*t + 0 = 0 => t can be anything | ||
| r1, r2 = solve(0.0, 0.0, 0.0) | ||
| assert np.isnan(r1) | ||
| assert np.isnan(r2) |
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.
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
📝 Committable suggestion
🤖 Prompt for AI Agents