Report missed turns, and split them when the swimmer says so - #9
Merged
Merged
Conversation
A swim came back 50 m short and the page said nothing. Lap 12 held a 151 s length with 51 strokes against a unit of ~79 s and a median of 28 strokes -- two lengths the watch recorded as one. The per-lap estimate saw three lengths there and was capped, correctly, at the two recorded, because this tool merges and never splits. The problem was the silence, and what followed from it: 51 strokes in "one length" cleared the breaststroke threshold, so the repair wrote breaststroke into a swim the swimmer confirms was freestyle throughout. The file came out worse than the watch left it. A single length now reads as a missed turn when it runs at least 1.75x the unit in duration *and* 1.75x the median in strokes. Both halves are needed. Duration alone fires on an 18 m pool, which reaches 1.61x on its own, and on a kick set or a pause at the wall, which are long without the strokes; the tempting "rounds to two lengths" at 1.5x would have flagged both short-pool fixtures. It is reported, never split -- splitting means inventing a turn time and a stroke split nobody recorded -- as a finding that accounts for the shortfall exactly (21 written + 1 reported = the confirmed 22) and as a marked row in Show the working. It works under a fixed lengths-per-lap too, since how the lap button was pressed has nothing to do with a missed turn. Fixing the stroke found an older bug of the same shape. The page has always said an ambiguous stroke -- count and duration disagreeing -- keeps the watch's label. It did not: repair() re-derived the stroke from the count and wrote that, and two ambiguous groups in the fixtures had the watch's breaststroke overwritten with freestyle. analyze() now decides the stroke per merged group once, including when to leave it alone, and repair() writes exactly that, the way it already takes lapTargets instead of recomputing them. keepStrokeWhenUnsure (default on) covers both cases; the Python reference writes the verdict unconditionally, so AS_REFERENCE turns it off and the goldens stay byte-exact. Default: 12 of 12 ambiguous groups keep the watch's label; as the reference: the same 2 overwrites as before. The swim is fixture swim-08, anonymized outside the tree and renamed on the way in. Each part of the rule has a test that fails without it: detection off, the ratio lowered to 1.5, the label kept off, and the stroke half of the rule removed -- the last needed a doctored kick-set length, since no real fixture exercises it. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The missed-turn finding says a swim is short and by how much; this lets the swimmer act on it. Each finding gets a "Split into 2 lengths" switch, off by default, and --split-missed-turns[=LAPS] does the same on the command line. swim-08 then comes out at the confirmed 1100 m. It is the one place this tool adds data, which is why it is opt-in and per length. A merge only discards: every number it writes is still one the watch measured. A split has to make up two -- where the turn fell, and how the strokes divide -- and on a threshold resting on a single real example the worst it could do is add distance nobody swam. The choices are the neutral ones: the turn goes halfway and the strokes divide with the time, the remainder to the last part so the totals are exact. It runs as a pre-pass. applySplits() writes the extra length as a copy of the original frame placed right after it, so the definition in force is the same one, and returns a new file; analyze() and repair() then run unchanged on that. Lap targets, stroke decisions and the guarantee that the working table adds up to what is written all hold on the split file with no special case. The finding is carried across marked `split`, or the switch that turned it on would vanish the moment it did. What the tests pin is the narrowest honest claim: it adds a length and nothing else. Total time and strokes are unchanged; the two parts sum to the original and start back to back; files without a missed turn come out byte-identical with it on; the output repairs to the same result again; it commutes with anonymizing. Every one of those fails when the split is deliberately broken -- strokes duplicated, parts started at the same second, the finding dropped, the parts left unmarked, or the split turned on by default. Two details the browser caught. The stat cards' "was" figures were counted from the split file and showed 1200 m for a swim the watch recorded as 1150 m -- counting made-up data as "before" -- so the made-up lengths come back out. And the working table's note still said nothing is ever split. The made-up lengths are italic and labelled "split -- made up, not recorded" in the table and starred in the CLI, and the switch keeps keyboard focus when the findings are rebuilt under it. Whether Garmin Connect accepts a file with an added length message is untested from here. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
From an adversarial review of #7 and #8. One real bug and three that share its cause: the split file was treated as if the watch had recorded it. The bug. The missed-turn guard is computed on the split file, where each half looks like one ordinary length. A fixed lengths-per-lap then merged the halves back together, and the doubled stroke count was classified as breaststroke again -- with lengths-per-lap 1, ticking "I remember turning here" wrote four breaststroke lengths into this all-freestyle swim, against three without it. A merged group that contains a made-up length is now as unsure as the missed turn it came from, and keeps the watch's stroke. A test runs every target and holds the split to never writing more breaststroke than not splitting. Same cause, three symptoms. The finding kept saying "now split into 2" when a fixed target had merged the split straight back and the distance had not moved; it now carries `gained`, the lengths the split really added, and the page and CLI say plainly when that is none. And everything that reports the watch counted the made-up length as recorded: the Watch column said 3 for a lap recorded as 2, the lap-structure guard said "24 lengths down to 16" beside a stat card saying 23, a phantom-turn finding claimed the watch recorded lengths it had not, and --dry-run counted 41 lengths in a 40-length file. applySplits() now returns, for every length of the split file, its index in the file as recorded; `recorded`, `info.lengths`, `madeUpLengths` and those findings count through it. That index is also the finding key now. Keys were start times, which two lengths can share -- a doctored file with the 97 s length starting in the missed turn's second had both split. The index is unique, and because it is the index *as recorded* it survives the page re-analysing a file in which another missed turn has already been split. And one of mine, caught by the browser rather than the tests: the recorded- index map was threaded through analyze() but not through repair()'s own copy of the same pre-pass, so the page -- which calls repair() -- still showed 1200 m and "24 lengths" while every node test, which called analyze(), passed. Both now go through one prepare() helper, and the test runs through both. Also: "Reset to defaults" clears the splits, since the default is none; a bare --split-missed-turns on a file with nothing to split says so; the carried finding's note no longer says nothing is split; and two AGENTS.md bullets are corrected -- one said a missed turn was out of reach, the other still described the unit as max() of two estimators, a rule the same file explains was discarded. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
From CodeRabbit on #8, and right. The non-last parts of a split were rounded, so every one could round up and leave the last part a negative remainder: 2 strokes in 4 parts came out as 1, 1, 1 and -1. Written into an unsigned field, -1 is the invalid marker, read back as 0, so total strokes rose from 2 to 3 -- the one thing a split promises not to do. Flooring keeps every non-last part at or below the equal share, so the remainder is never negative. Durations had the same pattern and get the same fix. Real swims do not reach it (it needs about one stroke per length), so the test doctors one: every length a single stroke, and one of them four lengths long. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 43 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (12)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Why this PR exists: #7 and #8 were stacked on #6. #6 was squash-merged into
mainfrom the UI while they were still open, and my later merges of #7 and #8 went into #6's already-merged branch, so none of their content reachedmain. This PR is those four commits replayed ontomain. The result is byte-identical to the reviewed stack, andmainitself was verified identical to #6's final commit.The full discussion lives on #7 (report a missed turn instead of hiding it, and stop writing its stroke) and #8 (split a missed turn when the swimmer says so). In short:
keepStrokeWhenUnsure, pinned off inAS_REFERENCE, keeps the goldens byte-exact.--split-missed-turns=LAP). The turn goes halfway and strokes divide with the time; made-up lengths are marked as such everywhere. swim-08 comes out at the confirmed 1100 m. Verified with Garmin Connect: the swimmer uploaded a split file and it was accepted.Since #7/#8, from an adversarial review and CodeRabbit:
lap-structure,phantom-turnandinfo.lengths→ everything counts through a recorded-index mapgainedsays what really changedrepair()had its own copy of the pre-pass that fell behindanalyze()'s → oneprepare()helper, with tests through bothtask check: 118 tests, lint, privacy scan, build.🤖 Generated with Claude Code