Skip to content

Port 'Fix verse range marker placement bug' from machine.py #559

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
claude wants to merge 1 commit into
base: master
Choose a base branch
from
+523 −124
Open

Port 'Fix verse range marker placement bug' from machine.py #559

Show file tree
Hide file tree
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
145 changes: 122 additions & 23 deletions src/SIL.Machine/Corpora/PlaceMarkersUsfmUpdateBlockHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,23 +42,21 @@ public UsfmUpdateBlock ProcessBlock(UsfmUpdateBlock block)
var elements = block.Elements.ToList();

// Nothing to do if there are no markers to place or no alignment to use
if (!block.Metadata.TryGetValue(PlaceMarkersAlignmentInfo.MetadataKey, out object alignmentObject))
{
if (!elements.Any(e => e.IsPlaceable(UpdateUsfmMarkerBehavior.Preserve, UpdateUsfmMarkerBehavior.Preserve)))
return block;
}
if (!(alignmentObject is PlaceMarkersAlignmentInfo alignmentInfo))
{

List<PlaceMarkersAlignmentInfo> rowAlignmentInfos = GetRowAlignmentInfos(block);
if (rowAlignmentInfos == null)
return block;
}
if (
elements.Count == 0
|| alignmentInfo.Alignment.RowCount == 0
|| alignmentInfo.Alignment.ColumnCount == 0
|| !elements.Any(e => e.IsPlaceable(alignmentInfo.ParagraphBehavior, alignmentInfo.StyleBehavior))
)
{

// The same behaviors as the merged alignment info, which takes them from the first row
UpdateUsfmMarkerBehavior paragraphBehavior = rowAlignmentInfos[0].ParagraphBehavior;
UpdateUsfmMarkerBehavior styleBehavior = rowAlignmentInfos[0].StyleBehavior;
if (!elements.Any(e => e.IsPlaceable(paragraphBehavior, styleBehavior)))
return block;
}

// The alignment is checked only now, so that a block with nothing to place can't fail on it
PlaceMarkersAlignmentInfo alignmentInfo = GetBlockAlignmentInfo(block, rowAlignmentInfos);

// Paragraph markers at the end of the block should stay there
// Section headers should be ignored but re-inserted in the same position relative to other paragraph markers
Expand Down Expand Up @@ -183,6 +181,7 @@ public UsfmUpdateBlock ProcessBlock(UsfmUpdateBlock block)
}

var toInsert = new List<(int Index, UsfmUpdateBlockElement Element)>();
var openStyleTargetTokensByMarker = new Dictionary<string, List<int>>();
foreach (
(UsfmUpdateBlockElement element, int adjacentSourceToken) in toPlace
.Zip(adjacentSourceTokens)
Expand All @@ -203,12 +202,41 @@ public UsfmUpdateBlock ProcessBlock(UsfmUpdateBlock block)
targetTokens
);
}
bool isEndStyle =
element.Type == UsfmUpdateBlockElementType.Style
&& element.Tokens[0].Marker[element.Tokens[0].Marker.Length - 1] == '*';
if (element.Type == UsfmUpdateBlockElementType.Style)
{
string marker = element.Tokens[0].Marker;
if (isEndStyle)
{
if (
openStyleTargetTokensByMarker.TryGetValue(
marker.Substring(0, marker.Length - 1),
out List<int> openingTargetTokens
)
&& openingTargetTokens.Count > 0
)
{
// Reordered words can predict a closing marker before its opening one
int openingTargetToken = openingTargetTokens[openingTargetTokens.Count - 1];
openingTargetTokens.RemoveAt(openingTargetTokens.Count - 1);
adjacentTargetToken = Math.Max(adjacentTargetToken, openingTargetToken + 1);
}
}
else
{
if (!openStyleTargetTokensByMarker.TryGetValue(marker, out List<int> openTokens))
{
openTokens = new List<int>();
openStyleTargetTokensByMarker[marker] = openTokens;
}
openTokens.Add(adjacentTargetToken);
}
}

int targetStringIndex;
if (
adjacentSourceToken > 0
&& element.Type == UsfmUpdateBlockElementType.Style
&& element.Tokens[0].Marker[element.Tokens[0].Marker.Length - 1] == '*'
)
if (isEndStyle && adjacentTargetToken > 0 && adjacentTargetToken <= targetTokens.Count)
{
targetStringIndex =
targetTokenStarts[adjacentTargetToken - 1] + targetTokens[adjacentTargetToken - 1].Length;
Expand Down Expand Up @@ -282,11 +310,82 @@ public UsfmUpdateBlock ProcessBlock(UsfmUpdateBlock block)
headerElements.RemoveAt(0);
}

var processedBlock = new UsfmUpdateBlock(
refs: block.Refs,
elements: placedElements.Concat(ignoredElements)
return new UsfmUpdateBlock(block.Refs, placedElements.Concat(ignoredElements), block.Rows);
}

internal static List<PlaceMarkersAlignmentInfo> GetRowAlignmentInfos(UsfmUpdateBlock block)
{
var infos = new List<PlaceMarkersAlignmentInfo>();
foreach (UsfmUpdateBlockRow row in block.Rows)
{
// Rows that contributed no text, such as the later verses of a verse range, have nothing to align
if (row.Text.Trim().Length == 0)

@claude claude Bot Oct 26, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

FYI: F1. A row with null Text now crashes the update: row.Text.Trim() throws NullReferenceException, which is not a UsfmUpdateBlockHandlerException, so the error handler never sees it. UpdateUsfmRow accepts null text, and the old t + " " path tolerated it. Python types text: str, so this gap exists only in C#. string.IsNullOrWhiteSpace(row.Text) would cover it. Unverified: reasoned from the code, not run.

continue;
if (
!row.Metadata.TryGetValue(PlaceMarkersAlignmentInfo.MetadataKey, out object infoObject)
|| !(infoObject is PlaceMarkersAlignmentInfo info)
|| info.TranslationTokens.Count == 0
)
{
return null;
}
infos.Add(info);
}
return infos.Count > 0 ? infos : null;
}

internal static PlaceMarkersAlignmentInfo GetBlockAlignmentInfo(
UsfmUpdateBlock block,
IReadOnlyList<PlaceMarkersAlignmentInfo> rowAlignmentInfos
)
{
// Checked before merging, which would shift a pair outside a row's tokens into the next row's tokens
foreach (PlaceMarkersAlignmentInfo info in rowAlignmentInfos)
CheckAlignedWordPairs(block, info);
return rowAlignmentInfos.Count == 1 ? rowAlignmentInfos[0] : MergeAlignmentInfos(rowAlignmentInfos);
}

private static void CheckAlignedWordPairs(UsfmUpdateBlock block, PlaceMarkersAlignmentInfo info)
{
foreach (AlignedWordPair pair in info.Alignment.ToAlignedWordPairs())
{
if (pair.SourceIndex >= info.SourceTokens.Count || pair.TargetIndex >= info.TranslationTokens.Count)
{
throw new UsfmUpdateBlockHandlerException(
$"Aligned word pair {pair.SourceIndex}-{pair.TargetIndex} is outside the tokens of its row for {FormatRefs(block)}.",
block
);
}
}
}

private static PlaceMarkersAlignmentInfo MergeAlignmentInfos(IReadOnlyList<PlaceMarkersAlignmentInfo> infos)
{
var sourceTokens = new List<string>();
var translationTokens = new List<string>();
var wordPairs = new List<(int, int)>();
foreach (PlaceMarkersAlignmentInfo info in infos)
{
// Offset by token counts, not matrix size, since a matrix may omit unaligned trailing tokens
foreach (AlignedWordPair pair in info.Alignment.ToAlignedWordPairs())
{
wordPairs.Add((sourceTokens.Count + pair.SourceIndex, translationTokens.Count + pair.TargetIndex));
}
sourceTokens.AddRange(info.SourceTokens);
translationTokens.AddRange(info.TranslationTokens);
}
return new PlaceMarkersAlignmentInfo(
sourceTokens,
translationTokens,
new WordAlignmentMatrix(sourceTokens.Count, translationTokens.Count, wordPairs),
infos[0].ParagraphBehavior,
infos[0].StyleBehavior
);
return processedBlock;
}

private static string FormatRefs(UsfmUpdateBlock block)
{
return string.Join(", ", block.Refs.Select(r => r.ToString()));
}

private int PredictMarkerLocation(
Expand Down
42 changes: 21 additions & 21 deletions src/SIL.Machine/Corpora/UpdateUsfmParserHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ public class UpdateUsfmParserHandler : ScriptureRefUsfmParserHandlerBase
private readonly UpdateUsfmMarkerBehavior _embedBehavior;
private readonly UpdateUsfmMarkerBehavior _styleBehavior;
private readonly HashSet<string> _preserveParagraphStyles;
private readonly Stack<UsfmUpdateBlock> _updateBlocks;
private readonly Stack<UsfmUpdateBlockBuilder> _updateBlocks;
private readonly Stack<IUsfmUpdateBlockHandler> _updateBlockHandlers;
private readonly List<(int, string)> _remarks;
private readonly Stack<bool> _replace;
Expand Down Expand Up @@ -89,7 +89,7 @@ public UpdateUsfmParserHandler(
_updateRowsVersification = GetRowsVersification(rows);
_tokens = new List<UsfmToken>();
_updatedText = new List<UsfmToken>();
_updateBlocks = new Stack<UsfmUpdateBlock>();
_updateBlocks = new Stack<UsfmUpdateBlockBuilder>();
_embedTokens = new List<UsfmToken>();
_idText = idText;
_replace = new Stack<bool>();
Expand Down Expand Up @@ -137,7 +137,7 @@ public override void StartBook(UsfmParserState state, string marker, string code
}

CollectReadonlyTokens(state);
_updateBlocks.Push(new UsfmUpdateBlock());
_updateBlocks.Push(new UsfmUpdateBlockBuilder());
var startBookTokens = new List<UsfmToken>();
if (_idText != null)
startBookTokens.Add(new UsfmToken(_idText + " "));
Expand All @@ -150,7 +150,7 @@ public override void EndBook(UsfmParserState state, string marker)
{
UseUpdatedText();
PopNewTokens();
UsfmUpdateBlock updateBlock = _updateBlocks.Pop();
UsfmUpdateBlock updateBlock = _updateBlocks.Pop().Build();
_tokens.AddRange(updateBlock.GetTokens());

base.EndBook(state, marker);
Expand Down Expand Up @@ -519,12 +519,9 @@ public string GetUsfm(UsfmStylesheet stylesheet)
return tokenizer.Detokenize(tokens);
}

private (IReadOnlyList<string> RowTexts, Dictionary<string, object> Metadata) AdvanceRows(
IReadOnlyList<ScriptureRef> segScrRefs
)
private List<UsfmUpdateBlockRow> AdvanceRows(IReadOnlyList<ScriptureRef> segScrRefs)
{
var rowTexts = new List<string>();
Dictionary<string, object> rowMetadata = null;
var blockRows = new List<UsfmUpdateBlockRow>();
int sourceIndex = 0;

// handle the special case of verse 0, which although first in the rows,
Expand Down Expand Up @@ -558,8 +555,9 @@ IReadOnlyList<ScriptureRef> segScrRefs
{
// source and row match
// grab the text - both source and row will be incremented in due time...
rowTexts.Add(text);
rowMetadata = metadata.ToDictionary(kvp => kvp.Key, kvp => kvp.Value);
blockRows.Add(
new UsfmUpdateBlockRow(text, metadata.ToDictionary(kvp => kvp.Key, kvp => kvp.Value))
);
break;
}
}
Expand All @@ -569,7 +567,7 @@ IReadOnlyList<ScriptureRef> segScrRefs
_verseRowIndex++;
}
}
return (rowTexts, rowMetadata);
return blockRows;
}

private void CollectUpdatableTokens(UsfmParserState state)
Expand Down Expand Up @@ -678,27 +676,29 @@ private bool HasNewText()

private void StartUpdateBlock(IReadOnlyList<ScriptureRef> scriptureRefs)
{
(IReadOnlyList<string> rowTexts, Dictionary<string, object> metadata) = AdvanceRows(scriptureRefs);
_updateBlocks.Push(
new UsfmUpdateBlock(scriptureRefs, metadata: metadata ?? new Dictionary<string, object>())
);
PushUpdatedText(rowTexts.Select(t => new UsfmToken(t + " ")));
List<UsfmUpdateBlockRow> blockRows = AdvanceRows(scriptureRefs);
_updateBlocks.Push(new UsfmUpdateBlockBuilder(scriptureRefs, blockRows));
PushUpdatedText(blockRows.Select(row => new UsfmToken(row.Text + " ")));
}

private void EndUpdateBlock(UsfmParserState state, IReadOnlyList<ScriptureRef> scriptureRefs)
{
UseUpdatedText();
PopNewTokens();
UsfmUpdateBlock updateBlock = _updateBlocks.Pop();
updateBlock.UpdateRefs(scriptureRefs);
UsfmUpdateBlockBuilder updateBlockBuilder = _updateBlocks.Pop();
updateBlockBuilder.UpdateRefs(scriptureRefs);

// Strip off any non-verse paragraphs that are at the end of the update block
var paraElems = new List<UsfmUpdateBlockElement>();
while (updateBlock.Elements.Count > 0 && IsNonverseParagraph(state, updateBlock.Elements.Last()))
while (
updateBlockBuilder.Elements.Count > 0 && IsNonverseParagraph(state, updateBlockBuilder.Elements.Last())
)
{
paraElems.Add(updateBlock.Pop());
paraElems.Add(updateBlockBuilder.Pop());
}

UsfmUpdateBlock updateBlock = updateBlockBuilder.Build();

foreach (IUsfmUpdateBlockHandler handler in _updateBlockHandlers)
{
try
Expand Down
Loading
Loading