Change Add to PriorityUnion

[jtmaxwell3]

I fixed a performance bug by changing AnalysisAffixProcessRule.Apply to use PriorityUnion instead of Add. Add acts like Union, PriorityUnion is more restrictive. Also, SynthesisAffixProcessRule uses PriorityUnion instead of Add in the other direction, so this makes AnalysisAffixProcessRule more parallel to SynthesisAffixProcessRule.

If I have a sequence of derivation rules that change the category like [CAT:A] => [CAT:B], [CAT:B] => [CAT:A], followed by [CAT:B] => [CAT:C], then the syntactic feature structure of the input of the third rule will be [CAT:{A, B}] if I use Add, and the third rule will not be filtered when it checks whether the input can unify with [CAT:B]. If I use PriorityUnion, then the syntactic feature structure of the input of the third rule will be [CAT:A], and the third rule will be filtered when it checks whether the input can unify with [CAT:B].

This performance bug only shows up when there are at least three derivational affixes. But words that are slow to parse try many long sequences of derivational affixes. Filtering these long sequences can make a big difference.

---

This change is 

[ddaspit]

It would be great if you could add a unit test to capture scenario you laid out.

@ddaspit reviewed 1 file and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on jtmaxwell3).

---

src/SIL.Machine.Morphology.HermitCrab/MorphologicalRules/AnalysisAffixProcessRule.cs line 59 at r1 (raw file):

                {
                    if (!_rule.RequiredSyntacticFeatureStruct.IsEmpty)
                        outWord.SyntacticFeatureStruct.PriorityUnion(_rule.RequiredSyntacticFeatureStruct);

This change should also apply to compounding rules as well.

[johnml1135]

I measured this on four FieldWorks grammars, 30 words each, counting the parser's steps rather than timing it, and comparing every parse by gloss, allomorph, and features.

Grammar	Feature checks	Synthesis attempts	Wall time
Mbugwe	14.7M to 4.0M	11.5M to 5.4M	340 s to 133 s
Sena	747k to 138k	340k to 70k	28 s to 8.5 s

Amharic and Indonesian: no change. Every word got exactly the same parses. All 36 conformance grammars pass, and 1,300 words generated from them parse the same as master.

One regression, with a fix. The parser merges two partial analyses whenever they have the same shape, even if they carry different feature requirements (MergeEquivalentAnalyses in AnalysisStratumRule). It then keeps working on only one of them and copies the results back to the other at the end. With the old Add, the kept one always had the looser requirements, so nothing was lost. With PriorityUnion, the kept one can be the stricter one, and a rule that was valid for the other analysis gets filtered out. A real parse disappears. The fix: when merging, loosen the kept analysis's features to cover both (FeatureStruct.Union). On Mbugwe this fires about 100k times with no change to any parse and under 1 percent change in step counts. Test included, fails without the fix.

Branch perf/pr494-proposed (one commit, 3be1eb9, on top of yours) has this fix, the same PriorityUnion change in AnalysisCompoundingRule as Damien asked, and three tests including the three-rule chain from your description. Happy to open it against this branch or you can cherry-pick.

[johnml1135]

Agreed, this is the right fix. When the parser builds a word (synthesis), it asks "does this stem match what the rule requires?" So when it takes a word apart (analysis), the stem left behind must match exactly what the rule requires. That is what PriorityUnion says. The old Add kept a running "any of these categories" list, which let obviously wrong stems through to be rejected later.

I tried hard to break this: 21 targeted unit tests, all 36 conformance grammars, and 1,300 words generated from those grammars, parsed on master and on this branch. Every word got the same parses. The one place it does bite is described in my general comment.

[johnml1135]

Not for this PR, but while we are here. When a rule has no required features, the features its output set are left sitting on the stem. So if an inner suffix marks "present" and an outer suffix marks "past", the parser sees "past" on the stem, then refuses to take off the inner suffix because it expects "present". That parse is lost today, on master and on this branch alike. Wiping the rule's output features off the stem here would find it, and in my measurements it also cuts another 30 to 45 percent of the work on Mbugwe and Sena. It needs one other change first, so: follow-up.

[jtmaxwell3]

@johnml1135: There is a bug in MergeEquivalentAnalyses that I attempt to fix in #493. The fix is based on AnalysisStateKey. I think that this is cleaner than using FeatureStruct.Union.

[jtmaxwell3]

@ddaspit: I prefer the fix to MergeEquivalentAnalyses in #493, but I defer to you.