feat(rate-of-closure): add launch monitor analytics tabs - #4212
Conversation
|
Engineering hardening follow-up 4b22e79 splits the analytics Python/TypeScript facades into focused contract, statistics, parsing, and fingerprint modules; all production modules are now <=361 lines and the spec trailing whitespace is fixed. Public facade APIs are preserved. Validation: 583 Rate of Closure Python tests, 374 React tests, focused analytics tests, TypeScript type-check, ESLint, production build, Python 3.12 mypy, Ruff, Black, module-size and diff checks passed. Python 3.13 mypy hit an upstream internal cache assertion; the CI-relevant 3.12 lane is clean. |
|
CI recovery is now published on exact head a4dcddd.\n\nResolved protected-check failures from earlier heads:\n- CI-pinned Ruff formatting for src/rate_of_closure/launch_monitor_analysis.py.\n- Narrow detect-secrets allowlist annotation on the published SHA-256 abc test vector; no baseline or broad exclusion change.\n- Pinned mypy 1.13 Literal typing at the PyQt combo-box boundary and distinct correlation/coefficient loop types; the shared analysis contract was not weakened.\n\nLocal evidence:\n- Ruff check and format: pass\n- focused detect-secrets scan: zero findings\n- React analytics: 5 passed\n- TypeScript type-check: pass\n- ESLint, zero warnings: pass\n- Python fail-closed/missingness boundary: 1 passed\n- PyQt analytics tab: 2 passed\n- exact mypy 1.13 adapter check: pass\n- Prettier and git diff --check: pass\n\nThe full hardening evidence remains 583 Python tests and 374 React tests, plus build, type, static, and module-size gates. The current handoff is docs/development/LAUNCH_MONITOR_ANALYTICS_HANDOFF.md.\n\nThis PR remains draft. Fresh protected checks and required review are still required; no checks or review controls have been bypassed. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a4dcddde61
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| missingPolicy: missing, groupBy: groupBy || undefined, | ||
| confidenceLevel: confidence, minSamples, | ||
| })); |
There was a problem hiding this comment.
In the React panel, every request omits allowAggregate, so an imported dataset containing observation_kind values other than shot always reaches the Aggregate observations require allowAggregate=true error—even when the user selects correlation-only mode, which the shared contract explicitly supports. Add an opt-in control and pass its value; the desktop request construction has the same omission.
Useful? React with 👍 / 👎.
| const columns = [...identity, ...selected]; | ||
| return sha256(JSON.stringify(rows.map((row) => Object.fromEntries( | ||
| columns.map((column) => [column, row[column] ?? null]), | ||
| )))); |
There was a problem hiding this comment.
The displayed dataset SHA-256 covers only the selected analysis columns plus four identity fields. If an imported source URL, environment, unit, monitor setting, or any other retained provenance field changes, the analysis export still reports the same fingerprint, so the hash cannot identify the dataset it claims to trace. Canonicalize and hash every retained row field rather than only columns.
Useful? React with 👍 / 👎.
| const pValue = method === "kendall" | ||
| ? 2 * (1 - normalCdf(Math.abs(coefficient) * Math.sqrt( | ||
| 9 * count * (count - 1) / (2 * (2 * count + 5)), | ||
| ))) |
There was a problem hiding this comment.
When either selected variable contains ties, the coefficient is Kendall tau-b, but its p-value is calculated with the no-ties variance formula. Rounded and categorical launch-monitor measurements commonly contain ties, so this produces incorrect p-values and consequently incorrect Benjamini-Hochberg adjusted values; use a tie-corrected Kendall variance or a validated statistical implementation.
Useful? React with 👍 / 👎.
| let low = -20; | ||
| let high = 20; |
There was a problem hiding this comment.
For low-residual-degree regressions at the allowed high confidence levels, the required Student-t critical value exceeds 20—for example, a four-row single-predictor regression has two residual degrees of freedom and a 99.9% interval needs a critical value above 30. The binary search therefore saturates at 20 and reports confidence intervals that are materially too narrow; grow the bracket until it encloses the requested probability.
AGENTS.md reference: AGENTS.md:L178-L182
Useful? React with 👍 / 👎.
| setRows(next); | ||
| setSourceName(file.name); | ||
| setOutcome(nextNumeric[0]); | ||
| setPredictors([nextNumeric[1]]); |
There was a problem hiding this comment.
When a user imports a file without the currently selected grouping column—most immediately, any file without the default monitor_vendor field—the rows, outcome, and predictors are updated but groupBy remains stale. The controlled selector then has no matching option, while Run still submits the stale name and fails with Columns not present; reset grouping to an available column or no grouping when the dataset changes.
Useful? React with 👍 / 👎.
| const design = complete.map((values) => [1, ...values.slice(1)]); | ||
| const inverseInformation = inverse(multiply(transpose(design), design)); | ||
| const beta = multiply( | ||
| multiply(inverseInformation, transpose(design)), outcome.map((value) => [value]), |
There was a problem hiding this comment.
The browser OLS path forms and directly inverts XᵀX, and the inverse routine rejects pivots using an absolute 1e-12 threshold. A valid full-rank predictor whose values vary around 1e-7 produces a post-intercept pivot below that threshold and is falsely reported as rank deficient; differently scaled or nearly collinear launch-monitor variables also suffer the numerical instability of normal equations. Standardize predictors or use a scale-aware QR/SVD solver.
AGENTS.md reference: AGENTS.md:L178-L182
Useful? React with 👍 / 👎.
8526f7e
into
feat/4181-launch-monitor-registry
|
1 participant
Summary
Stack
This draft intentionally targets
feat/4181-launch-monitor-registry(#4203), the authoritative shared convention/provenance foundation identified for the active Impact Zone Model stack.Verification
python -m pytest tests/rate_of_closure -q— 583 passednpm test— 374 passednpm run type-check— passednpm run lint— passednpm run build— passedRelates to D-sorganization/UpstreamDrift#8364
Closes #4205