Repository navigation
Prune reduplication matches whose copies cannot agree - #519
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## fix/hc-strrep-unapplication #519 +/- ##
===============================================================
+ Coverage 74.46% 74.57% +0.11%
===============================================================
Files 456 456
Lines 38372 38472 +100
Branches 5271 5297 +26
===============================================================
+ Hits 28573 28692 +119
+ Misses 8634 8617 -17
+ Partials 1165 1163 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…not agree Port of sillsdev/machine#519. Morpher::with_prune_disagreeing_copies (off by default) skips an affix-process analysis match when two copies of an unmodified input part differ in length or fail to unify. Copies with an optional node, a failed or zero-width capture, or a modified part are kept. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Matches sillsdev/machine#519, which now defaults PruneDisagreeingCopies on. with_prune_disagreeing_copies(false) restores the old search. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Pushed 2e1eb06: The same change in PanGloss (Rust) was measured against v0.4.0 on words both builds finish, with identical analyses:
All runs used 🤖 Generated with Claude Code |
…letion bug 049 records the reduplication copy-agreement prune ported from sillsdev/machine#519, its soundness argument, and the v0.3.3/v0.4.0 parity and timing evidence. 050 records sillsdev/machine#520, reproduced in both engines. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2e1eb06 to
d73fc10
Compare
|
I don't understand why MultiplePatternRule doesn't already filter this, given that PatternRule does. What is synthesis doing different from analysis? I noticed that AnalysisAffixProcessRule doesn't propagate the SyntacticFeatureStruct through during Apply the way that SynthesisAffixProcessRule does. Is this the issue? Or is the issue that we don't have the stem features during analysis? If we changed things so that analysis filters the way that synthesis does, that would be a more general solution. [Update] Synthesize doesn't filter the bad reduplication until the very end, when the output is checked against the original word. So it isn't filtered by PatternRule. |
|
I don't know if it is worth it, but I don't think that you need to treat matches with optional nodes as undecidable. Instead, you could non-deterministically match the two lists of nodes. Compare the first nodes. If they match, increment both indices, add 1 to the number of matches, and try again. If either of them is optional, then increment its index and try matching again. When you get to the end of the lists, compare the number of matches against the number of matches allowed by the part (e.g. 2, 0+, 1+, etc.). If at any point the number of matches exceeds the number of matches allowed, then stop. This is exponential in the number of optional nodes, but you can make it O(n^2 * min(n, max #matches allowed)) if you cache on (index1, index2, #matches). |
19a82cc to
21c662b
Compare
|
I don't think that it is necessary to turn pruning off when tracing. We don't record rules that don't match when tracing, and pruning reduplication matches is just an extension of this. If Try A Word is too slow, then it isn't useful. I turned MergeEquivalentAnalyses off when tracing because it hides a wide class of failures. I don't think that it is a good analogy for pruning reduplication matches. |
|
You no longer need a separate DisagreeingCopiesPruningRule. It appears wherever MultiplePatternRule used to appear. You can move the changes into MultiplePatternRule and delete DisagreeingCopiesPruningRule. |
Your update is right: nothing compares the copies until synthesis checks its output against the original word, so |
It is worth it; 81ff2fe does it. One change from your version: with Measured against the previous rule (copies with optional nodes always kept), analyses compared by morpheme sequence:
|
Agreed: a pruned copy match is a pattern that didn't match, and the trace already leaves those out. 425f1f3 removes the tracing exemption; |
Done in 425f1f3, through the pattern rather than
Validation: Release build clean; all four test projects pass (HermitCrab 131/131 at 81ff2fe); CSharpier and strict comment hygiene clean. Making the predicate accept every match fails all six pruning tests, which cover the affix, realizational and compounding paths and tracing. |
This comment has been minimized.
This comment has been minimized.
81ff2fe to
15b9596
Compare
15b9596 to
9d61200
Compare
This comment has been minimized.
This comment has been minimized.
…nstead of sharing one instance Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ous application modes Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…rewrittenForms covering a {t,k} -> d StrRep lhs in both application modes
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
When unapplying an affix process rule that copies a part more than once, Morpher.PruneDisagreeingCopies (off by default) skips matches whose copies cannot unify segment by segment. Synthesis writes every copy from the same input, so such a match never survives synthesis. Copies containing an optional node or modified by the rule are always kept. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A self-feeding deletion strips two segments from the second copy only; with DeletionReapplications 0 analysis restores one. Forward generation defines the expected parse, and pruning must return the unpruned result. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Rules that copy no part twice no longer rescan their captured parts on every unapplication. Mirrors PanGloss 77aca87f, which measured 0.62-0.64x of the prior time on Aweti with identical analyses. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- Prune compounding rules as well as affix-process rules. - Compare every pair of copies, since unifiability is not transitive. - Skip pruning while tracing, as MergeEquivalentAnalyses does, so traces still show each doomed split failing in synthesis. - Collapse the SkippedOptionalNodes forwarder into a protected internal static GetSkippedOptionalNodes; split HasDisagreeingCopies into helpers. - Rename to DisagreeingCopiesPruningRule / PruneDisagreeingCopiesTests. - Add tests: realizational and compounding paths, three copies, parallelism and memo settings, partial morphemes with and without AlwaysEnforceFinalTemplates, DeletionReapplications = 2, tracing, and a counterbleeding opacity case pinned against forward generation. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The copy check is now the analysis rule spec's Pattern.Acceptable, so the three analysis rules use MultiplePatternRule unchanged and DisagreeingCopiesPruningRule is deleted. Affix-process specs set the predicate only when a part is copied, so other rules pay nothing; the compounding spec combines it with its existing head-capture check. Tracing no longer turns pruning off: a pruned match is a pattern that did not match, which a trace already leaves out. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
A copy containing an optional segment was always kept. Now copies are compared by alignment: an optional segment may stand for any number of segments of the other copy it unifies with, including none, and the copies disagree only if no alignment exists. A single optional node can stand for several segments because DeletionReapplications (0 by default) lets analysis restore fewer segments than a deletion removed; matching it to at most one segment pruned the valid parse in CapLimitedDeletionInOneCopyParsesTheSameWithAndWithoutPruning. Copies without optional nodes keep the linear comparison. Optional nodes that are not segments still make a copy undecidable. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…s and wire the morpher through an internal property The pruning change had altered the protected constructor of AnalysisMorphologicalTransformRuleSpec, added an optional morpher parameter to the public AnalysisAffixProcessAllomorphRuleSpec and AnalysisCompoundingSubruleRuleSpec constructors, and turned GetSkippedOptionalNodes into a protected internal static. Those are back to their master form; the rules now set an internal PruningMorpher property and the pruning code calls an internal static CollectSkippedOptionalNodes. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…ional segments and empty copies with exact expected analyses Each new test asserts the exact analysis forms produced with pruning off and on. WordInitialOptionalNonSegmentIsNeverPruned fails if the skipped word-initial nodes are dropped, OptionalNonSegmentInACopyIsNeverPruned fails if the optional non-segment bailout is removed, and EmptyCopyIsNeverPruned covers the unsuccessful-capture bailout; with the existing tests, TryGetSegmentCopies is now fully line and branch covered. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…nstall the copy-agreement predicate only then The base now records HasRepeatedParts once at construction and exposes CopyAgreementCheck, which is null when nothing repeats. The affix spec and the compounding spec both consume it, so the per-spec LINQ re-evaluation and the third check inside CopiesMayAgree are gone; behaviour is unchanged. This also drops a stray byte-order mark the earlier commit had added to AnalysisCompoundingSubruleRuleSpec.cs. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…ut pruning Depends on #538's literal-rewrite unapplication (fix/hc-strrep-unapplication). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ply the rewrite for pruning to keep the parse Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…cause the rule also modifies it Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
960a710 to
ebd3a38
Compare
76cbee7 to
394e730
Compare
This comment has been minimized.
This comment has been minimized.
HasRepeatedParts and HasDisagreeingCopies each restated which parts are compared (at least two copies, not modified). Both now read one list, so the predicate's installation and its loop cannot disagree again. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Public API: one addition, Findings: F1 addressed (e55c37b); F2 addressed (06d2782), mutants unverified; F3 addressed (68700d2); F4 addressed (76cbee7, via #538); F5 addressed (d55b65a); F6 addressed (394e730); F7 addressed (c8860f0). Reviewed at c8860f0 |
#519 landed on master with tests that set Word.AnalysisScope, which this branch removes along with the memo. The transform does not read it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
* refactor: remove analysis cascade memo Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test: remove analysis memo coverage Adds focused AnalysisMergeKey regressions covering non-head count and per-rule unapplication counts, the two merge-key dimensions the deleted memo tests exercised. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs: remove obsolete memo review guidance Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Add FinalTemplateState to AnalysisMergeKey The merge key dropped FinalTemplateState, which AnalysisStateKey carried. Two analyses that differ only in it (unapplying a template rule last leaves None; unapplying a non-template rule last leaves NonTemplate) were merged, and the canonical's NonTemplate then blocked a final template in a lower, non-blocking stratum that the merged-away None alternative would have allowed, losing the parse. Add a parse test that fails without the key change, and a key test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Restore single-threaded vs parallel parity tests The sequential CombinationRuleCascade (maxDegreeOfParallelism 1) is still a distinct code path, and only the compounding grammar compared it with the parallel default. Add comparisons for affix templates, a Linear stratum, and Simultaneous epenthesis (buibui parses to one analysis), without the memo hit-counter assertions. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Test AnalysisMergeKey order invariance Two words with the same multiset of unapplied rules reached in different orders must produce equal keys and equal hashes; that is what the XOR multiset combine exists for. Replacing it with a rolling combine now fails this test, and previously passed every AnalysisMergeKey test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Document that AnalysisMergeKey freezes the syntactic feature struct The constructor mutates its argument by freezing SyntacticFeatureStruct, which Word.FreezeImpl leaves unfrozen; a plain constructor does not signal that, so say so at the constructor. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Make the compounding and Linear-stratum parity tests assert non-empty parses The compounding test compared two empty results for pʰutdidat, and the Linear test compared an empty digusagd; both now assert the words that really parse and say which word does not. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Make the review checklist and MaxDegreeOfParallelism doc match the code The hermitcrab checklist lists exactly the AnalysisMergeKey fields, MaxDegreeOfParallelism is documented again as a results-neutral performance knob, and the merge-order test comment is stated in terms of AnalysisMergeKey. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Assert pʰutdat parses in the parallelism-cap test so it cannot pass vacuously The parallelism-cap test compared capped and unbounded results only, so two empty results for pʰutdat would pass. It now asserts the unbounded parse of pʰutdat is non-empty, matching the compounding parity test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Caveat MaxAlternatives in the MaxDegreeOfParallelism doc The doc said every value produces equal parse results. With MaxAlternatives set the parallel cascade counts alternatives before removing duplicates, so it can throw where a value of 1 parses. The doc is a rule again, as on master, and names that exception. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Port the syntactic-feature and unfrozen-word key tests to AnalysisMergeKey AnalysisStateKeyTests covered both behaviors; the port dropped them, so removing either from AnalysisMergeKey passed every test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Pin gudisagd in the Linear-stratum parity test On a Linear stratum the di and gu prefixes still both apply, but only in rule order, so gudisagd parses and digusagd does not. The test and the fixture comment said neither nesting parsed; the test now asserts gudisagd is non-empty and compares it across both cascades. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Count distinct results toward MaxAlternatives in parallel cascades ParallelCombinationRuleCascade and ParallelPermutationRuleCascade added every rule output to the count before removing duplicates, so with MaxAlternatives set they could throw where the sequential cascades, which count a set, do not. Collect output in a set and count only new results. The MaxDegreeOfParallelism doc no longer needs the exception. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Drop AnalysisScope from the copy-pruning tests #519 landed on master with tests that set Word.AnalysisScope, which this branch removes along with the memo. The transform does not read it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Default a null comparer in RuleCascade The parallel cascades now build a ConcurrentDictionary from Comparer, whose netstandard2.0 constructor rejects null; before, Distinct(null) fell back to the default. ParallelCombinationRuleCascade also called Comparer.Equals directly, so a null comparer already crashed there. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Quick summary
HermitCrab now drops reduplication analyses whose copies provably disagree, since synthesis would reject them anyway.
Copies are lined up across unapplied deletions; anything it cannot judge, such as a part the rule modifies, is kept.
Synthesis is untouched; the new
Morpher.PruneDisagreeingCopiesoption defaults to on, covers affix-process,realizational and compounding rules, and also applies while tracing.
Where to look
AnalysisMorphologicalTransform.HasDisagreeingCopies/CanAlign-- the invariant: an undecidable copy is always kept,and an optional segment may stand for any number of segments it unifies with.
AnalysisMorphologicalTransformRuleSpec.CopiesMayAgree-- installed as the spec'sPattern.Acceptable, so the rulesuse
MultiplePatternRuleunchanged.PruneDisagreeingCopiesTests-- split selection, on/off pruning, on-by-default, all three rule paths,pairwise comparison of three copies, optional segments that do and do not line up, parallelism/memo settings,
partial morphemes with final templates, tracing, and cap-limited-deletion and counterbleeding-opacity cases where forward generation defines the
expected parse.
AffixProcessRuleTests.ReduplicationRules/ModifyFromInputRules-- run with pruning both off and on.Deliberately not included
The earlier figures are kept below the rule, labelled as prior measurements.
Validation
dotnet build -c Release-- Build succeeded, 0 Warning(s), 0 Error(s).dotnet test-- all four test projects pass; HermitCrab Passed: 131, Failed: 0, Skipped: 0.csharpier check .(1.2.6) -- Checked 721 files, clean.pwsh -NoProfile -File scripts/comment-hygiene.ps1-- clean.Mutants: accepting every match fails the six pruning tests across affix, realizational, compounding and
tracing; aligning everything fails
OptionalSegmentThatCannotLineUpIsPruned; letting an optional segmentstand for at most one segment fails
CapLimitedDeletionInOneCopyParsesTheSameWithAndWithoutPruning.Corpus parity against the previous rule (copies with optional nodes always kept), analyses compared by
morpheme sequence, 20 s cap per word:
Reading this a year from now
A full-copy reduplication rule (
CopyFromInputof the same part twice) is unapplied by matching eachcopy as an independent capture; only the first capture rebuilds the base. So every split where the
inserted material lines up becomes its own analysis, and synthesis throws the disagreeing ones away much
later. This change moves that rejection earlier, into analysis, for matches that can be judged.
Synthesis writes every copy of a part from the same input, and anything that later changes one copy is
unapplied before this rule runs, so a match whose copies are proven to disagree segment by segment can
never survive synthesis. An unapplied deletion leaves an optional segment, and with
DeletionReapplicationsat its default of 0 one optional segment can stand for several deleted ones, so copies are lined up with each
optional segment matching any number of segments it unifies with, including none. A part the rule modifies,
or an optional node that is not a segment, cannot be judged and is never reported as disagreeing -- it is
kept, at the cost of the candidates the prune would otherwise have removed.
Decisions, and why
Pattern.Acceptablerather than aMultiplePatternRulesubclass or a changeto
MultiplePatternRuleitself, which is genericSIL.Machine.Rulescode. WithAllSubmatches = truethematcher drops exactly the matches a post-filter would. Affix-process specs install it only when a part is
copied, since the matcher builds a
Matchfor every candidate it tests.leaves out.
Consistent/Inconsistent/Undecidableenum. Nocaller or test distinguished
ConsistentfromUndecidable-- the pattern rule only everchecked for
Inconsistent, and both other states took the same keep path -- so it was collapsed to asingle
HasDisagreeingCopiesbool, removing the enum and the now-unreachablebranching that tracked the discarded state.
MorphologicalOutputAction.GetSkippedOptionalNodesbecameprotected internal staticso the copycheck reuses the same optional-node walk; existing derived callers compile unchanged.
Morpher.PruneDisagreeingCopies's summary no longer promises a specific memory/timing outcome; thecontract is what the option controls (skip provably-disagreeing copies) and its default, not a
particular grammar's measured speedup.
AnalysisAffixProcessAllomorphRuleSpecandAnalysisCompoundingSubruleRuleSpectake theMorpheras a newoptional constructor parameter (null means no pruning). Existing source compiles unchanged; callers compiled
against the old constructors need a rebuild. Nothing in this repository constructs them outside the three
analysis rules.
Prior measurements (reported on an earlier revision of this branch, not reverified this session)
Grammars without full-copy rules (Sena, Amharic) never enter the new code path. The unpruned parse of
Aweti index 182 never finishes, so its 2 analyses rest on the argument above plus the parity sweep, not a
direct comparison. Mbugwe gains little from this change alone (the prune fires on 161 words but removes
0.2% of candidates).
🤖 Generated with Claude Code
This change is