Skip to content

Use IsCliticRule to allow clitics to apply to templates - #529

Merged
jtmaxwell3 merged 13 commits into
masterfrom
add-IsCliticRule
Oct 15, 2026
Merged

jtmaxwell3 merged 13 commits into
masterfrom
add-IsCliticRule

Conversation

@jtmaxwell3

@jtmaxwell3 jtmaxwell3 commented Oct 1, 2026 •

Copy link
Copy Markdown
Collaborator

This uses IsCliticRule to allow clitics to apply to templates instead of clearing FinalTemplateState and IsLastAppliedRuleFinal at the end of each stratum (since clitics are in a different stratum). This is necessary now that FieldWorks lets users specify the strata for performance reasons. Non-final templates and partial parses are only checked on the last stratum.

Damien agreed to something like this in an earlier discussion.

I also had to make changes at line 67 of SynthesisAffixTemplateRule and line 70 of SynthesisStratumRule to make this work.


This change is Reviewable

@jtmaxwell3
jtmaxwell3 marked this pull request as draft October 1, 2026 16:06
@codecov-commenter

codecov-commenter commented Oct 1, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 76.08696% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.36%. Comparing base (be5407d) to head (835d137).

Files with missing lines Patch % Lines
src/SIL.Machine.Morphology.HermitCrab/Morpher.cs 60.00% 2 Missing and 2 partials ⚠️
...Machine.Morphology.HermitCrab/XmlLanguageWriter.cs 25.00% 1 Missing and 2 partials ⚠️
...hine.Morphology.HermitCrab/SynthesisStratumRule.cs 77.77% 1 Missing and 1 partial ⚠️
...orphology.HermitCrab/MorphemicMorphologicalRule.cs 83.33% 0 Missing and 1 partial ⚠️
...rphology.HermitCrab/SynthesisAffixTemplatesRule.cs 75.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master     #529   +/-   ##
=======================================
  Coverage   74.36%   74.36%           
=======================================
  Files         456      456           
  Lines       38292    38325   +33     
  Branches     5251     5263   +12     
=======================================
+ Hits        28474    28499   +25     
- Misses       8667     8669    +2     
- Partials     1151     1157    +6     

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

Comment thread src/SIL.Machine.Morphology.HermitCrab/XmlLanguageWriter.cs Outdated
Comment thread src/SIL.Machine.Morphology.HermitCrab/AnalysisStratumRule.cs
Comment thread src/SIL.Machine.Morphology.HermitCrab/SynthesisStratumRule.cs Outdated
Comment thread src/SIL.Machine.Morphology.HermitCrab/MorphemicMorphologicalRule.cs Outdated
Comment thread src/SIL.Machine.Morphology.HermitCrab/MorphemicMorphologicalRule.cs Outdated
Comment thread tests/SIL.Machine.Morphology.HermitCrab.Tests/AffixTemplateTests.cs
Comment thread tests/SIL.Machine.Morphology.HermitCrab.Tests/AffixTemplateTests.cs
Comment thread src/SIL.Machine.Morphology.HermitCrab/XmlLanguageWriter.cs Outdated
Comment thread src/SIL.Machine.Morphology.HermitCrab/SynthesisStratumRule.cs
@claude

This comment has been minimized.

@claude

This comment has been minimized.

@claude

This comment has been minimized.

Comment thread tests/SIL.Machine.Morphology.HermitCrab.Tests/AffixTemplateTests.cs Outdated
Comment thread tests/SIL.Machine.Morphology.HermitCrab.Tests/AffixTemplateTests.cs Outdated
@claude

This comment has been minimized.

@claude

This comment has been minimized.

@claude

This comment has been minimized.

@jtmaxwell3
jtmaxwell3 marked this pull request as ready for review October 14, 2026 17:48
@claude

This comment has been minimized.

@johnml1135

Copy link
Copy Markdown
Collaborator

The conformance suite in #480 has a negative control this changes. In edge-cases/morphotactic-attribute-breadth, kulpu should get no parse: mrNF is reachable only through nonFinalTemplate, which is final="false". Master refuses it; at 86f2a19 it parses as KUL+NF|kulpu.

As I read it, stratum Main no longer closes the word, so stratum Second's final pogTemplate, whose only slot is optional, applies without adding anything and marks the word final. Making pogSlot required brings the refusal back.

Is a final template that adds nothing meant to complete a non-final template from an earlier stratum? If so, I'll change the fixture to expect the parse. If not, the closing check should not count a template that applied vacuously.

@jtmaxwell3

Copy link
Copy Markdown
Collaborator Author

@johnml1135 If a final template that adds nothing completes a non-final template within the same stratum, then it should also complete a non-final template in an earlier stratum. This is a change in behavior, since earlier a change in strata caused non-final templates that hadn't been completed to fail.

For background, strata were originally designed to handle languages like English that had a Germanic layer on top of a Latin/Greek layer. Latin/Greek affixes should only apply to Latin/Greek roots. The idea was that the Latin/Greek affixes and roots would be in one stratum, and the Germanic affixes and roots would be in another. This was never implemented in FieldWorks: everything went in one stratum except for the phonology and clitics, and affixes were restricted to particular roots using an attribute instead. When I added Strata to FieldWorks recently it was for performance reasons, but it interacted badly with final/non-final templates and clitics. Damien gave me permission to change how clitics were detected, but I also had to change how final/non-final templates were handled. This change should be backwards compatible with existing FieldWorks grammars because users couldn't add strata until recently and it is considered an experimental feature.

@claude

This comment has been minimized.

@johnml1135 johnml1135 left a comment

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.

Thanks — that makes sense, and the strata background helps. I've adjusted the #480 fixture so kulpu stays a same-stratum negative control: the later stratum's template no longer applies to it, and it passes both before and after this PR. Once both are in, I'll add a positive case pinning the cross-stratum completion you describe.

@claude

claude Bot commented Oct 15, 2026

Copy link
Copy Markdown
Contributor
  1. Verdict: approve.
  2. Most important: nothing open. The only change since 1959c30 is 835d137, a merge from master. It brings in Corpora/USFM, workflow, and skill changes from master and touches no HermitCrab file. git diff --stat be5407d 835d137 still lists only the 12 HermitCrab source, DTD, and test files this PR changes.
  3. Counts of open findings: Critical 0, Important 0, Low 0.
  4. What I ran: dotnet test tests/SIL.Machine.Morphology.HermitCrab.Tests passed 108 of 108 at this head. In CI, both "Report comment hygiene" checks pass; "Build on ubuntu-22.04" and "Build on windows-latest" were still running. I did not run ./local_check.sh --agent-strict.
  5. Not verified: the full build and test of the merged tree, until CI finishes. The F2 fix is still untested: no test covers a final template in a stratum with phonological rules followed by an affix in a later stratum.

Public API: new public property MorphemicMorphologicalRule.IsCliticRule (bool?) and a new optional isCliticRule attribute in HermitCrabInput.dtd. Final-template closing waits for the last selected stratum unless a stratum has phonological rules, so a final template in a later stratum can complete a non-final template from an earlier one. The author confirmed that is intended. No target framework, package, or machine.py parity contract changed.

Findings: F1 addressed; F2 addressed; F3 addressed; F4 addressed; F5 addressed; F6 addressed; F7 addressed; F8 addressed; F9 addressed; F10 withdrawn; F11 addressed; F12 addressed. No new findings this round.

Reviewed at 835d137

@jtmaxwell3
jtmaxwell3 merged commit a2f80e2 into master Oct 15, 2026
6 checks passed
@jtmaxwell3
jtmaxwell3 deleted the add-IsCliticRule branch October 15, 2026 19:46
johnml1135 added a commit that referenced this pull request Oct 15, 2026
#529 added isCliticRule to MorphologicalRule and RealizationalRule in the
library DTD, and let a vacuous final template in a later stratum complete a
non-final template from an earlier one.

- Sync conformance/HermitCrabInput.dtd with the library copy, and add
  ConformanceDtdTests so the two cannot drift again: the coverage inventory
  is read from the conformance copy, so a missed sync hides new surfaces.
- Add edge-cases/clitic-rule-designation: isCliticRule true and false on
  MorphologicalRule, the "Clitics" stratum default, stacked clitics, and the
  cross-stratum completion of a non-final template (kolpu).
- Keep morphotactic-attribute-breadth's kulpu a same-stratum negative
  control: the Second stratum's material moves to its own part of speech,
  so no later final template applies to kul.
- Map the ten new DTD surfaces in semantic-catalog.yaml. The two
  RealizationalRule/isCliticRule values stay todo in the baseline: the
  engine never reads them, and FieldWorks does not use realizational rules.
- Point conformance/docs/README.md at PanGloss's xample-projector, which
  produces the .fwdata witnesses and runs the XAMPLE comparison.
- Regenerate the ledgers and re-pin the measured counts.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@johnml1135

Copy link
Copy Markdown
Collaborator

Thanks — done in #480, now rebased on master. The conformance DTD is synced, and a new test fails if it ever drifts from the library's embedded copy. A new fixture, edge-cases/clitic-rule-designation, covers isCliticRule true and false on MorphologicalRule, the "Clitics" stratum default, stacked clitics, and the cross-stratum completion you described (kolpu). RealizationalRule/isCliticRule stays listed as uncovered, since nothing in the engine reads it there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants