Skip to content
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.

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

Implement Lt 21911: Novel Root Guesser #172

Open
wants to merge 9 commits into
base: release/9.1
Choose a base branch
from
Open

Conversation

jtmaxwell3
Copy link
Collaborator

@jtmaxwell3 jtmaxwell3 commented Oct 3, 2024

This implements the Fieldworks side of https://jira.sil.org/browse/LT-21911. It uses HermitCrab 3.3.0. It changes the FieldWorks display code to show the underlying form of a morph bundle instead of the morpheme's lexeme when the lexeme is a lexical pattern. Normally, the underlying form and the lexeme are the same (except perhaps for canonicalization), but in this case they are quite different.


This change is Reviewable

@jasonleenaylor
Copy link
Contributor

Src/LexText/ParserCore/HCLoader.cs line 2237 at r1 (raw file):

			// This assumes that "[" and "]" are not part of any phonemes.
			return form.Contains("[") && form.Contains("]");
		}

I would like to have a single IsLexicalPattern if the current project dependencies allow us. This way if we change our criteria we only have to do it in one place.

Code quote:

		/// <summary>
		/// Does form contain a lexical pattern (e.g. [Seg]*)?
		/// </summary>
		public static bool IsLexicalPattern(string form)
		{
			// This assumes that "[" and "]" are not part of any phonemes.
			return form.Contains("[") && form.Contains("]");
		}

Copy link
Contributor

@jasonleenaylor jasonleenaylor left a comment

Choose a reason for hiding this comment

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

Reviewed 13 of 13 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @jtmaxwell3)


Src/LexText/ParserCore/ParseResult.cs line 166 at r1 (raw file):

		}

		public ParseMorph(IMoForm form, IMoMorphSynAnalysis msa, ILexEntryInflType inflType)

Since you added a new constructor with one extra parameter could you change this to follow the pattern used in the constructor defined above?


Src/LexText/ParserCore/ParserCore.csproj line 175 at r1 (raw file):

      <HintPath>..\..\..\Output\Debug\ApplicationTransforms.dll</HintPath>
    </Reference>
    <Reference Include="netstandard, Version=2.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51" />

I didn't see what made this necessary.


Src/LexText/ParserCore/ParserCoreTests/HCLoaderTests.cs line 965 at r1 (raw file):

			AllomorphCoOccurrenceRule coOccurRule = hcStemEntry.PrimaryAllomorph.AllomorphCoOccurrenceRules.First();
			Assert.That(coOccurRule.Adjacency, Is.EqualTo(MorphCoOccurrenceAdjacency.SomewhereToRight));
			Assert.That(coOccurRule.Others.Select(a => a.Morpheme.ToString()), Is.EqualTo(new[] {"-ɯd"}));

These old asserts may no longer be valid, but I'd like to see some new ones.
Can we add a new test or some new asserts to validate the new logic?

@jtmaxwell3
Copy link
Collaborator Author

I couldn't find a file to put IsLexicalRelation in that was accessible to both SandboxBase and HCLoader.

coOccurRule.Key doesn't exist in HermitCrab anymore because of a change to HermitCrab unrelated to my change. I implemented unit tests for the novel root guesser in the HermitCrab library. Most of the changes in FieldWorks are related to the UI and are covered by the acceptance test. Did you have a specific unit test in mind?

I fixed the other two issues.

Copy link
Contributor

@jasonleenaylor jasonleenaylor left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @jtmaxwell3)

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