Skip to content

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

Open
johnml1135 wants to merge 1 commit into
masterfrom
perf/hc-allomorph-loop-cutoff-pr
Open

johnml1135 wants to merge 1 commit into
masterfrom
perf/hc-allomorph-loop-cutoff-pr

Conversation

@johnml1135

@johnml1135 johnml1135 commented Oct 27, 2026 •

Copy link
Copy Markdown
Collaborator

Quick summary

Synthesis now stops trying an affix rule's allomorphs once it passes the one analysis recorded. Parses are identical on every corpus measured; only work whose output was always discarded is removed. No public API changes. Only SynthesisAffixProcessRule.Apply and an internal Word accessor change.

Where to look

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

Deliberately not included

Validation

  • dotnet test tests/SIL.Machine.Morphology.HermitCrab.Tests -c Release -- master 151 passed; branch 154 passed, 0 failed
  • dotnet csharpier check . and pwsh scripts/comment-hygiene.ps1 -- clean
  • ./local_check.sh --agent-strict not run end to end

Issue / porting context

Research: #490 (experiment E1).


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.

Measured effect (steps, never time)

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. The gain is largest where an affix has a long list of environment-conditioned allomorphs (Turkic suffix sets, Bantu class prefixes). 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.

Pre-existing failures (not on master)

The full HermitCrab test project has no failures on this branch or on origin/master (c245476). 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 FieldworksProducibilityLedgerTests (missing AllomorphNotApplicable, a merge interaction of #480 with #548). 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>
recordedIndex = _rule.Allomorphs.IndexOf(recorded);
for (int i = 0; i < _rules.Count; i++)
{
if (recordedIndex >= 0 && i > recordedIndex)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: F1. The cutoff and input.IsAllomorphApplicable(allo) (line 205) now decide the same thing, each reading _allomorphApps[_mruleAppIndex] separately. If IsAllomorphApplicable is later loosened (e.g. to accept free-fluctuation variants), the break silently drops output it would allow. Consider deriving one from the other, e.g. recordedIndex < 0 || i == recordedIndex at line 205.

@claude

claude Bot commented Oct 27, 2026

Copy link
Copy Markdown
Contributor
  1. Verdict: approve. The cutoff is sound: since Filter synthesis by the allomorphs used #548 output is only added when IsAllomorphApplicable matches the recorded allomorph by reference, so no allomorph after recordedIndex could reach output, and the earlier ones still run, keeping the disjunctive break and appliedAllomorphIndices intact.
  2. Most worth a look: F1, SynthesisAffixProcessRule.cs:146. The new break and IsAllomorphApplicable encode the same rule separately and could drift.
  3. Counts: 0 Critical, 0 Important, 1 Low.
  4. Ran: read the diff against c2454760 plus Word.cs (_allomorphApps copied in the copy constructor and through ExpandAlternatives) and AnalysisStratumRule.cs:142; gh pr checks 560: Build on ubuntu-22.04 and windows-latest still pending, comment hygiene (advisory) passed.
  5. Not verified: I did not run mutations myself. The PR says two of the three new tests fail with the break disabled. By reading, EarlierEnvironmentFreeAllomorphStillBlocksLaterOnes guards the earlier-allomorph path, not the cutoff. All three new tests trace, which turns off MergeEquivalentAnalyses, so the merged path is covered only by the existing untraced tests.

Public API, target framework, package, machine.py parity: None verified. CurrentRecordedAllomorph is internal. Trace output drops entries for allomorphs after the recorded one; the PR description says so.

Findings: F1 new.

Reviewed at f902cf8

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

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

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    38348       +6     
  Branches     5284     5287       +3     
==========================================
+ Hits        28593    28679      +86     
+ 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.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants