Cushion Normal Improvements

[derek-mcblane]

This is a few minor changes. See the individual commits.

One concern I have is the precision for sphere_half_space_collision seems a little bit worse this way. I had to increase the tolerance in the symmetry unit test. It avoids quaternions and the coordinate system transformation, though, which means the whole thing can be numba-ized.

TODO:

• unit test for decompose_normal_tangent

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[codecov]

Codecov Report

❌ Patch coverage is 68.18182% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 45.34%. Comparing base (0e89b7f) to head (416d65a).
⚠️ Report is 50 commits behind head on main.

Files with missing lines	Patch %	Lines
pooltool/ptmath/utils.py	22.22%	7 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #248      +/-   ##
==========================================
+ Coverage   44.22%   45.34%   +1.11%     
==========================================
  Files         127      144      +17     
  Lines        8554    10103    +1549     
==========================================
+ Hits         3783     4581     +798     
- Misses       4771     5522     +751     

Flag	Coverage Δ
service	45.34% <68.18%> (+1.11%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[ekiefl]

It avoids quaternions and the coordinate system transformation, though, which means the whole thing can be numba-ized.

This is really nice. It seems to be about 60x faster, and in absolute terms this fn call used to take around 160us, which is pretty slow considering a 9-ball break shot currently takes 42ms on my computer. Since there were 15 ball-cushion collisions, around 2ms of that is due to resolving ball-cushion collisions.

---

One concern I have is the precision for sphere_half_space_collision seems a little bit worse this way. I had to increase the tolerance in the symmetry unit test.

I find this concerning, too. It's quite a concession to bump the atol/rtol to 1e-2 when their defaults are 1e-5/1e-8. My biggest fear is that there is some numerical imprecision that's palattable under most conditions, but becomes problematic when doing sensitive parameter sweeps--like the numerical imprecision Ersin discovered in Mathavan ball-ball when he was fitting his 3-cushion trajectories.

Is it clear to you why this imprecision exists?

---

I noticed that the decomposition function can return negative magnitudes. I added a test case with a FIXME highlighting the issue. It seems sensible that maybe that's a desired behavior and that these really aren't magnitudes but signed components or something. If that's the case, maybe we could change the language so there's less potential for misinterpretation.

---

Thanks for improving test parametrization w.r.t $\theta$.

[ekiefl]

Thanks for creating this symmetry in the call signatures.

[ekiefl]

Thanks for adding these tests

[derek-mcblane]

Is it clear to you why this imprecision exists?

No. Maybe due to the cross products in the non-transformed coordinate system. There are probably more chances for error to accumulate this way vs in the collision-normal coordinate system. Or maybe I made a subtle error.

[ekiefl]

Well since I'm worried about it, I'll find time to do a sensitivity test and see if it really is an issue in practice

[derek-mcblane]

Well since I'm worried about it, I'll find time to do a sensitivity test and see if it really is an issue in practice

I suspect it’s due to the subtractions in the cross product calculation. There could be “catastrophic cancellation” in some cases. And probably some other stuff I’m unaware of.

Maybe better to just leave it how it was.

[ekiefl]

So I ran an experiment where the ball bounces off 7 cushions. I vary phi by 1 degree over 100 simulations and plot the x-position of the ball when it contacts the 7th cushion.

Mathavan (max_steps: 1000, delta_p: 0.001):

Frictional inelastic:

Frictional inelastic (this branch):

Stronge compliant:

---

It didn't capture the numerical imprecision observed in the test cases the way that I thought it would. But it did reveal how problematic iteration-based approaches (Mathavan) are.

---

(Written by claude)

import matplotlib.pyplot as plt
import numpy as np

import pooltool as pt
from pooltool.events import AgentType, EventType, filter_type

engine = pt.physics.PhysicsEngine()
engine.resolver.ball_linear_cushion = pt.physics.ball_lcushion_models[pt.physics.BallLCushionModel.IMPULSE_FRICTIONAL_INELASTIC]()


def get_seventh_cushion_x_position(system):
    cushion_events = filter_type(
        system.events, [EventType.BALL_LINEAR_CUSHION, EventType.BALL_CIRCULAR_CUSHION]
    )

    if len(cushion_events) < 7:
        return None

    seventh_event = cushion_events[6]

    for agent in seventh_event.agents:
        if agent.agent_type == AgentType.BALL:
            return agent.final.state.rvw[0][0]

    return None


phi_values = np.linspace(52, 53, 100)
x_positions = []

print("Running parameter scan...")
for i, phi in enumerate(phi_values):
    system = pt.System(
        cue=pt.Cue(cue_ball_id="white"),
        balls=(
            pt.Ball.create("white", xy=(0.4, 0.4)),
            pt.Ball.create("yellow", xy=(0.1, 0.1)),
        ),
        table=pt.Table.from_game_type(pt.GameType.THREECUSHION),
    )

    system.cue.set_state(V0=4, phi=phi, a=0.4, b=0.2)
    pt.simulate(system, inplace=True, engine=engine)

    x_pos = get_seventh_cushion_x_position(system)
    x_positions.append(x_pos)

    if (i + 1) % 10 == 0:
        print(f"  Progress: {i + 1}/100 simulations")

valid_indices = [i for i, x in enumerate(x_positions) if x is not None]
valid_phi = [phi_values[i] for i in valid_indices]
valid_x = [x_positions[i] for i in valid_indices]

print(f"\nSimulations with 7+ cushion hits: {len(valid_indices)}/100")

plt.figure(figsize=(10, 6))
plt.plot(valid_phi, valid_x, "b-", linewidth=0.5, marker="o", markersize=2)
plt.xlabel("Phi (degrees)")
plt.ylabel("X position at 7th cushion hit")
plt.title("Numerical Precision: X Position vs Initial Angle")
plt.grid(True, alpha=0.3)
plt.tight_layout()
plt.savefig("round_the_world_numerical_imprecision.png", dpi=300)
print("\nPlot saved to: round_the_world_numerical_imprecision.png")
plt.show()
pt.show(system)

[ekiefl]

Maybe better to just leave it how it was.

This is my instinct. And I suspect you're bang on about catastrophic cancellation.

Should we just stomach the loss and revert that change, or should we dig deeper? Your call.

[derek-mcblane]

Should we just stomach the loss and revert that change, or should we dig deeper? Your call.

Reverting for now. Not worth the trouble, I think.

[derek-mcblane]

That's an interesting analysis. Yeah Mathavan doesn't look great there. Could maybe fiddle with max_steps and delta_p.

[ekiefl]

LGTM! Merging now. Thanks for these changes.

[ekiefl]

@derek-mcblane, just noticed this straggler:

I noticed that the decomposition function can return negative magnitudes. I added a test case with a FIXME highlighting the issue. It seems sensible that maybe that's a desired behavior and that these really aren't magnitudes but signed components or something. If that's the case, maybe we could change the language so there's less potential for misinterpretation.

Am I right that this is a signed component?