lab: diagnose technique-origin performance state - #214
Merged
Merged
Conversation
PhysShell
marked this pull request as ready for review
September 23, 2026 14:21
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 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 |
deterministic_restricted_string scored a known hand by Chain::with_anchor's
anchor_distance feature, which note_features sums over every fretted note on
the path. The preregistered secondary is origin-local, abs(origin_fret(s) -
h), so a neighboring note with a smaller or open fret (often the target)
could outweigh the origin's own distance from the anchor and flip the
reported string.
estimate_with_hand already computes the registered secondary directly from
the conditioned string profile. deterministic_restricted_string now takes
that profile, uses estimate_with_hand to get the origin-local survivor set,
and restricts the chain to those strings before the existing zero-secondary
full-chain DP tie-break — reusing the same restrict_note_strings path the
prior T-only fix validated, instead of Chain::with_anchor.
Adds a brute-force differential test across ~3,200 generated HandEstimate::
Known chains, plus a concrete open-neighbor counterexample
([40, 60, 64], technique domain {2..6}, anchor 10) that the withdrawn code
resolves to origin string 2 and the fix resolves to string 3, matching
independent brute force. Both fail against the withdrawn implementation.
T and J call deterministic_restricted_string only with HandEstimate::Unknown
and are unaffected; estimate_with_hand's typed Known/Ambiguous estimates were
already origin-local and are unaffected.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FNwMUpsNpVM5PsubJLp2kj
The prior deterministic-reporting amendment claimed H-P/H-C used "the registered anchor-distance secondary," but Chain::with_anchor's anchor_distance is a whole-path sum, not the origin-local abs(origin_fret(s) - h) the preregistration froze. Adds a "Hand-secondary correctness correction" section documenting the defect, the fix, and the new regression tests, flags the deterministic H-P (14,613) and H-C (13,360) exact counts and their paired comparisons as stale pending a corpus rerun this environment cannot perform, and notes which figures (typed Known/ Ambiguous, precision, T, J) were never affected. Caveats the Hand state section and evidence-rule D verdict accordingly without rewriting the original numbers. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FNwMUpsNpVM5PsubJLp2kj
Runs technique-origin-state against the real 410-file corpus (fingerprint 9e53e55a19cddf29, verified) with the hand-secondary fix. V0/T/J/BLIND and every typed H-P/H-C Known/Ambiguous/precision figure reproduce bit for bit; only the deterministic H-P/H-C figures move, from the withdrawn 14,613/ 13,360 to 14,729/13,394 — landing exactly on the pre-amendment numbers, because Tuning::candidates orders by ascending string number (ADR-0019), so the DP-based tie-break and the original "lowest string number" reporting provably agree (verified: zero disagreements across every Ambiguous T/H-P/ H-C row in this rerun). Only the anchor_distance bug ever moved these numbers. Updates paired better/equal/worse, macro-by-song, concentration and LOO figures accordingly, and re-examines evidence-rule D: H-P stays unambiguously positive over T; H-C is case-weighted and concentration-view negative but macro-by-song positive, closer than the original framing suggested. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FNwMUpsNpVM5PsubJLp2kj
Second review caught the evidence-rule-D verdict picking case-weighted and the concentration diagnostic as decisive after seeing results, rather than applying the preregistered rule as written: "supported only when... the H-P effect does not disappear under H-C," judged over summaries the prereg itself fixes as case-weighted and macro-by-song, with concentration explicitly registered as descriptive, not a primary gate. Case-weighted (-47) and macro-by-song (+0.1pp) disagree on sign for H-C vs T, so the verdict is now "not supported, mixed" rather than "rejected, on balance," and no longer cites concentration or the (differently-scoped) H-C-vs-V0 LOO figure to break the tie. Also softens "provably... for any chain" for the tie-break-agreement claim: ascending candidate ordering (ADR-0019) explains why the DP-based and lowest-string-number tie-breaks agreeing is plausible and stable, but the DP resolves ties by backward parent-pointer propagation from the chain's last note, not a local comparison at the origin candidate, so ordering alone doesn't prove agreement for an arbitrary chain. The exhaustive zero-disagreement check across all 12,520 Ambiguous T/H-P/H-C rows in the real corpus is the evidence actually used. No numbers change; this is interpretation and doc precision only. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FNwMUpsNpVM5PsubJLp2kj
Owner
Author
|
@coderabbitai config |
|
Tip For best results, initiate chat on the files or code changes.
You are interacting with an AI system. |
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
3 participants
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.
Summary
Outcome
What changed since this PR opened
Review on the original head (
f88cb25) founddeterministic_restricted_stringscored a known hand by the whole path'sanchor_distance(Chain::with_anchor, summed over every fretted note) instead of the registered origin-local secondary,abs(origin_fret(s) - h). That had silently changed the once-reported14,729/13,394to14,613/13,360in an earlier "amendment" commit.f05363dfixes it (routes the registered secondary throughestimate_with_handinstead ofwith_anchor, plus a differential regression test that fails against the withdrawn code and passes against the fix);dcdbc36documents the defect and fix indocs/audit/2026-09-technique-origin-state.md, "Hand-secondary correctness correction."The fix has now been run against the actual licensed 410-file corpus (fingerprint
9e53e55a19cddf29, verified — census, cross-line and relation-count invariants all matched exactly). It reproduced every already-published figure for V0, T, J, BLIND and the typed H-P/H-C Known/Ambiguous/precision columns bit for bit; only the deterministic H-P/H-C figures moved, landing exactly back on the original pre-amendment14,729/13,394(checked directly: the corrected code and the original "lowest surviving string number" reporting agree on all 12,520AmbiguousT/H-P/H-C rows in this rerun, zero disagreements — that corpus-wide agreement, not a general proof, is why; the anchor-distance bug was the only thing that ever moved these numbers).fd04554records that rerun in the audit doc.ae8a552then corrects the doc's own evidence-rule-D verdict, which a second review caught reading two prereg'd summaries (case-weighted, macro-by-song) selectively after the fact instead of applying the preregistered rule as written: H-P is unambiguously positive over T, but H-C is 47 cases behind case-weighted and 0.1 points ahead macro-by-song — the two registered aggregates disagree on sign, so per the preregistered rule the verdict is "not supported, mixed," not a one-sided rejection; it also stops citing the (differently-scoped) H-C-vs-V0 LOO figure for this gate and softens an overclaimed "provably" down to the exhaustive corpus check it's actually based on. Full writeup is in the audit doc.A second review accepted this revision (
ae8a552) as the final one: prereg adherence, implementation fix, regression coverage, the corpus rerun, result accounting, and interpretation all pass; no further experimental round is needed.Validation
cargo fmt --all -- --checkcargo test --release --locked --offline -j 1cargo clippy --all-targets --locked --offline -j 1 -- -D warnings -A clippy::too_many_lines -A clippy::items_after_statements9e53e55a19cddf29f05363d/dcdbc36/fd04554/ae8a552) separately passescargo fmt --all -- --check, the fullgriff-constraint-labtest suite (172 tests, incl. 2 new differential regressions), the clippy invocation above, a release build offingering_gap, and this repo's GitHub Actions run on the current headae8a552(green, 14/14, run35936064204)technique-origin-statererun against the real 410-file corpus completed in ~6m (runtime_ms: 366257) with no census/fingerprint drift