Skip to content

fix(ballistics): replace Hill lift form with Ferguson quadratic Cd/Cl - #266

Merged
jewbetcha merged 3 commits into
open-flight:mainfrom
cvrt-jh:fix/cl-quadratic-form
Sep 23, 2026
Merged

jewbetcha merged 3 commits into
open-flight:mainfrom
cvrt-jh:fix/cl-quadratic-form

Conversation

@cvrt-jh

@cvrt-jh cvrt-jh commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Replaces the aerodynamic coefficient forms in ballistics.py. Drag was linear in the spin parameter and lift a Hill-type saturating curve, four fitted constants between them. Both are now second-order polynomials in the spin parameter, using the coefficients Ferguson, McNally & McPhee (2022, ISEA 14, open access) published from their 1040-shot fit. Nothing was re-fitted here; the six numbers are theirs as printed.

Fixes #230, Option A, the variant we recommended in the validation comment there.

Five files, Python and docs only:

  • src/openflight/ballistics.py: two coefficient tuples, a clamp constant, and the two evaluators. Integrator, Magnus geometry, spin decay and ball constants untouched.
  • tests/test_ballistics_trackman_regression.py: budgets ratcheted down to the new baseline.
  • scripts/analysis/sweep_ballistic_coeffs.py: the fitting tool referenced the removed constant names and would have crashed, so it now sweeps the six polynomial coefficients.
  • docs/how-it-works/ballistics.md and docs/reference/constants.md: model equations and constants tables (both still showed pre-fix(ballistics): re-fit aero coefficients against TrackMan reference capture #229 values).

Why was this required?

The Hill form is monotonic and flattens by construction. Measured lift rises, peaks near a spin parameter of about 0.5, then falls. No choice of the two Hill constants produces that shape, so #229 could only trade carry accuracy against drag, leaving every club about 4 yd low at apex.

Both arms below run the unchanged simulate() path fed TrackMan's measured launch conditions, so the only difference is the body of _cd and _cl.

Committed 24-shot capture (the #219 regression fixture):

club Hill (#229) quadratic
7-iron 2.96 1.33
driver 1.75 2.38
pitching wedge 6.26 4.48
overall carry RMSE 3.97 2.90

Driver gets slightly worse on this fixture; its budget was held, not raised, and still passes.

Independent 5-session archive from @johnpacino, 279 clean shots these coefficients never saw, with per-shot spin axis and per-session air density:

Hill (#229) quadratic
carry RMSE 3.48 1.98
carry MAE 2.79 1.61
apex RMSE 4.41 0.97
apex bias -3.99 -0.59

Per club carry RMSE, Hill vs quadratic: 9i 3.22 / 1.61 (n=176), driver 4.10 / 1.90 (33), 5i 3.35 / 2.58 (32), 7i 3.90 / 1.96 (30), sand wedge 4.73 / 4.92 (8). On all 416 shots including mishits: 4.83 / 2.25.

Apex is the strongest evidence: nothing here was ever fitted against it, and the bias column shows the Hill form was structurally low, not noisy.

One implementation detail worth a look. The spin parameter is evaluated every integration step, and for wedges near apex it climbs past the fitted range (0.02 to 0.75) because speed falls faster than spin decays. Extrapolated, the parabolas send lift negative above 1.09 and drag negative above 1.25, both reachable on a high lob. So above SP_FIT_MAX = 0.75 both curves are held at their end value. That clamp is why the numbers above differ slightly from the #230 comment, which ran the raw polynomials (carry 1.91 to 1.98, apex 1.00 to 0.97, sand wedge 3.82 to 4.92 on 8 shots). Lift is also clamped at zero, and zero spin still gives zero lift.

Automated tests

No new test file. The #219 guard covers exactly this surface; its budgets were ratcheted: overall 5.0 to 3.5, 7-iron 4.0 to 2.0, pitching wedge 7.5 to 5.5, driver held at 3.0. The before/after table is in the module comment.

Verified they can still fail: lowering the linear lift coefficient from 1.2031 to 1.0031 fails all four assertions (overall 10.94 yd against 3.5), and restoring it goes green.

Full suite locally on Python 3.11: 1691 passed, 10 skipped, zero failures.

Manual (human) testing

  • Re-ran both validations against the patched module rather than a monkeypatch, the committed capture through validate_ballistics.py and the archive through our own harness, reproducing every number above.
  • Controls: a zero-lift patch moves capture RMSE to 76.55 yd and restores exactly; a Hill re-patch reproduces 3.97 exactly; the clamp returns identical lift at 0.75 and 1.5, zero at zero spin, and zero for a deliberately overshooting negative term.
  • Smoke-ran the adapted sweep script on the capture with a tiny search budget: runs end to end, prints six coefficients, fits to 2.03 yd near the published values, consistent with the 24-shot fit in Cl Hill form is monotonic, so it cannot represent the measured lift peak near S~0.5 #230.
  • Docs build clean with the strict flag. Pylint 9.52, identical to main. Ruff check has one pre-existing import-order finding in the sweep script's untouched import block, and ruff format flags pre-existing lines in both Python files; no line this PR adds is flagged. Left alone rather than mixing a formatting sweep into a physics change.

Caveat: Python 3.11 only locally; 3.10 and 3.12 rely on CI. Pure float arithmetic, no version sensitivity expected.

AI assistance disclosure, per AI-POLICY.md: worked with Claude throughout, on the code, the validation runs and this text. The physics decisions, the clamp, and the numbers reported here are ours.

Checklist

  • Single feature/fix: the Cd/Cl functional form, plus the test, tool and doc updates the change forces
  • Automated tests included: ratcheted regression guard, proven able to fail
  • Manual testing described
  • Python tests pass: 1691 passed, 10 skipped
  • Pylint passes: 9.52/10, unchanged from main
  • Ruff passes on all lines this PR adds (one pre-existing finding in an untouched import block noted above)
  • UI builds: n/a, no UI changes
  • UI lint passes: n/a, no UI changes
  • Updated docs: both constants tables and the model description
  • No unrelated changes mixed in

🤖 Generated with Claude Code

@johnpacino johnpacino left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The model change looks good: I confirmed the committed 24-shot carry results, and the cap addresses the negative-drag case we discussed.

One request before I approve: please add automated tests for the new safeguards and the apex improvement. The current carry tests still pass if the cap is removed, even though drag becomes negative at a spin ratio of 1.5. A small test covering zero-spin lift, both curves above the 0.75 cap, and positive drag for a low-speed lob-wedge shot would protect that behavior.

The committed TrackMan CSV also has apex measurements (Max Height - Height, in feet). Please add an apex error budget using that fixture; I independently confirmed a substantial improvement for the 7-iron and pitching wedge. There’s no need to add the full five-session archive to this PR.

With those tests and green CI on the update, I’d be comfortable approving.

The saturating Hill form is monotonic, so it cannot represent the measured
lift peak near Sp ~ 0.5 and left every club ~4 yd low at apex. Switch Cd
and Cl to the second-order polynomials Ferguson, McNally & McPhee (2022)
fitted to 1040 shots, using their published coefficients unchanged, and
hold both curves at Sp = 0.75 so in-flight spin parameters beyond the
fitted range do not extrapolate into negative lift or drag.

Carry RMSE on the committed capture 3.97 -> 2.90 yd; on an independent
279-shot set 3.48 -> 1.98 yd carry and 4.41 -> 0.97 yd apex. Regression
budgets ratcheted, sweep tool moved to six parameters, docs updated.

Fixes open-flight#230
The Sp clamp added with the Ferguson quadratics had no test: the full
suite passed with both clamps removed, even though the raw parabolas
give Cd = -0.35 at Sp 1.5 (negative drag accelerates the ball) and
Cl < 0 above Sp 1.09.

Add coefficient-level guards for zero-spin lift, non-negative lift and
positive drag above the fitted range, that both curves are held flat
past SP_FIT_MAX, and that the fitted region below it is untouched.
Reverting the clamp fails three of them.

Also add an apex error budget against the committed TrackMan capture,
which ships apex as "Max Height - Height" in feet. Apex is out-of-sample
evidence since nothing was ever fitted against it: overall RMSE is
15.05 ft on the Hill form against 2.56 ft on the quadratics, with the
7-iron at 18.15 -> 1.53 ft and the pitching wedge at 18.00 -> 2.81 ft.
Restoring the Hill form fails all four apex budgets.

Apex is plumbed through the validator's own loader rather than parsed
separately, so the manual tool and the test cannot disagree about the
fixture. Budgets are seeded above measured to absorb float drift, matching
the carry budgets, and a coverage test pins apex presence so a changed
fixture cannot turn them into vacuous passes over an empty list.
@cvrt-jh
cvrt-jh force-pushed the fix/cl-quadratic-form branch from af0bbeb to 2308b02 Compare September 17, 2026 11:29
@cvrt-jh

cvrt-jh commented Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

Both added, and the clamp gap was real. With SP_FIT_MAX removed from _cd and _cl the entire suite still passed, so the guard had no coverage at all.

Safeguard tests (TestAeroCoefficientSafeguards, 6 tests): zero-spin lift, non-negative lift and positive drag above the fitted range, both curves held flat past SP_FIT_MAX, and the fitted region below it unchanged. Reverting the clamp fails three of them.

Your Sp 1.5 figure checks out: raw Cd is -0.3503 there, crossing zero at Sp ~1.251, and raw Cl goes negative above Sp ~1.09.

One thing worth flagging on the lob-wedge case, because it did not work the way I expected. I wrote that test, and it passed with the clamp removed. Removing the clamp lowers peak Sp rather than raising it (1.510 to 1.226 for 55 mph / 40 deg / 10000 rpm), since less drag keeps the ball faster, and the unclamped flight then never reaches negative drag at all: 0 negative-Cd evaluations out of 6076 integration steps. So a flight-level assertion cannot act as a red control here. I kept the shot as a reachability test, asserting that a realistic lob wedge does push Sp past SP_FIT_MAX at all, with the limitation written into the docstring. The coefficient-level tests are what actually hold the guard.

I also discarded an assertion of my own along the way: "speed never increases in flight" is wrong, since a descending ball gains speed under gravity either way (31 rises clamped, 30 unclamped). It would have failed on correct code.

Apex budget, from the committed capture's Max Height - Height:

club Hill quadratic budget
7-iron 18.15 1.53 2.0
pitching wedge 18.00 2.81 3.5
driver 5.08 3.17 4.0
overall 15.05 2.56 3.0

All feet, n=24. Restoring the Hill form fails all four budgets. This matches the substantial 7-iron and pitching wedge improvement you saw independently.

Apex is plumbed through load_trackman/validate_tm_inputs rather than parsed separately, so the test and the manual validator cannot disagree about the fixture, per that module's existing note. The fields are Optional and default to None, so the paired comparison CSV path is unaffected and the per-shot CSV columns are unchanged. There is also a coverage test pinning apex presence on all 24 shots, so a fixture that loses the column fails loudly instead of turning the budgets into vacuous passes over an empty list.

Budgets are seeded above measured to absorb float and platform drift, matching the carry budgets; ratchet down, never up.

No five-session archive here, as you asked.

Verification: 1565 passed, 7 skipped on py3.11. That is below the 1691 I reported on the last round: clean main is now 1554/7 after the recent commits, and this branch adds 11. Rebased onto current main, with the pre-existing commit's diff unchanged apart from blob hashes and no competing commits on any touched file. Docs build clean under --strict. The ruff/pylint findings on validate_ballistics.py (unused json and SPIN_CONFIDENCE_HIGH imports, plus one formatting complaint) are present on clean main and left alone rather than mixed into this change.

Worked with Claude throughout, on the code, the validation runs and this text. The physics decisions, the clamp, and the numbers reported here are ours.

@johnpacino johnpacino left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for updating, this looks good!

@johnpacino

Copy link
Copy Markdown
Collaborator

@jewbetcha - can I get your Eyes on this one?

@jewbetcha jewbetcha left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jewbetcha
jewbetcha merged commit 2974e5d into open-flight:main Sep 23, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Cl Hill form is monotonic, so it cannot represent the measured lift peak near S~0.5

3 participants