Skip to content

Port transductive alignment model - #466

Open
pmachapman wants to merge 2 commits into
masterfrom
transductive-model
Open

Port transductive alignment model#466
pmachapman wants to merge 2 commits into
masterfrom
transductive-model

Conversation

@pmachapman

@pmachapman pmachapman commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

Fixes #455.

Port of sillsdev/machine.py#327

@ddaspit I am unsure whether my implementations of ThotSymmetrizedWordAlignmentModel.TrainingAlignmentCount and ThotWordAlignmentModel.GetTrainingAlignment() are correct. I wrote them based on what I understood the underlying logic in thot to be doing, as SymmetrizedAlignmentModel is not exposed via the C API.

If they are not right, I will update thot to expose the methods from SymmetrizedAlignmentModel in the C API that are exposed in thot's module.cc and used by the machine.py implementation.


This change is Reviewable

@pmachapman
pmachapman requested review from Enkidu93 and ddaspit August 3, 2026 03:17

@ddaspit ddaspit left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@ddaspit reviewed 9 files and all commit messages, and made 3 comments.
Reviewable status: 9 of 10 files reviewed, 3 unresolved discussions (waiting on Enkidu93 and pmachapman).


src/SIL.Machine/Corpora/CorporaExtensions.cs line 1135 at r1 (raw file):

        }

        public static IParallelTextCorpus WordAlign(

I think there are too many pitfalls with trying to implement this operation. Instead, we should add a similar IParallelTextCorpus extension method in SIL.Machine.Translation.Thot that trains and aligns a corpus using a Thot model. The method should accept a ThotWordAlignmentModelType parameter. You can have overloads that accept a ThotWordAlignmentModel or ThotSymmetrizedWordAlignmentModel as well. It would mimic the word_align_corpus function in Machine.py. If the passed in model is not configured to emit training alignments, then it should fall back to inductive alignment.


src/SIL.Machine.Translation.Thot/ThotWordAlignmentModel.cs line 169 at r1 (raw file):

        {
            CheckDisposed();
            IntPtr nativeMatrix = Thot.AllocNativeMatrix(_sourceWords.Count, _targetWords.Count);

To get the size of the matrix, you can call swAlignModel_getTrainingAlignment with a null buffer.


src/SIL.Machine.Translation.Thot/ThotWordAlignmentModelTrainer.cs line 24 at r1 (raw file):

        private readonly bool _isEflomal;

        public ThotWordAlignmentModelTrainer(

You forgot emitTrainingAlignments here.

@pmachapman
pmachapman requested a review from ddaspit August 5, 2026 03:14

@pmachapman pmachapman left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@pmachapman made 3 comments.
Reviewable status: 9 of 10 files reviewed, 3 unresolved discussions (waiting on ddaspit and Enkidu93).


src/SIL.Machine/Corpora/CorporaExtensions.cs line 1135 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

I think there are too many pitfalls with trying to implement this operation. Instead, we should add a similar IParallelTextCorpus extension method in SIL.Machine.Translation.Thot that trains and aligns a corpus using a Thot model. The method should accept a ThotWordAlignmentModelType parameter. You can have overloads that accept a ThotWordAlignmentModel or ThotSymmetrizedWordAlignmentModel as well. It would mimic the word_align_corpus function in Machine.py. If the passed in model is not configured to emit training alignments, then it should fall back to inductive alignment.

I think I understand what you mean, but still have a couple of questions (and think I might have misunderstood some of your instructions) - see the updated code committed below. Can we talk about this in the meeting tomorrow?


src/SIL.Machine.Translation.Thot/ThotWordAlignmentModel.cs line 169 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

To get the size of the matrix, you can call swAlignModel_getTrainingAlignment with a null buffer.

Unless I misunderstand what is going on, passing IntPtr.Zero to the matrix argument crashes with an access violation as the matrix is clamped:

  double swAlignModel_getTrainingAlignment(void* swAlignModelHandle, unsigned int n, bool** matrix, unsigned int* iLen,
                                           unsigned int* jLen)
  {
    auto alignmentModel = static_cast<AlignmentModel*>(swAlignModelHandle);

    WordAlignmentMatrix waMatrix;
    LgProb prob = alignmentModel->getTrainingAlignment(n, waMatrix);
    // A filtered/out-of-range pair yields an empty matrix, so clamp to its actual
    // dimensions as well as the caller-provided capacity.
    for (unsigned int i = 0; i < *iLen && i < waMatrix.get_I(); i++)
      for (unsigned int j = 0; j < *jLen && j < waMatrix.get_J(); j++)
        matrix[i][j] = waMatrix.getValue(i, j);
    *iLen = waMatrix.get_I();
    *jLen = waMatrix.get_J();
    return prob;
  }

If it sounds good to you, I can create a PR to remove the clamping in thot? I think this will be OK as we output the X and Y lengths.


src/SIL.Machine.Translation.Thot/ThotWordAlignmentModelTrainer.cs line 24 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

You forgot emitTrainingAlignments here.

Done. Thanks!

@codecov-commenter

codecov-commenter commented Aug 5, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 86.84211% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.64%. Comparing base (dac2d89) to head (a0a16d2).

Files with missing lines Patch % Lines
.../SIL.Machine.Translation.Thot/CorporaExtensions.cs 85.24% 7 Missing and 2 partials ⚠️
...nslation.Thot/ThotSymmetrizedWordAlignmentModel.cs 82.14% 3 Missing and 2 partials ⚠️
....Translation.Thot/ThotWordAlignmentModelTrainer.cs 88.88% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #466      +/-   ##
==========================================
+ Coverage   73.33%   73.64%   +0.30%     
==========================================
  Files         445      447       +2     
  Lines       37317    37428     +111     
  Branches     5118     5127       +9     
==========================================
+ Hits        27367    27563     +196     
+ Misses       8825     8733      -92     
- Partials     1125     1132       +7     

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

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.

Port 'Add support for transductive alignment models'

3 participants