Skip to content

perf(hermitcrab): keep the required-slot look-ahead testing behind slots whose unmatched parts are unbounded runs - #579

Open
johnml1135 wants to merge 1 commit into
perf/hc-ordered-slot-lookaheadfrom
perf/hc-impure-slot-lookahead
Open

johnml1135 wants to merge 1 commit into
perf/hc-ordered-slot-lookaheadfrom
perf/hc-impure-slot-lookahead

Conversation

@johnml1135

@johnml1135 johnml1135 commented Oct 28, 2026 •

Copy link
Copy Markdown
Collaborator

Start here: src/SIL.Machine.Morphology.HermitCrab/RequiredSlotMaterial.cs, RestoredSegments and the purity test that calls it. Review only the top commit (faa2499); earlier commits are #575 and #576.

Status: open since 2026-10-28, stacked on #576 (open), which stacks on #575 (open). CI green.

What it does. The required-slot look-ahead used to stop testing at any slot whose rule copies a part that may match nothing. Mbugwe's vowel-fusion allomorphs copy OptionalSegmentSequence runs with min 0 and max -1, so every object and subject slot there stopped the look-ahead. RestoredSegments now counts exactly how many segments AnalysisMorphologicalTransform.Untruncate writes back for an uncaptured part, so an unbounded run counts as zero and the slot is pure again. On Mbugwe that is 35.58% less work than #576 alone. Parses are unchanged.

The unknown you arrive with: is "restores nothing" actually true? RestoredSegments mirrors Untruncate: one for a bare segment, max(0, MaxOccur) times the children for a quantifier, the sum of the children otherwise. An unbounded quantifier is written back as nothing, so the allomorph's output is only input nodes, which is all the presence argument in RequiredSlotMaterial needs. Bounded runs, bare segments, alternations with an optional branch and ModifyFromInput stay impure as before.

Where to look

  • RequiredSlotMaterial.RestoredSegments against Untruncate: if Untruncate ever writes something for MaxOccur == -1, this is unsound.
  • AnalysisAffixTemplateFusionTests: 5 methods. With the new condition disabled the unbounded-run case fails; with bounded quantifiers ignored in RestoredSegments, 5 cases fail.
  • Apply_CopiedPartCappedAtOneThatMayMatchNothing_... replaces a test that used an unbounded ZeroOrMore (which restores nothing); it now uses Optional (max 1).
  • A part copied twice stays unordered, so only the independent presence test runs behind it.

Not here. Aweti still has about 16% of events behind an impure slot (bounded runs, bare restored segments, modification), measured before this change; #580 takes the truncating-slot part of it. The synthetic fusion grammar is the bantu-tam-fusion stress fixture added in #578.

Stack and verification. Merge order: #575, #576, this, #580. Ran earlier: HermitCrab test project, dotnet csharpier check ., comment-hygiene.ps1 on the added lines, shadow mode over every grammar (0 violations, 0 completions under a fired check). Not rerun after #522 removed the analysis memo; the measurements below predate it. The four flaky truncate-morphotactic conformance fixtures are excluded.

Next: confirm RestoredSegments matches Untruncate, then approve the top commit once #576 is in.


Reading this a year from now -- start here

Third commit of the look-ahead stack (#575 required-slot look-ahead, #576 ordered and reachable-edge tests, this, then #580), all under the RequiredSlotLookahead Off/Shadow/On switch. It follows #567; no separate issue. The count is computed once per allomorph at construction; nothing is cached and no Interlocked is added. The relation rests on Untruncate emitting nothing for MaxOccur == -1; the tests pin it.

Decisions and why

An unbounded quantifier counts as zero because Untruncate writes nothing for it. A part copied twice (kō-style) is still unordered under the existing ordered flag, so order, window and reach tests stay off behind it.

Deferred, and what would unblock it

A supply-set design (an optional restored node could stand for any required segment, so count distinct nodes) was not built here; #580 implements one for truncating slots.

Evidence

Per word, the same words in every arm; words aborted in any arm are dropped from all. Units: events plus synthesis steps, feature-structure unification calls including the look-ahead's own checks, and matcher work (traversals, arc inspections, edge unifications). Measured before #522 removed the analysis memo; not rerun. "#576" is this base without the top commit.

grammar words events + steps: #576 / this unification calls: #576 / this matcher work: #576 / this
Mbugwe 40 8.19% less / 40.85% less 1.83% less / 22.55% less 7.65% less / 30.47% less
Sena 51 45.92% less / same 5.22% less / same 8.54% less / same
Aweti 99 11.50% less / same 9.27% less / 9.27% less (0.002% more than #576) 11.67% less / same
Amharic 24 14.08% less / same 2.75% less / same 4.59% less / same
Indonesian 70 equal equal equal
conformance 1,452 1.80% less / same 1.22% less / same 2.70% less / same
fusion stress grammar 60 2.11% less / 74.59% less 33.70% MORE / 34.01% less 3.19% less / 72.94% less

Against #576 alone: Mbugwe 35.58% less work, 21.11% less unification, 24.71% less matcher work; fusion grammar 74.04%, 50.65% and 72.04% less. The other stress grammars (bantu-tam-templates, bantu-tam-templates-shared, early-lexicon-agglutinative) are unchanged.

The look-ahead's own check unifications rose 13.88% on Mbugwe (995,434 to 1,133,568) and 0.15% on Aweti (232,302 to 232,639), and fell 2.42% on the fusion grammar; all are inside the totals above. Shadow: 0 violations and 0 completions under a fired check. Parses equal master in the On and Shadow arms on every completed word.

The fusion grammar is synthetic (12 templates, 20 object and 15 subject rules, each with a plain and a kō-style fusion allomorph, 60 words) because none of the 16 stress grammars models fusion in template slots. Its 74% is not evidence about a real language; Mbugwe is. The census counters live on a separate census branch, not here.

🤖 Generated with Claude Code


This change is Reviewable

…hed parts are unbounded runs, which restore no segments.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
johnml1135 added a commit that referenced this pull request Oct 28, 2026
R1 called a slot impure when a copied part may match nothing. An
unbounded run (max=-1) restores no segment in Untruncate, so Mbugwe's
vowel-fusion slots were treated as impure and the look-ahead stopped
testing behind them. PR #579 (stacked on #576) makes the purity test
exact against Untruncate. Row 48 of the ledger and a section in the
2026-10-27 round record the result: Mbugwe events plus synthesis steps
8.19% less -> 40.85% less than master, unification 1.83% less ->
22.55% less, matcher 7.65% less -> 30.47% less. Parses are equal to
master; shadow shows 0 violations.

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

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.73%. Comparing base (28cbee1) to head (faa2499).

Additional details and impacted files
@@                        Coverage Diff                         @@
##           perf/hc-ordered-slot-lookahead     #579      +/-   ##
==================================================================
+ Coverage                           74.64%   74.73%   +0.09%     
==================================================================
  Files                                 454      454              
  Lines                               38716    38730      +14     
  Branches                             5370     5374       +4     
==================================================================
+ Hits                                28900    28946      +46     
+ Misses                               8634     8603      -31     
+ Partials                             1182     1181       -1     

☔ 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.

{
var templates = new Dictionary<AffixTemplate, string[]>
{
[TemplateOf(Slot(false, Suffix("ti")), Slot(true, FusionPrefix("fuse", "k", "a")))] = new[]

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.

Minor: F1. No test fails if Untruncate starts writing back a segment for an unbounded run, even though the PR says the tests pin that. fuse unapplies with an empty run on no test word (ksagti gives run ti; the rest never match k X a X), so Off and On agree whatever is restored. Add a word like ktat with an inner Suffix("i"), so that the Off arm gains a parse a restored segment would allow and On would prune. Unverified: I reasoned this out and did not mutate Untruncate and run the tests.

{
if (!HasSegmentConstraint(part))
continue;
if (RestoredSegments(part) == 0)

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: F2. This skip comes before !copied.Contains, so a slot that deletes an unbounded run (lhs 1=[Seg]+, 2=[Seg]*, copies only 1) is now pure too, not just one that copies it. That is sound because Untruncate writes nothing for it, but every new test copies all of its unbounded parts, so this case has no test.


// Like Mbugwe's object markers: a prefix whose stem vowel is copied twice, followed by an unbounded run
// that may be empty.
private AffixProcessRule FusionPrefix(string name, string before, string between) =>

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: F3. Every newly pure slot in these tests is unordered, because FusionPrefix copies part 1 twice. Behind a newly pure ordered slot (copy 1, copy 2, insert z), the order, reach and window tests now run where they could not before, and nothing covers that.

@@ -429,6 +430,8 @@ private static bool IsPure(AffixProcessAllomorph allomorph)
{
if (!HasSegmentConstraint(part))

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: F4. !HasSegmentConstraint(part) is now redundant: a part with no segment constraint already has RestoredSegments(part) == 0. HasSegmentConstraint has no other caller, so both the line and the method can go.

@claude

claude Bot commented Oct 28, 2026

Copy link
Copy Markdown
Contributor

Review of the top commit (faa2499), as the description asks; #575 and #576 are out of scope here.

  1. Verdict: approve with fixes. RestoredSegments matches AnalysisMorphologicalTransform.Untruncate (MorphologicalRules/AnalysisMorphologicalTransform.cs:228): a MaxOccur == -1 loop writes nothing back, so an allomorph whose parts all restore 0 segments only copies input nodes and inserts its own. That makes the purity claim sound.
  2. Most important: F1 (AnalysisAffixTemplateFusionTests.cs:193). The soundness argument depends on Untruncate writing nothing for an unbounded run, but no test word unapplies the fusion slot with an empty run, so nothing would fail if that changed.
  3. Counts: 0 Critical, 1 Important, 3 Low.
  4. Ran: gh pr checks 579: both Build jobs (ubuntu-22.04, windows-latest) pass; Create NuGet package and claude-review are pending. I read Untruncate, IsPure, MinSegments and the new tests, traced every test word through FusionPrefix by hand, and grepped src/ to confirm HasSegmentConstraint has no caller outside IsPure.
  5. Not verified: F1 is unverified. I could not create a scratch worktree here, so I did not mutate Untruncate and run the tests. I also did not reproduce the census figures. The commit is Claude co-authored, but the description reports no ./local_check.sh --agent-strict run, and the green advisory Comment hygiene check does not stand in for it.

Public API, target framework, package, machine.py parity: None verified. All changes are private members of RequiredSlotMaterial, behind the existing RequiredSlotLookahead switch.

Findings: F1 new, unverified; F2 new; F3 new; F4 new.

Reviewed at faa2499

johnml1135 added a commit that referenced this pull request Oct 28, 2026
…he worst case for the supply-set look-ahead stacked on #579.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@johnml1135
johnml1135 requested a review from jtmaxwell3 October 28, 2026 18:08
johnml1135 added a commit that referenced this pull request Oct 29, 2026
…he worst case for the supply-set look-ahead stacked on #579.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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