Repository navigation
Conversation
Sanjana-Siddani
left a comment
There was a problem hiding this comment.
Nice work! All tests pass and everything runs fine for me.
One fix before merging: abs_bias averages the bias across flights first and then takes the absolute value. So if a pilot is 50 ft too high on one flight and 50 ft too low on another, they cancel out and the pilot looks perfect. Taking the absolute value for each flight first and then averaging fixes this. When I tried it, the groups matched the real tiers on all 12 test seeds instead of only some.
Also, maybe add the generated CSVs to .gitignore. Will approve after the fix
Phlabry
left a comment
There was a problem hiding this comment.
ADR 0002's linear-versus-log numbers don't hold at this head. 0.408 and 0.873 reproduce only
against the old abs(mean bias). With the per-flight abs_bias, log and linear both score 1.000,
and so do mad only and mad plus std, the options the ADR rejects.
Cause is the generator: TIER_SCALE 1.0 / 2.5 / 6.0 with METRIC_VARIATION 0.7 to 1.4 gives tier
bands that never overlap, so once the fix removed the noise the tiers are trivially separable. That
also makes test_synthetic_pilots_are_grouped_by_their_hidden_tier's >= 0.8 pass for every option
here. Overlap the tiers, then re-measure the ADR.
Two smaller ones:
--kisn't validated.--k 1gives one group for everyone,--k 15on 15 pilots gives one pilot
each, while the sweep path enforces 3 per group. Check k againstcandidate_ks.truth_agreementmergeshow="left", so a duplicatepilot_idin--truthturns 15 pilots into
16 rows and shifts the score.truth["pilot_id"].is_uniquewould catch it.
Otherwise it runs clean: 61 tests pass, the README commands work from a fresh checkout, and nothing
generated lands in git status.
|
Follow-ups that came out of this review. They are tracked separately and none is needed to merge this PR:
The figures quoted in #216 and #224 changed with this PR, so I commented on both. |
SummaryAddresses the three review points: overlap the tiers and re-measure ADR 0002, validate inference/
├── generate_synthetic_pilots.py
-│ TIER_SCALE 1 / 2.5 / 6, METRIC_VARIATION 0.7–1.4 # tier bands never overlap
+│ TIER_SCALE 1 / 2 / 4, METRIC_VARIATION 0.5–2.0 # adjacent tiers overlap
├── clustering.py
│ fit(pilots, k)
+│ if k not in candidate_ks(n): raise ValueError # the range the sweep offers
│ truth_agreement(assignments, truth)
+│ if a pilot_id repeats in truth: raise ValueError
└── tests/
docs/adr/0002-cluster-on-std-and-absolute-bias.md Evidence table re-measured
GLOSSARY.md + Hidden tierEvidence
Merge DangerDoor: two-way Docs, a synthetic-data generator and two input checks. No data migration and nothing destructive. Blast Radius: Narrow
|
|
New commit adds inference/data/client to .gitignore fixing #227 |
Fixes #216
What was changed?
inference/generate_synthetic_pilots.py: Synthetic pilots in 3 hidden skill tiers, with targets, in post-Plugin applies metric conversion factors to pilot altitude and airspeed that are already in feet and knots #196 units.inference/clustering.pyandinference/cluster_pilots.py: forms Performance groups from per-pilot Deviation features.inference/tests/test_clustering.py,test_cluster_pilots.py,test_generate_synthetic_pilots.py.docs/adr/0001(pilot metadata checks groups, it does not form them) anddocs/adr/0002(which features, and why a log scale).GLOSSARY.md(Performance group, Synthetic pilot) andinference/README.md(steps 7a and 8).features.py,compute_features.pyandgenerate_balanced_data.pyare untouched.Why was it changed?
The client wants 10 to 20 pilots grouped by performance across heading, vertical speed, altitude and airspeed. The repo had no clustering and no way to test it, since real telemetry with targets doesn't exist yet (#213, #211). This adds a baseline that runs on synthetic pilots now and on real flights later.
Decisions that shape the result:
stdand|bias|per metric (8 total), notmador signed bias. Signed bias would split a pilot 50 ft high from one 50 ft low, andmadmostly repeats the other two. See ADR 0002.log(x + floor). Linear features recovered the synthetic tiers at about 0.40 agreement (adjusted Rand index), the log scale at 0.87.n_init=10returns nearly the same partition for any seed, so a seed-based score would always look stable.How was it changed?
Screenshots (if applicable):
Before: no clustering code;
uv run pytestran 40 tests. After: 59 pass.Tier recovery, the test that gates the PR:
End-to-end run on 15 synthetic pilots (seed 0):
Silhouette favours k=2, stability peaks at k=3. That is the case for leaving the choice of k to a person.
Merge Danger
Door: two-way. All new files plus doc edits, so a revert is clean.
Blast Radius: inference only.
inference/scripts changed.inference/Data/(about 100k rows for the synthetic telemetry). They are not gitignored, so don't commit them.