Repository navigation
Require slot literals in order and at reachable edges in the template look-ahead - #576
johnml1135 wants to merge 1 commit into
Conversation
…lking a template The required-slot look-ahead now needs each rule's inserted segments to match distinct segments in the order the rule inserts them, a slot that inserts only before or after its copied parts to find that material within the reach of the outer slots' own edge material, and the material of a required ordered slot to lie after the prefix and before the suffix of the required slots that unapply first. Slots that can reorder, repeat or remove from the middle switch the order and edge tests off for the slots behind them. Closes #567 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| } | ||
| else | ||
| } | ||
| if (segment && !node.Annotation.Optional) |
There was a problem hiding this comment.
Minor: F1. Untested soundness rule: no test puts an optional node (an unapplied deletion) in the shape, yet before/after leave such nodes out of the reach. If that exclusion breaks, a required prefix behind an optional node is pruned in On mode and a valid parse is lost; every fixture in AnalysisAffixTemplateOrderTests uses plain Segment(form) shapes, so nothing would fail. Add an Off/On/Shadow equality case with a deletion rule ahead of the template.
| return false; | ||
| any = true; | ||
| newLo = Math.Min(newLo, allomorph.Pre > 0 ? prefixEnd : walk.Lo); | ||
| newHi = Math.Max(newHi, allomorph.Post > 0 ? suffixStart : walk.Hi); |
There was a problem hiding this comment.
FYI: F2. Only the left side of the window is tested. Apply_InnerMaterialOnlyInsideTheOuterPrefix_PrunesAtEntry covers Lo, but no case has an outer required suffix narrow Hi, or exercises the backward pass stopping at forwardEnd/walk.Lo (line 234). An off-by-one there would go unnoticed. The mirror case, inner material only inside the outer suffix, would cover it.
| foreach (int[] requirements in summary.Allomorphs) | ||
| // The window is only narrowed for a slot that has slots after it in the walk, and that needs every | ||
| // passing allomorph, not the first. | ||
| bool narrow = walk.Ordered && summary.IsOrdered && slot > 0; |
There was a problem hiding this comment.
FYI: F3. Every ordered required slot above 0 now runs Embeds over all its allomorphs, not just the first that passes, even when no required, unfired slot behind it will read the new Lo/Hi (all optional, fired, or past an impure slot). That may be part of the 3.5% rise in Aweti check-only unifications noted in the description. Narrowing only when a later slot will be tested would avoid it.
|
|
||
| // Once a slot that can reorder nodes has been walked the order of the shape says nothing about the order of | ||
| // the shape the slot unapplies to, so each constraint is only looked for on its own. | ||
| private static bool AllPresent(AllomorphSummary allomorph, Shape shape, ref long unifications) |
There was a problem hiding this comment.
FYI: F4. The per-call cache of distinct constraints (known/present over summary.Distinct) is gone. So AllPresent rescans the shape for the same constraint once per allomorph that carries it, where before it scanned once per call. The cache still applies on this path, because order plays no part in it.
| return false; | ||
| } | ||
|
|
||
| // The ordinals are those of the shape, margins excluded. A node is counted against the reach only when it |
There was a problem hiding this comment.
FYI: F5. Embeds restates the analysis matcher settings it relies on: anchored at both ends, a segment-only filter, optional nodes skippable. Nothing ties it to AnalysisAffixProcessRule and AnalysisRealizationalAffixProcessRule (AnchoredToStart/AnchoredToEnd/Filter). If those change, the edge test silently drops parses. A test asserting those settings, or a <see cref> to them here, would keep the two together.
| AddRequirement(requirements, insertContext.SimpleContext.FeatureStruct); | ||
| } | ||
| int added = sequence.Count - before; | ||
| if (first < 0 || (i > first && i < last)) |
There was a problem hiding this comment.
FYI: F6. An insert between copies that adds no segment constraint (for example a boundary-only InsertSegments, added == 0) still sets infix. That makes the reach Unbounded and switches off the edge tests for this slot and every slot behind it. This is sound, but it gives up pruning for no reason. Guard with added > 0.
|
Verdict: approve with fixes. I found no defect that loses a parse. Earliest-match from the left and latest-match from the right give sound Lo/Hi bounds, and the reach sums are upper bounds. Most important: F1. Nothing tests the rule that keeps optional nodes out of the edge reach ( Counts: 0 Critical, 1 Important, 5 Low. Ran: Not verified: the corpus figures (Sena, Aweti, Amharic, Mbugwe, stress grammar) and the shadow runs, which depend on the Public API / target framework / package / machine.py parity: None verified. Every change is internal to HermitCrab. Findings: F1 new, F2 new, F3 new, F4 new, F5 new, F6 new. Reviewed at 28cbee1 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## perf/hc-required-slot-lookahead #576 +/- ##
===================================================================
- Coverage 74.65% 74.64% -0.01%
===================================================================
Files 454 454
Lines 38573 38716 +143
Branches 5338 5370 +32
===================================================================
+ Hits 28796 28900 +104
- Misses 8601 8634 +33
- Partials 1176 1182 +6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Rows 43 and 44 move from in progress to done. R1 on #546 is draft PR #577 (commit bbb81a0, shadow 0 violations); R3 ordered look-ahead is draft PR #576 (shadow 0 violations). Adds the round-doc section for both and the PanGloss port issues (sillsdev/PanGloss #65, #66). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…for the impure-slot look-ahead stacked on #576. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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>
…for the impure-slot look-ahead stacked on #576. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Start here:
src/SIL.Machine.Morphology.HermitCrab/RequiredSlotMaterial.cs-- the class remarks (Presence, Order) hold the soundness argument; the rest of the PR is wiring.Status: stacked on #575 (open); merge #575 first. All CI jobs pass.
The affix-template look-ahead from #575 now also requires a rule's inserted segments to appear in the order the rule inserts them, a slot that inserts only before (or after) its copied parts to find that material near the word edge once the outer slots have taken theirs, and the material of required slots to lie in slot order. Parses are unchanged; slots that can reorder, repeat or remove from the middle switch these tests off for the slots behind them.
Worth knowing before reading: it is a tightening of the #575 prune, not a new pass. The diff against #575 is
RequiredSlotMaterial.cs, 4 changed lines inAnalysisAffixTemplateRule.cs, and a new test class; the question worth your time is whether the order and edge conditions can wrongly prune a real parse.Where to look
IsOrderedis the per-rule order-preserving flag; theSlotWalkLo/Hiwindow bounds where later material may sit.AnalysisAffixTemplateOrderTests: cases go red when the condition is removed for copies out of order, a part copied twice, an infix, optional slots in the left and right reach, circumfix overlap, and theLo/Hiwindow (deleted or inverted).Morpher.RequiredSlotShadowandRequiredSlotCheckObserverare unchanged and null by default; noInterlockedwas added.Not here: Mbugwe's object-marker and subject slots are impure (vowel-fusion allomorphs restore segments), so the walk still stops testing at them. That needs a purity extension, not more ordering.
Verification: the HermitCrab test project (Release), shadow runs, the conformance runner,
dotnet csharpier check .andpwsh ./scripts/comment-hygiene.ps1were run when this was written; none was re-run for this description. Conformance runner: same 4 known failures (truncate-morphotactic family) on base and branch. Nothing red. Work figures are in steps, in Evidence.Next: review
RequiredSlotMaterial.csafter #575 lands, or tell me which condition to split out.Reading this a year from now -- start here
#575 prunes an affix-template partial when a required slot cannot find its material anywhere. This PR makes that test stricter: material must be in the right order and near the right word edge. The measured gain over #575 is below; the unification counters that produced it were never merged and live only on the research branch
research/hc-ordered-census@4bb687f0. Issue: #567.Evidence -- shadow, parity, work counts
Surprising findings -- the cost side
Aweti check-only unifications (the look-ahead's own calls) rose 3.5% against #575 (232,302 vs 224,347), though its total unification work fell 5.0%. Check-only counts fell on Mbugwe, Sena, Amharic, conformance and the stress grammar.
Deferred -- impure slots
Mbugwe's object-marker and subject slots stay untested because vowel-fusion allomorphs restore segments. Unblocking needs a purity extension to the walk.
🤖 Generated with Claude Code
This change is