Skip to content

Prune reduplication matches whose copies cannot agree - #519

Open
johnml1135 wants to merge 5 commits into
masterfrom
perf/copy-agreement-prune
Open

johnml1135 wants to merge 5 commits into
masterfrom
perf/copy-agreement-prune

Conversation

@johnml1135

@johnml1135 johnml1135 commented Sep 24, 2026 •

Copy link
Copy Markdown
Collaborator

Quick summary

HermitCrab now drops reduplication analyses whose copies provably disagree, since synthesis would reject them anyway.
Anything it cannot judge, such as an unapplied deletion inside a copy, is kept.
Synthesis is untouched; the new Morpher.PruneDisagreeingCopies option defaults to on, covers affix-process,
realizational and compounding rules, and is ignored while tracing (like MergeEquivalentAnalyses).

Where to look

  • AnalysisMorphologicalTransform.HasDisagreeingCopies -- the invariant: an undecidable copy is always kept.
  • PruneDisagreeingCopiesTests -- split selection, on/off pruning, on-by-default, all three rule paths,
    pairwise comparison of three copies, 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

  • Independent reruns of the previously reported Aweti/Sena/Mbugwe/Amharic/Indonesian corpus timing and
    parity sweep. This session only reran the unit/integration suite; prior figures are kept below the
    rule, labelled as prior measurements.

Validation

  • dotnet build -- Build succeeded, 0 Warning(s), 0 Error(s).
  • dotnet test tests/SIL.Machine.Morphology.HermitCrab.Tests -- Passed: 129, Failed: 0, Skipped: 0.
  • dotnet csharpier check . -- Checked 721 files, clean.
  • pwsh -NoProfile -File scripts/comment-hygiene.ps1 -- clean.

Reading this a year from now

A full-copy reduplication rule (CopyFromInput of the same part twice) is unapplied by matching each
copy 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. A copy containing an optional node (an unapplied deletion), or a part the rule
modifies, 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
  • The classifier was originally a three-state Consistent / Inconsistent / Undecidable enum. No
    caller or test distinguished Consistent from Undecidable -- the pattern rule only ever
    checked for Inconsistent, and both other states took the same keep path -- so it was collapsed to a
    single HasDisagreeingCopies bool, removing the enum and the now-unreachable
    branching that tracked the discarded state.
  • MorphologicalOutputAction.GetSkippedOptionalNodes became protected internal static so the copy
    check reuses the same optional-node walk; existing derived callers compile unchanged.
  • Copies are compared pairwise, not each against the first, because unifiability is not transitive.
  • Morpher.PruneDisagreeingCopies's summary no longer promises a specific memory/timing outcome; the
    contract is what the option controls (skip provably-disagreeing copies) and its default, not a
    particular grammar's measured speedup.
  • The pruning rule's class-level summary was removed: it only restated the class and property
    names, the class is internal with no external reader, and no sibling rule class in this file's
    directory carries one.
Prior measurements (reported on an earlier revision of this branch, not reverified this session)
Check Result
Aweti index 182, default settings OOM at 316 s -> 9 s, 2 analyses
Aweti whole list (206) 192 complete (was 172); 4.95x on the 172 both finish; 0 analysis-count differences
Exact analysis-signature parity, off vs on 0 divergences in 1,855 words: Sena 209, Mbugwe 562, Amharic 656, Aweti 179 x 2 exports, Indonesian 70
Real reduplicated analyses Mbugwe 759/759 accepted copy analyses preserved
Conformance suite (#480, incl. 9 new reduplication x phonology words), prune on 43/44, identical to the branch without this change; the prune fires 612 times
Mutants (prune agreeing copies / undecidable copies / everything) each fails unit tests and 4-8 conformance words

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 Reviewable

@codecov-commenter

codecov-commenter commented Sep 24, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.26027% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.42%. Comparing base (c7146cd) to head (64337cd).

Files with missing lines Patch % Lines
...rphologicalRules/AnalysisMorphologicalTransform.cs 95.83% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #519      +/-   ##
==========================================
+ Coverage   74.34%   74.42%   +0.07%     
==========================================
  Files         456      457       +1     
  Lines       38261    38331      +70     
  Branches     5242     5256      +14     
==========================================
+ Hits        28445    28526      +81     
+ Misses       8666     8656      -10     
+ Partials     1150     1149       -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.

johnml1135 added a commit to sillsdev/PanGloss that referenced this pull request Sep 24, 2026
…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>
johnml1135 added a commit to sillsdev/PanGloss that referenced this pull request Sep 25, 2026
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>
@johnml1135

Copy link
Copy Markdown
Collaborator Author

Pushed 2e1eb06: CopyAgreementPatternRule now computes HasRepeatedParts once in its constructor instead of on every unapplication. Rules that copy nothing no longer rescan their captured parts.

The same change in PanGloss (Rust) was measured against v0.4.0 on words both builds finish, with identical analyses:

Grammar Fixed / v0.4.0
Aweti 0.62x, 0.64x (two rounds)
Mbugwe 0.97x (two clean rounds)
Amharic 1.01x

All runs used AlwaysEnforceFinalTemplates off. I haven't timed the C# change separately; it removes the same per-call work.

🤖 Generated with Claude Code

johnml1135 added a commit to sillsdev/PanGloss that referenced this pull request Sep 25, 2026
…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>
@johnml1135
johnml1135 force-pushed the perf/copy-agreement-prune branch from 2e1eb06 to d73fc10 Compare September 25, 2026 22:54
@jtmaxwell3

jtmaxwell3 commented Sep 26, 2026 •

Copy link
Copy Markdown
Collaborator

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.

@jtmaxwell3

Copy link
Copy Markdown
Collaborator

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

johnml1135 and others added 4 commits September 30, 2026 14:09
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>
@johnml1135
johnml1135 force-pushed the perf/copy-agreement-prune branch from 19a82cc to 21c662b Compare September 30, 2026 18:09
- 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>
@jtmaxwell3

Copy link
Copy Markdown
Collaborator

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.

@jtmaxwell3

Copy link
Copy Markdown
Collaborator

You no longer need a separate DisagreeingCopiesPruningRule. It appears wherever MultiplePatternRule used to appear. You can move the changes into MultiplePatternRule and delete DisagreeingCopiesPruningRule.

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.

3 participants