Solver migration#327
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:
WalkthroughMoves collision-time solvers from ChangesSolver Relocation and API Consolidation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 #327 +/- ##
==========================================
+ Coverage 47.71% 47.78% +0.06%
==========================================
Files 158 157 -1
Lines 10700 10638 -62
==========================================
- Hits 5106 5083 -23
+ Misses 5594 5555 -39
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: 1
🧹 Nitpick comments (3)
pooltool/evolution/event_based/detect/ball_ball.py (2)
37-38: 💤 Low valueInconsistent numeric literal types between branches.
Line 38 uses integer literals (
0, 0, 0, 0) while line 55 uses float literals (0.0, 0.0, 0.0, 0.0). For Numba nopython mode, consistent typing avoids potential type-inference surprises.Proposed fix
if s1 == const.spinning or s1 == const.pocketed or s1 == const.stationary: - a1x, a1y, b1x, b1y = 0, 0, 0, 0 + a1x, a1y, b1x, b1y = 0.0, 0.0, 0.0, 0.0 else:Also applies to: 54-55
🤖 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/evolution/event_based/detect/ball_ball.py` around lines 37 - 38, The branch that sets a1x, a1y, b1x, b1y uses integer literals (0,0,0,0) while another branch uses float literals (0.0,0.0,0.0,0.0), which can break Numba nopython type inference; update the integer assignments to floating-point literals (0.0) in the branch that checks s1 == const.spinning or s1 == const.pocketed or s1 == const.stationary and any other similar branch (the one around the other zero assignment) so both use 0.0 for a1x, a1y, b1x, b1y.
27-28: 💤 Low valueRemove unused parameters
m1andm2from function signature.These mass parameters are declared and passed by all callers but never referenced in the function body. Collision time is a kinematic property—mass doesn't affect when balls collide, only post-collision dynamics. Removing them requires updating the function signature and all call sites (main code at lines 129-130 and test cases), but improves clarity.
🤖 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/evolution/event_based/detect/ball_ball.py` around lines 27 - 28, Remove the unused parameters m1 and m2 from the function in ball_ball.py that currently declares them (they are never referenced in the body); update the function signature to omit m1 and m2, remove any related parameter docs/comments, and update all call sites that pass masses (including the main caller and tests) to stop supplying those two arguments so signatures match; run tests/linter to ensure no remaining references.pooltool/evolution/event_based/detect/ball_pocket.py (1)
128-140: 💤 Low valueMinor inconsistency in threshold handling.
Line 139 uses
r1 < 0.0to reject negative roots, whileball_vertical_plane_collision_time(line 89) usesroot.real <= const.EPSto reject near-zero roots. Similarly, line 129 uses a hardcoded1e-9while the plane function usesconst.EPSfor imaginary checks.This could cause edge-case differences where very small positive roots (e.g., 1e-15 due to floating-point precision) are accepted here but rejected in other solvers. Consider aligning with
const.EPSfor consistency:♻️ Suggested alignment with const.EPS
- atol = 1e-9 - if abs(roots[0].imag) > atol or abs(roots[1].imag) > atol: + if abs(roots[0].imag) > const.EPS or abs(roots[1].imag) > const.EPS: return np.inf- if r1 < 0.0: + if r1 <= const.EPS: return np.inf🤖 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/evolution/event_based/detect/ball_pocket.py` around lines 128 - 140, The code in ball_pocket.py uses hardcoded thresholds (atol = 1e-9 and r1 < 0.0) causing inconsistency with ball_vertical_plane_collision_time which uses const.EPS; replace the hardcoded atol with const.EPS for the imaginary-part checks on roots and change the negative-root test from r1 < 0.0 to r1 <= const.EPS so root rejection logic (roots[], r1, r2, and the imaginary checks) aligns with ball_vertical_plane_collision_time's use of const.EPS.
🤖 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 `@sandbox/airborne_demos.py`:
- Line 100: The docstring currently references the stale symbol
ball_pocket_collision_time_airborne; update that reference to the migrated
helper name ball_pocket_collision_time_if_airborne in the docstring so it points
to the canonical symbol used in this PR (search for any occurrences of
"ball_pocket_collision_time_airborne" and replace with
"ball_pocket_collision_time_if_airborne" in the airborne_demos docstring and
cross-reference markup).
---
Nitpick comments:
In `@pooltool/evolution/event_based/detect/ball_ball.py`:
- Around line 37-38: The branch that sets a1x, a1y, b1x, b1y uses integer
literals (0,0,0,0) while another branch uses float literals (0.0,0.0,0.0,0.0),
which can break Numba nopython type inference; update the integer assignments to
floating-point literals (0.0) in the branch that checks s1 == const.spinning or
s1 == const.pocketed or s1 == const.stationary and any other similar branch (the
one around the other zero assignment) so both use 0.0 for a1x, a1y, b1x, b1y.
- Around line 27-28: Remove the unused parameters m1 and m2 from the function in
ball_ball.py that currently declares them (they are never referenced in the
body); update the function signature to omit m1 and m2, remove any related
parameter docs/comments, and update all call sites that pass masses (including
the main caller and tests) to stop supplying those two arguments so signatures
match; run tests/linter to ensure no remaining references.
In `@pooltool/evolution/event_based/detect/ball_pocket.py`:
- Around line 128-140: The code in ball_pocket.py uses hardcoded thresholds
(atol = 1e-9 and r1 < 0.0) causing inconsistency with
ball_vertical_plane_collision_time which uses const.EPS; replace the hardcoded
atol with const.EPS for the imaginary-part checks on roots and change the
negative-root test from r1 < 0.0 to r1 <= const.EPS so root rejection logic
(roots[], r1, r2, and the imaginary checks) aligns with
ball_vertical_plane_collision_time's use of const.EPS.
🪄 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: 8b176fe0-8ed2-457c-804a-76a365e20a91
📒 Files selected for processing (14)
docs/include_exclude.pypooltool/evolution/event_based/detect/__init__.pypooltool/evolution/event_based/detect/ball_ball.pypooltool/evolution/event_based/detect/ball_cushion.pypooltool/evolution/event_based/detect/ball_pocket.pypooltool/evolution/event_based/detect/ball_table.pypooltool/evolution/event_based/detect/detector.pypooltool/physics/motion/__init__.pypooltool/physics/motion/solve.pypooltool/physics/utils.pypooltool/ptmath/roots/__init__.pysandbox/airborne_demos.pytests/evolution/event_based/test_ball_pocket.pytests/evolution/event_based/test_simulate.py
💤 Files with no reviewable changes (2)
- pooltool/physics/motion/solve.py
- docs/include_exclude.py
| Exercises both Strategy 1 (landing directly inside the pocket) and Strategy 2 | ||
| (xy trajectory crossing the pocket cylinder mid-fall) in | ||
| :func:`pooltool.physics.motion.solve.ball_pocket_collision_time_airborne`. | ||
| :func:`pooltool.evolution.event_based.detect.ball_pocket.ball_pocket_collision_time_airborne`. |
There was a problem hiding this comment.
Docstring references a likely stale function name.
Line 100 points to ball_pocket_collision_time_airborne, but the migrated airborne helper in this PR is ball_pocket_collision_time_if_airborne. Please update the reference so docs/users don’t chase a non-canonical symbol.
Suggested docstring fix
- :func:`pooltool.evolution.event_based.detect.ball_pocket.ball_pocket_collision_time_airborne`.
+ :func:`pooltool.evolution.event_based.detect.ball_pocket.ball_pocket_collision_time_if_airborne`.📝 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.
| :func:`pooltool.evolution.event_based.detect.ball_pocket.ball_pocket_collision_time_airborne`. | |
| :func:`pooltool.evolution.event_based.detect.ball_pocket.ball_pocket_collision_time_if_airborne`. |
🤖 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 `@sandbox/airborne_demos.py` at line 100, The docstring currently references
the stale symbol ball_pocket_collision_time_airborne; update that reference to
the migrated helper name ball_pocket_collision_time_if_airborne in the docstring
so it points to the canonical symbol used in this PR (search for any occurrences
of "ball_pocket_collision_time_airborne" and replace with
"ball_pocket_collision_time_if_airborne" in the airborne_demos docstring and
cross-reference markup).
1 participant
Co-locate collision-time solvers with their detectors
Summary
pooltool/physics/motion/solve.pywas a kitchen-sink module holding collision-time root-finders for every event type, while their corresponding detectors (cache + dispatch + event construction) lived underpooltool/evolution/event_based/detect/. Solver and detector for the same event type sat in distant directories, which made the 2D/3D fork harder to reason about and obscured what code each event type actually owned.This PR collapses that split. Each
detect/<event>.pynow owns both its detector wrapper and the math function that powers it.physics/motion/is deleted. Along the way, the 2D/3D detection fork is simplified — only the event types where the math genuinely diverges (ball-ball, ball-table) keep an explicit fork; the rest now share a single detector that branches per ball state internally.Closes #322.
Summary by CodeRabbit
Refactor
Documentation