fix(hy3): decode the swimmer sex column case-insensitively - #28
Open
fsalum wants to merge 13 commits into
Open
Conversation
Hy-Tek uses three diving stroke chars, not one: F = 1-metre springboard, G = 3-metre springboard, H = platform. The prior DIVING = "F", "6", 6 treated F as the only diving code and extrapolated numeric aliases "6"/6 from the FREESTYLE..MEDLEY pattern (1..5) with no corpus evidence Hy-Tek ever writes those in the stroke column for diving -- a wrong alias could silently mis-map some other numeric code onto diving. Measured on a real corpus: 2012 PAC-12 (F=26 G=26 H=26), 2007 SEC (F=60 G=60 H=54), 2005 PAC-10 (F=15 G=15 H=12) all show three parallel diving events. Across 13 multi-diving-event files, G never appears without F and H never appears without G (strict nesting). Dive counts match within a meet across chars, and median scores ascend F < G < H, tracking increasing degree of difficulty. HS meets emit only F, consistent with NFHS being 1-metre only. G and H previously fell through select_from_enum() to UNKNOWN, silently dropping all 3-metre and platform diving results. Drop the numeric aliases; use the character form only, since select_from_enum(Stroke, ...) is only ever called with a single extracted stroke-column char (hy3) or event_stroke field (hyv), never a numeric value, in the whole codebase.
Two hy3 line parsers raised on real-world Meet Manager exports instead of degrading gracefully: - f3_parser: an F3 relay leg referencing a swimmer meet-id with no D1 roster record raised KeyError. Skip the unrostered leg (the relay keeps its other legs), matching the existing tolerance for the empty-swimmers and absent leg-1 cases. - h1_parser: an H1 DQ-detail line whose last_entry carries no DQ slot raised an AssertionError. Make it a no-op, mirroring h2_parser -- an H1 can resolve to a non-DQ entry (e.g. a relay DQ, or a non-DQ entry emitted between the DQ result and its H1). The DQ result itself is unaffected; only the reason string is skipped. Adds regression unit tests for both.
Reaction and relay-takeoff columns need their own coercion. parse_time_or_none
requires a positive value, but a takeover slot records an early exchange as a
negative number -- 7,868 such values across a 33,008-file corpus -- so reusing
it would drop every one of them.
Sentinels are plural and slot-dependent: blank, 0.00 in any sign spelling, and
the literal NRT ("No Reaction Time") that Meet Manager writes into unmeasured
takeover slots in 513 files. All map to None.
Values above the plausible reaction range are passed through unchanged. A
minority of files put something else in these columns; filtering here would
make that population permanently invisible.
The parser read backup_4 through col 82 and then jumped to the date at col 88, dropping the five columns between. They hold the swimmer's start reaction time: 2.56% of 53.7M E2 rows corpus-wide carry a value in the plausible range. Includes an offset regression test. A one-column error still produces plausible-looking floats, so the neighbouring date is the tripwire that catches a misread.
The code already noted a '15-column gap' before F2's date field without
identifying it. That gap is relay legs 2, 3 and 4 (3 x 5 chars); leg 1 sits in
the 5-char slot F2 shares with E2.
The columns are reaction times, and the file format says so itself: 513 files
write the literal NRT ('No Reaction Time') into unmeasured takeover slots, and
leg 1 in those same rows carries a signed +0.00.
The physics corroborates it across the corpus. Leg 1 is a block start: 23
negatives out of 87,069, median 0.61. Legs 2-4 are exchanges: 2,702 / 2,282 /
2,861 negatives, median ~0.24. Nothing but a takeoff time behaves that way.
Exposed as a positional 4-list rather than four named attributes, matching the
file's uniform column layout.
Two PII-redacted slices of real meets, covering the two opposite shapes the reaction columns take. The dense fixture is a modern fully-automatic-timing meet: 35 of its 37 E2 rows carry a start reaction time (0.53-0.90), all four F2 takeoff slots are populated on every relay, and one relay has three negative takeovers. That last one is the regression guard -- a >0.0 filter on these columns would drop every early exchange and the tests would still look green without it. The NRT fixture is the opposite: an older generation that wrote the literal NRT into every takeover slot and a signed +0.00 into every leadoff, with the E2 reaction column blank throughout. Everything must parse to None. A raw-file tripwire asserts the sentinels are still physically present, so a regenerated fixture that lost them cannot pass by vacuous truth. Redaction extends the documented rules to two fields they had missed: the 5-char last-name prefix that E1 and each F3 relay-leg slot carry. No parser reads them, but they are real surname fragments. Verified by diffing every emitted line against its source line and asserting the differing character positions fall only inside the redacted column ranges.
E1 cols 9-13, and the same columns in each 13-char F3 relay-leg slot, hold the first five characters of the swimmer's last name. The documented redaction rule set did not cover them, so the five fixtures added before the rule existed still carried 109 distinct real surname fragments between them. No parser reads either field -- e1_parser takes the swimmer id at cols 4-8 and the event gender at col 14, and f3_parser takes the id and the leg number at col 15 -- so blanking them to Swimm, matching the D1 last_name placeholder Swimmer<meet_id>, is parse-safe. All 106 tests pass unchanged. Only those columns were touched: every file is byte-for-byte the same length, and a character-level diff against the previous revision confirms every changed position falls inside an E1 or F3 name-prefix range. Line checksums and all other record types are untouched. A full re-audit of all seven fixtures against the complete documented rule set now reports no unredacted personal field of any kind.
…ture Completes the previous commit, which covered five of the six fixtures that predate the E1/F3 name-prefix rule and missed this one. 221 occurrences, 142 distinct real surname fragments, all in F3 leg slots -- this fixture exists to exercise relays with alternates, so it is almost entirely F3 content. Two of its F3 lines carry a fifth leg, so anything checking only the first four slots will under-report it; all eight slots are covered here, matching f3_parser's own range. No parser reads cols 9-13 of a leg slot -- f3_parser takes the swimmer id at cols 4-8 and the leg number at col 15 -- so blanking them to Swimm is parse-safe. The file is byte-for-byte the same length, a character-level diff against the previous revision puts every changed position inside an F3 name-prefix range, and test_bug2b_relay_entries_with_alternates_preserve_leg_numbers, which reads this fixture, still passes along with the rest of the 106. Every fixture in the directory now satisfies the documented rule set.
float() happily parses "nan"/"inf"/"-inf" and all fit the 5-character E2/F2 reaction-time column, so parse_reaction_time let them through. A bare int() downstream then raises (ValueError on nan, OverflowError on inf) instead of yielding None like every other malformed token, dropping the whole file from a run. Reject non-finite values and add unit test coverage for nan and inf in both signs.
Two inaccuracies in the mm_relay_nrt_sentinel.hy3 documentation: - The fixtures README attributed the file to "Montana Swimming", but its A1 licensee and first C1 team both read Inland Empire; corrected to the LSC the file's own C1 region actually names. - Both the README and a test docstring claimed the E2 reaction column is "blank throughout". Measured: 25 of the 40 E2 rows carry a signed +0.00 sentinel and only 15 are blank. The test itself was already correct (both spellings parse to None); only the prose was wrong, and it matters here because this fixture is the only integration coverage of the +0.00 sentinel -- a maintainer trusting the blank claim could swap in a genuinely blank fixture and silently lose that coverage with a green suite.
A library should not ship a lockfile; it was untracked but not gitignored, so a future git add -A could have pushed it into history.
The D1 sex column is read through select_from_enum(Gender, ...), which turns any ValueError into Gender.UNKNOWN. Gender is Unique over "M"/"F"/"U", so a lowercase sex byte silently became UNKNOWN. That conflates two different things. UNKNOWN means the file did not state a sex; a lowercase 'm' means it did, in lowercase. Consumers that gate on a known gender then drop those swims entirely, and nothing upstream reports an error -- the record parses fine. Fixed with a _missing_ hook on Gender rather than .upper() at the call sites: there are four readers of this enum across the hy3 and hyv parsers, and call-site normalization drifts. The hook retries only when upcasing changes the value, or an unrecognized uppercase byte would re-enter it forever. _missing_ rather than aenum's _missing_value_ so the behavior survives a change of enum base instead of silently becoming a no-op. Scoped to Gender. A census of 34,008 hy3 files found lowercase bytes in no other letter-code column across 56.7M records, and normalizing them would coerce garbled input into plausible values -- a shifted lowercase byte in the course column would read as SCM instead of UNKNOWN.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
d1_parserreads the D1 sex column throughselect_from_enum(Gender, ...).GenderisUniqueover"M"/"F"/"U", andselect_from_enummaps anyValueErrortoGender.UNKNOWN, so a lowercase sex byte silently decodes toUNKNOWN:UNKNOWNis the "file did not state a sex" value. A lowercasemmeans the file did state it. Collapsing the two makes a stated fact unrecoverable, and any consumer that gates on a known gender drops those swims — with no error, since the record parses fine.Scope
Rare but real. Across a corpus of 34,008
.hy3files (10,991,243 D1 records) I found one lowercase sex byte, in a real file where two siblings sharing a surname appear twenty lines apart (names below anonymized):That one byte cost 8 results.
Fix
A
_missing_hook onGender, rather than.upper()at the call sites — there are four readers of this enum acrosshy3andhyv, and call-site normalization drifts apart over time.Two details worth flagging for review:
_missing_with an identical value and recurses until the stack blows._missing_rather than aenum's native_missing_value_. Both work identically today;_missing_is also the stdlib hook, so the behavior survives a change of enum base rather than silently becoming a no-op.Scoped deliberately to
Gender. The same census found lowercase bytes in no other letter-code column across 56.7M records (stroke, course, seed course, result type, time code, meet type, gender-age), so normalizing insideselect_from_enumwould buy nothing and cost signal: a shifted lowercase byte in the course column would read asSCMinstead ofUNKNOWN, which is what makes corrupt records detectable today.Blank,
X, and unrecognized bytes all still resolve toUNKNOWN— covered by the tests.Stacking
Stacked on #27 (which is stacked on #26 → #25). Only the final commit is new; please merge those first.