Skip to content

Change Add to PriorityUnion #494

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jtmaxwell3 wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Change Add to PriorityUnion #494

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/SIL.Machine.Morphology.HermitCrab/MorphologicalRules/AnalysisAffixProcessRule.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ public IEnumerable<Word> Apply(Word input)
foreach (Word outWord in _rules[i].Apply(input).RemoveDuplicates())
{
if (!_rule.RequiredSyntacticFeatureStruct.IsEmpty)
outWord.SyntacticFeatureStruct.Add(_rule.RequiredSyntacticFeatureStruct);
outWord.SyntacticFeatureStruct.PriorityUnion(_rule.RequiredSyntacticFeatureStruct);

@johnml1135 johnml1135 Sep 10, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

else if (_rule.OutSyntacticFeatureStruct.IsEmpty)
outWord.SyntacticFeatureStruct.Clear();

@johnml1135 johnml1135 Sep 10, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

outWord.MorphologicalRuleUnapplied(_rule);
Expand Down
Loading