fix(ballistics): re-fit aero coefficients against TrackMan reference capture - #229
Conversation
2209610 to
7854ac5
Compare
|
@cvrt-jh - Great addition! The coefficient change looks empirically strong. As part of review, I compared the old and new coefficients against my 334 independent TrackMan shots from July 14/17/22 and August 9/24. Overall carry RMSE improved from 14.53 to 5.95 yards, MAE improved from 12.12 to 4.43 yards, and every represented club improved. I’m not requesting changes to the fitted coefficients. Before approval, can you fix the following?
This PR is appropriately scoped to carry accuracy. I’m not requesting total-distance validation or another optimizer run here. A committed anonymized holdout fixture would be useful for future regression protection, but I consider that non-blocking. |
johnpacino
left a comment
There was a problem hiding this comment.
@cvrt-jh — I left a few comments above that should be addressed before approval. The coefficient change itself looks strong. Once the updates are pushed and exact-head CI passes, I’ll re-review.
…capture Carry was systematically wrong per club: fed TrackMan's own measured launch conditions, the model ran 37 yd short on driver (up to 52 yd on one shot) and ~11 yd long on 7-iron and pitching wedge. Root cause was CL_HALF_SP = 0.15, which sits above the entire realistic spin-parameter range. A driver at Sp ~ 0.08 therefore landed at Cl ~ 0.11 against the ~0.18-0.25 that Bearman & Harvey (1976) and Kensrud & Smith (2018) report, so the lift curve never left its low-lift regime. Irons run at Sp ~ 0.27 and escaped it, which is why the error was club-dependent. Re-fit all four coefficients with the in-repo optimizer (scripts/analysis/sweep_ballistic_coeffs.py: differential evolution followed by Nelder-Mead, rho=1.184 to match TrackMan "Flat" normalization). Overall carry RMSE against the capture: 24.52 -> 3.97 yd (-83.8%). club rmse before -> after bias before -> after driver 38.60 -> 1.75 -37.18 -> -0.45 7-iron 11.64 -> 2.96 +11.00 -> +1.52 pitching wedge 13.56 -> 6.26 +11.02 -> -2.96 The fitted Cl now falls inside the cited literature band at every realistic spin parameter, so this is a physics correction rather than a curve fit. Secondary indicators corroborate: driver landing angle moves from 30.8 to 40.0 degrees (real drivers land at 38-42) and apex to 36 yd. Ratchet the regression budgets in test_ballistics_trackman_regression.py down to the new baseline, and make test_spin_decays_over_flight assert the decay law rather than a window hardcoded around an assumed ~6 s flight -- a correctly-lifted ball now flies 6.9 s and decays further, which tripped the old literal bounds without indicating a regression.
CL_HALF_SP=0.15 was too high for the low-spin driver regime specifically, not the entire realistic spin-parameter range. Irons and wedges were already past it and ran long, which is why the error flipped sign by club. Also removes the claim that the re-fitted Cl stays within the cited 0.18-0.25 band across all realistic spin parameters: driver Cl is 0.13-0.16, below that band. Only the iron/wedge values fall inside it. Addresses review feedback on open-flight#229.
7854ac5 to
54884f8
Compare
|
@johnpacino - thank you for validating against 334 independent shots across five sessions. That is a far stronger test than the 24-shot capture we had, and 14.53 -> 5.95 yd RMSE on data the optimizer never saw is the cross-session evidence we flagged as missing in the limitations section. Much appreciated. Both points are fixed and pushed ( 1. Provenance wording. You are right on both counts, and we checked the numbers rather than just softening the language: So the band claim was simply false for drivers. The comment in 2. Rebased on current We have also added an AI assistance disclosure to the PR description per the new On the anonymized holdout fixture - can you share the data? Short version: the more data you can get us, the more we can help. The physics side is the part we genuinely enjoy digging into, and we would happily put real work into it - the binding constraint is not effort, it is ground truth. We have 24 shots, 3 clubs, one player, one session. You have 334 across five. That gap is why the guard from #219 is in-sample: the current coefficients were fitted against the same capture it tests, so it catches a regression but cannot catch overfitting to that one session. Your validation run is exactly the missing out-of-sample test, and right now it lives in your working copy rather than in CI. With more data we could take on the modelling work properly rather than just guarding it:
Most useful, in rough order: launch conditions plus carry per shot, club label, and whatever session or environment metadata you can include - air density matters and trades off against the coefficients if #169 lands. Anonymization, format and what is publishable are entirely your call. We can work with a CSV shaped like the existing capture, or adapt to whatever suits you. If the data cannot be published, the fallback that still works is you running validation and posting the numbers as you did here - slower, but it keeps the loop closed. Happy to open a separate issue for the fixture either way. |
johnpacino
left a comment
There was a problem hiding this comment.
Re-reviewed the updated head. Both requested changes are resolved: the coefficient provenance now accurately describes the low-spin driver regime and the limits of the cited Cl range, and the branch has been rebased onto current main.
I independently reproduced the committed fixture’s 3.97-yard carry RMSE, the focused tests pass, and all required checks are green on the exact latest SHA. The fitted coefficients are unchanged from the version previously validated against 334 independent TrackMan shots, where every represented club improved.
Approved.
johnpacino
left a comment
There was a problem hiding this comment.
@jewbetcha - can we get your approval on this as well?
What does this PR do?
Re-fits the four aerodynamic coefficients in
ballistics.pyagainst the committed TrackMan reference capture, using the in-repo optimizer. Overall carry RMSE against that capture drops from 24.52 to 3.97 yd (-83.8%).Three files, Python only:
src/openflight/ballistics.py- the four constants, plus a comment recording the fit provenancetests/test_ballistics_trackman_regression.py- ratchet the budgets down to the new baselinetests/test_ballistics.py- fixtest_spin_decays_over_flight(explained below)Follow-up to #218. Branch is cut fresh from current
mainper your note about drift since #169.Why was this required?
Fed TrackMan's own measured launch conditions, carry error was large and club-dependent: driver ~37 yd short, irons and wedges ~11 yd long.
Root cause is
CL_HALF_SP = 0.15. That is the spin parameter at whichClreaches half its saturation value, and it sits well above the low-spin driver regime (Sp ≈ 0.05-0.08). A driver atSp ≈ 0.08lands atCl ≈ 0.11, so for drivers the lift curve never left its low-lift regime. Irons and wedges (Sp ≈ 0.21-0.63) were already pastCL_HALF_SPand ran long instead, which is exactly why the error flipped sign by club.How the numbers were produced
scripts/analysis/sweep_ballistic_coeffs.py, differential evolution then Nelder-Mead,rho=1.184to match TrackMan "Flat". Converged in 1968 evaluations.Evidence this is a physics correction, not a curve fit
Fitting 4 parameters to 24 shots invites overfitting, so I checked it four ways the optimizer never saw.
1. Literature agreement. Fitted
Clnow falls inside the cited 0.18-0.25 band for irons and wedges. DriverCl(0.13-0.16) remains below that band - the band as cited is not a whole-range claim, and the driver figures below make that explicit:2. Independent physical indicators not fitted for. Driver landing angle 30.8 -> 40.0 deg (real drivers land 38-42) and apex to 36 yd. The old shallow landing angle was the classic under-lift signature.
3. Cross-validation.
Sp0.050-0.078, 7-iron 0.212-0.254, wedge 0.376-0.632. Holding out driver removes the only data constrainingCL_HALF_SPin the low-Spregime, so that test demands extrapolation into an unsampled region. It is a coverage limit of a 3-club capture, not overfitting.Spin training confirms this: held-out drivers score 2.98 and 7.82 yd (shipped scores 36.80 and 40.33 on the same shots), andCL_HALF_SPlands at 0.0505 / 0.0373 across independent folds - nowhere near 0.15.4. Agreement with an independent published fit. Ferguson, McNally & McPhee (2022, ISEA 14, open access - the baseline the CVPR 2023 paper compares against) fitted
CD = a+bS+cS²,CL = d+eS+fS²to N=1040 shots (GCQuad + FlightScope X3, Titleist Pro V1), achieving 2.74 yd carry MAE. Mean |difference| against their curves:In both regimes where this capture has data, the re-fit moves closer to that independent fit on all four measures.
Known limitations - please read before merging
sweep_ballistic_coeffs.pyhas a--losoflag, so I assume that was the intent.Spdata. Ranges 0.08-0.21 and 0.25-0.38 are empty. A 5-iron or hybrid capture is the single most valuable next data point.Cd0.340 atSp≈0.47above the ~0.21-0.27 literature range, one 15.97 yd outlier). This is a functional-form limit, not a fit limit: the Hill formCL_SAT·S/(CL_HALF+S)is monotonic and can only flatten, while measuredClpeaks nearS≈0.5then falls. Swapping in a quadratic reaches 1.99 yd overall / 3.33 yd on wedges on the same data. I will file that separately rather than expand this PR.AI assistance disclosure
Per the new AI-POLICY.md: AI assistance was used for this change. It ran the in-repo optimizer, the cross-validation splits, and drafted the provenance comment and this description. I directed the work, reviewed every changed line, independently re-ran
validate_ballistics.pyto reproduce the numbers outside the optimizer's own scoring path, hand-checkedCl/Cdat four spin parameters against the literature, and verified the tightened budget still fails on a deliberately reverted constant. The four coefficients are machine-fitted output; every claim made about them here was checked against the repository and the cited sources.Automated tests
No new test file - the guard from #219 already existed and did its job. Its budgets were seeded at the old error, so it went red immediately; that is the ratchet working. Lowered: overall 26.0 -> 5.0, driver 40.0 -> 3.0, 7-iron 13.0 -> 4.0, wedge 15.0 -> 7.5, with the before/after table in the module comment.
test_spin_decays_over_flightneeded a real fix. It asserted2300 < final_spin < 2900against an assumed ~6 s flight. A correctly-lifted ball now flies 6.92 s and decays to 2274.6 rpm - which matches the decay law exactly (3000·e^(-0.04·6.919) = 2274.7), so this was a stale literal rather than a regression. It now asserts the law (pytest.approx, rel=1e-3) plus a loose sanity band, so it stays valid through future coefficient changes instead of needing another manual edit.Manual (human) testing
validate_ballistics.py, reproducing the sweep's numbers exactly (3.97 yd overall, driver bias -0.45) - so the improvement is not an artifact of the optimizer's own scoring path.CL_HALF_SPto 0.15 and confirmed it goes red loudly -driver: carry RMSE 54.25 yd exceeds budget 3.0 yd. A budget that cannot go red is worthless.Cl/Cdat four realistic spin parameters against the literature (table above).Caveat: I only have Python 3.13/3.14 locally, so the 3.10-3.12 matrix is unverified on my side. The change is pure float arithmetic so I expect no version sensitivity, and CI will confirm (it did for #219).
Checklist
checkpasses on all three filesOne note:
ruff format --checkflagsballistics.py, but those complaints are pre-existing onmain(verified by stashing) in code this PR does not touch. I left them rather than mix a formatting sweep into a physics change - happy to send that separately.