Skip to content

Stop synthesis trying affix allomorphs after the one analysis recorded - #560

Open
johnml1135 wants to merge 2 commits into
masterfrom
perf/hc-allomorph-loop-cutoff-pr
Open

johnml1135 wants to merge 2 commits into
masterfrom
perf/hc-allomorph-loop-cutoff-pr

Conversation

@johnml1135

@johnml1135 johnml1135 commented Oct 27, 2026 •

Copy link
Copy Markdown
Collaborator

Start here: src/SIL.Machine.Morphology.HermitCrab/MorphologicalRules/SynthesisAffixProcessRule.cs -- the six added lines in Apply that end the allomorph loop at the recorded allomorph.

Status: open; CI green on this head (the claude job is skipped).

Synthesis stops trying an affix rule's allomorphs once it passes the one analysis recorded (Word.CurrentRecordedAllomorph, new and internal). Parses are identical on every corpus measured; only work whose output was always discarded is removed. No public API changes. On real grammars the saving in total work is small (0.007% on Aweti to 0.35% on Mbugwe); it is largest where an affix has a long list of environment-conditioned allomorphs (8.45% and 9.49% on Turkic- and Bantu-style stress fixtures).

The unknown a reviewer arrives with is whether skipping allomorphs can ever drop an output. It cannot: an output from an allomorph after the recorded one fails IsAllomorphApplicable and was never added (argument in the first accordion).

Where to look:

  • Soundness: later allomorphs never reach the output.
  • The three new AffixProcessRuleTests pin the cutoff and the earlier-allomorph cases: SynthesisStopsAtRecordedAllomorph, SynthesisTriesEveryAllomorphUpToRecordedLast, EarlierEnvironmentFreeAllomorphStillBlocksLaterOnes.
  • Traces no longer show NotApplied/pattern entries for allomorphs after the recorded one, since they are no longer tried.

Not here: a public on/off switch, the research harness and stress fixtures (see #490).

Verification on this branch: dotnet test tests/SIL.Machine.Morphology.HermitCrab.Tests -c Release passed with 0 failures (and none on origin/master); dotnet csharpier check . and pwsh scripts/comment-hygiene.ps1 clean. Not run: ./local_check.sh --agent-strict end to end.

Next: approve, or tell me which corpus you want measured again.


Reading this a year from now -- start here

The change is a loop cutoff in SynthesisAffixProcessRule.Apply. The research behind it is experiment E1 of #490 (open); the harness and stress fixtures stayed there, so the measurements below cannot be reproduced from this branch alone.

Soundness argument

An output from allomorph i > r fails IsAllomorphApplicable and was never added. appliedAllomorphIndices is copied into each output when it is built (UnionWith in Word.MorphologicalRuleApplied), so indices added after r never reach r's output. Allomorphs i <= r run exactly as before, so the disjunctive break and the recorded indices are unchanged. With no recorded allomorph (null, or not in the list), recordedIndex stays -1 and the loop is untouched. Compounding and realizational rules do not use recorded allomorphs and are left alone. The new tests also check that the last allomorph recorded still applies and that an earlier environment-free allomorph still blocks later ones. With the break disabled, two of the three fail.

Evidence: measured effect in steps

Synthesis steps with the cutoff off and on, same words in both arms. "Total work" is analysis events plus synthesis steps. Analysis events and candidate counts were equal in both arms on every corpus, and the parse output was identical per word.

Corpus Synthesis steps % less synthesis % less total work
Mbugwe, stride 40 151,505 to 138,737 8.43% 0.35%
Sena, stride 100 1,908 to 1,772 7.13% 0.13%
Aweti, stride 2 15,940 to 15,831 0.68% 0.007%
Amharic, limit 28 1,277 to 1,271 0.47% 0.07%
Conformance (minus 4 flaky fixtures) 8,150 to 8,109 0.50% 0.12%
Indonesian 677 to 677 0% 0%
Turkic-style stress fixture 566 to 337 40.46% 8.45%
Bantu-style stress fixture 341 to 190 44.28% 9.49%

Synthesis is a small share of total work on real grammars (1.0% on Aweti, 9.0% on Mbugwe), so the real-grammar saving in total work is small. An environment-free allomorph already ends the loop; environment-bearing ones never did, so every one was applied. Each rule application cost A allomorph applications and now costs r + 1.

Cost not in the step counters: one IndexOf over the rule's allomorphs per rule application, at most A reference comparisons.

Surprising findings: failures seen on the research branch

The HermitCrab test project has no failures on this branch or on origin/master. Failures seen in the research branch's runs came from the research branch, not this change: truncate-morphotactic* conformance fixtures (nondeterministic, fail without the cutoff) and a producibility-ledger test in the conformance suite (#480, open) that lacked AllomorphNotApplicable after #480 was merged with #548 (merged). Neither exists on master.

🤖 Generated with Claude Code


This change is Reviewable

Analysis records which allomorph of each affix rule it unapplied, and
synthesis only keeps output from that allomorph or an earlier one. Later
allomorphs were still pattern-matched and built, then discarded. The loop in
SynthesisAffixProcessRule.Apply now ends at the recorded allomorph.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@claude

This comment has been minimized.

@codecov-commenter

codecov-commenter commented Oct 27, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 74.78%. Comparing base (c245476) to head (aabf752).

Files with missing lines Patch % Lines
src/SIL.Machine.Morphology.HermitCrab/Word.cs 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #560      +/-   ##
==========================================
+ Coverage   74.57%   74.78%   +0.21%     
==========================================
  Files         453      453              
  Lines       38342    38345       +3     
  Branches     5284     5286       +2     
==========================================
+ Hits        28593    28676      +83     
+ Misses       8583     8511      -72     
+ Partials     1166     1158       -8     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Moves the cutoff inside the branch that already decides applicability, so
it and IsAllomorphApplicable cannot drift apart. Output is unchanged.
Suggested in review.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@claude

claude Bot commented Oct 29, 2026

Copy link
Copy Markdown
Contributor
  1. Verdict: approve. The cutoff now runs only after the recorded allomorph's output has passed IsAllomorphApplicable (SynthesisAffixProcessRule.cs:217). No later allomorph could pass that check, so synthesis output is unchanged.
  2. Most worth knowing: if the recorded allomorph fails during synthesis (an MPR continue or a pattern miss), the later allomorphs are tried and rejected as on master. That is the trade-off agreed in the F1 thread and given in the author's reply, not a defect.
  3. Counts: 0 Critical, 0 Important, 0 Low new. F1 (Low) is addressed.
  4. Ran: git diff f902cf87 aabf7527 (compare reports ahead); the diff is source-only and the tests are unchanged. dotnet test tests/SIL.Machine.Morphology.HermitCrab.Tests -c Release --filter FullyQualifiedName~AffixProcessRuleTests on the merge checkout: 25 passed, 0 failed. gh pr checks 560: Build on ubuntu-22.04 passed, Build on windows-latest pending, comment hygiene (advisory) passed.
  5. Not verified: that the new tests still fail with the moved break removed. My scratch-worktree mutation was not run. By reading, SynthesisStopsAtRecordedAllomorph should still catch it, because NotApplied traces for later indices are collected too.

Public API, target framework, package, machine.py parity: None verified. CurrentRecordedAllomorph is internal.

Findings: F1 addressed.

Reviewed at aabf752

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants