Fix verse range marker placement bug

machine/corpora/__init__.py

@@ -71,7 +71,7 @@
 from .usfm_tag import UsfmJustification, UsfmStyleAttribute, UsfmStyleType, UsfmTag, UsfmTextProperties, UsfmTextType
 from .usfm_token import UsfmAttribute, UsfmToken, UsfmTokenType
 from .usfm_tokenizer import RtlReferenceOrder, UsfmTokenizer
-from .usfm_update_block import UsfmUpdateBlock
+from .usfm_update_block import UsfmUpdateBlock, UsfmUpdateBlockRow
 from .usfm_update_block_element import UsfmUpdateBlockElement, UsfmUpdateBlockElementType
 from .usfm_update_block_handler import UsfmUpdateBlockHandler
 from .usfm_versification_analyzer_base import UsfmVersificationAnalyzerBase
@@ -185,6 +185,7 @@
     "UsfmUpdateBlockElement",
     "UsfmUpdateBlockElementType",
     "UsfmUpdateBlockHandler",
+    "UsfmUpdateBlockRow",
     "UsfmVersificationAnalysis",
     "UsfmVersificationAnalyzerBase",
     "UsfmVersificationAnalyzerHandler",

machine/corpora/place_markers_usfm_update_block_handler.py

@@ -1,6 +1,7 @@
 from __future__ import annotations
 
-from typing import List, TypedDict, cast
+import logging
+from typing import Dict, List, Optional, Sequence, Tuple, TypedDict, cast
 
 from ..translation.word_alignment_matrix import WordAlignmentMatrix
 from .segment_boundary_adjuster import SegmentBoundaryAdjuster
@@ -10,6 +11,8 @@
 from .usfm_update_block_element import UsfmUpdateBlockElement, UsfmUpdateBlockElementType
 from .usfm_update_block_handler import UsfmUpdateBlockHandler, UsfmUpdateBlockHandlerError
 
+logger = logging.getLogger(__name__)
+
 PLACE_MARKERS_ALIGNMENT_INFO_KEY = "alignment_info"
 
 
@@ -21,6 +24,73 @@ class PlaceMarkersAlignmentInfo(TypedDict):
     style_behavior: UpdateUsfmMarkerBehavior
 
 
+def _get_row_alignment_infos(block: UsfmUpdateBlock) -> Optional[List[PlaceMarkersAlignmentInfo]]:
+    infos: List[PlaceMarkersAlignmentInfo] = []
+    for row in block.rows:
+        # Rows that contributed no text, such as the later verses of a verse range, have nothing to align
+        if len(row.text.strip()) == 0:
+            continue
+        info = cast(Optional[PlaceMarkersAlignmentInfo], row.metadata.get(PLACE_MARKERS_ALIGNMENT_INFO_KEY))
+        if info is None or len(info["translation_tokens"]) == 0:
+            logger.warning(
+                "Markers were not placed for %s because a row with text has no alignment info.", _format_refs(block)
+            )
+            return None
+        infos.append(info)
+    return infos if len(infos) > 0 else None
+
+
+def _get_block_alignment_info(
+    block: UsfmUpdateBlock, row_alignment_infos: Sequence[PlaceMarkersAlignmentInfo]
+) -> PlaceMarkersAlignmentInfo:
+    # Checked before merging, which would shift a pair outside a row's tokens into the next row's tokens
+    for info in row_alignment_infos:
+        _check_aligned_word_pairs(block, info)
+    alignment_info = (
+        row_alignment_infos[0] if len(row_alignment_infos) == 1 else _merge_alignment_infos(row_alignment_infos)
+    )
+    alignment = alignment_info["alignment"]
+    if not any(alignment.is_row_aligned(i) for i in range(alignment.row_count)):
+        logger.warning(
+            "The alignment for %s has no aligned words, so its markers were placed at the end of its text.",
+            _format_refs(block),
+        )
+    return alignment_info
+
+
+def _check_aligned_word_pairs(block: UsfmUpdateBlock, info: PlaceMarkersAlignmentInfo) -> None:
+    for pair in info["alignment"].to_aligned_word_pairs():
+        if pair.source_index >= len(info["source_tokens"]) or pair.target_index >= len(info["translation_tokens"]):
+            raise UsfmUpdateBlockHandlerError(
+                block,
+                f"Aligned word pair {pair.source_index}-{pair.target_index} is outside the tokens of its row "
+                f"for {_format_refs(block)}.",
+            )
+
+
+def _merge_alignment_infos(infos: Sequence[PlaceMarkersAlignmentInfo]) -> PlaceMarkersAlignmentInfo:
+    source_tokens: List[str] = []
+    translation_tokens: List[str] = []
+    word_pairs: List[Tuple[int, int]] = []
+    for info in infos:
+        # Offset by the token counts rather than the matrix size, since a matrix may omit unaligned trailing tokens
+        for pair in info["alignment"].to_aligned_word_pairs():
+            word_pairs.append((len(source_tokens) + pair.source_index, len(translation_tokens) + pair.target_index))
+        source_tokens.extend(info["source_tokens"])
+        translation_tokens.extend(info["translation_tokens"])
+    return PlaceMarkersAlignmentInfo(
+        source_tokens=source_tokens,
+        translation_tokens=translation_tokens,
+        alignment=WordAlignmentMatrix.from_word_pairs(len(source_tokens), len(translation_tokens), word_pairs),
+        paragraph_behavior=infos[0]["paragraph_behavior"],
+        style_behavior=infos[0]["style_behavior"],
+    )
+
+
+def _format_refs(block: UsfmUpdateBlock) -> str:
+    return ", ".join(str(ref) for ref in block.refs)
+
+
 class PlaceMarkersUsfmUpdateBlockHandler(UsfmUpdateBlockHandler):
     def __init__(self, *args):
         super().__init__(*args)
@@ -30,20 +100,25 @@ def process_block(self, block: UsfmUpdateBlock) -> UsfmUpdateBlock:
         elements = list(block.elements)
 
         # Nothing to do if there are no markers to place or no alignment to use
-        if PLACE_MARKERS_ALIGNMENT_INFO_KEY not in block.metadata:
+        # Checked before reading any alignment info, so that blocks without markers don't warn about it
+        if not any(
+            e.is_placeable(UpdateUsfmMarkerBehavior.PRESERVE, UpdateUsfmMarkerBehavior.PRESERVE) for e in elements
+        ):
             return block
 
-        alignment_info = cast(PlaceMarkersAlignmentInfo, block.metadata[PLACE_MARKERS_ALIGNMENT_INFO_KEY])
-        if (
-            len(elements) == 0
-            or alignment_info["alignment"].row_count == 0
-            or alignment_info["alignment"].column_count == 0
-            or not any(
-                e.is_placeable(alignment_info["paragraph_behavior"], alignment_info["style_behavior"]) for e in elements
-            )
-        ):
+        row_alignment_infos = _get_row_alignment_infos(block)
+        if row_alignment_infos is None:
             return block
 
+        # The same behaviors as the merged alignment info, which takes them from the first row
+        paragraph_behavior = row_alignment_infos[0]["paragraph_behavior"]
+        style_behavior = row_alignment_infos[0]["style_behavior"]
+        if not any(e.is_placeable(paragraph_behavior, style_behavior) for e in elements):
+            return block
+
+        # The alignment is checked only now, so that a block with nothing to place can't fail on it
+        alignment_info = _get_block_alignment_info(block, row_alignment_infos)
+
         # 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
         end_elements = []
@@ -128,6 +203,7 @@ def process_block(self, block: UsfmUpdateBlock) -> UsfmUpdateBlock:
 
         # Predict marker placements and get insertion order
         to_insert = []
+        open_style_trg_toks_by_marker: Dict[str, List[int]] = {}
         for element, adj_src_tok in zip(to_place, adj_src_toks):
             adj_trg_tok = self._predict_marker_location(alignment_info["alignment"], adj_src_tok, src_toks, trg_toks)
 
@@ -137,11 +213,18 @@ def process_block(self, block: UsfmUpdateBlock) -> UsfmUpdateBlock:
                     adj_trg_tok, trg_toks
                 )
 
-            if (
-                adj_trg_tok > 0
-                and element.type == UsfmUpdateBlockElementType.STYLE
-                and element.tokens[0].marker[-1] == "*"
-            ):
+            is_end_style = element.type == UsfmUpdateBlockElementType.STYLE and element.tokens[0].marker[-1] == "*"
+            if element.type == UsfmUpdateBlockElementType.STYLE:
+                marker = element.tokens[0].marker
+                if is_end_style:
+                    opening_trg_toks = open_style_trg_toks_by_marker.get(marker[:-1])
+                    if opening_trg_toks:
+                        # Reordered words can predict a closing marker before its opening one
+                        adj_trg_tok = max(adj_trg_tok, opening_trg_toks.pop() + 1)
+                else:
+                    open_style_trg_toks_by_marker.setdefault(marker, []).append(adj_trg_tok)
+
+            if is_end_style and 0 < adj_trg_tok <= len(trg_toks):
                 # Insert end tokens directly after the token they follow
                 trg_str_idx = trg_tok_starts[adj_trg_tok - 1] + len(trg_toks[adj_trg_tok - 1])
             elif adj_trg_tok < len(trg_tok_starts):
@@ -181,8 +264,7 @@ def process_block(self, block: UsfmUpdateBlock) -> UsfmUpdateBlock:
         while len(header_elements) > 0:
             placed_elements.append(header_elements.pop(0)[1])
 
-        block._elements = placed_elements + ignored_elements
-        return block
+        return UsfmUpdateBlock(block.refs, placed_elements + ignored_elements, rows=block.rows)
 
     def _predict_marker_location(
         self,

machine/corpora/update_usfm_parser_handler.py

@@ -9,7 +9,7 @@
 from .usfm_tag import UsfmTextType
 from .usfm_token import UsfmAttribute, UsfmToken, UsfmTokenType
 from .usfm_tokenizer import UsfmTokenizer
-from .usfm_update_block import UsfmUpdateBlock
+from .usfm_update_block import UsfmUpdateBlock, UsfmUpdateBlockRow
 from .usfm_update_block_element import UsfmUpdateBlockElement, UsfmUpdateBlockElementType
 from .usfm_update_block_handler import UsfmUpdateBlockHandler, UsfmUpdateBlockHandlerError
 
@@ -27,6 +27,56 @@ def __init__(self, refs: Sequence[ScriptureRef], text: str, metadata: Optional[d
         self.metadata = metadata
 
 
+class _UsfmUpdateBlockBuilder:
+    def __init__(self, refs: Iterable[ScriptureRef] = [], rows: Iterable[UsfmUpdateBlockRow] = []) -> None:
+        self._refs: List[ScriptureRef] = list(refs)
+        self._elements: List[UsfmUpdateBlockElement] = []
+        self._rows: List[UsfmUpdateBlockRow] = list(rows)
+
+    def add_text(self, tokens: Iterable[UsfmToken]) -> None:
+        self._elements.append(UsfmUpdateBlockElement(UsfmUpdateBlockElementType.TEXT, list(tokens)))
+
+    def add_token(self, token: UsfmToken, marked_for_removal: bool = False) -> None:
+        if token.type == UsfmTokenType.TEXT:
+            element_type = UsfmUpdateBlockElementType.TEXT
+        elif token.type == UsfmTokenType.PARAGRAPH:
+            element_type = UsfmUpdateBlockElementType.PARAGRAPH
+        elif token.type == UsfmTokenType.CHARACTER or token.type == UsfmTokenType.END:
+            element_type = UsfmUpdateBlockElementType.STYLE
+        else:
+            element_type = UsfmUpdateBlockElementType.OTHER
+        self._elements.append(UsfmUpdateBlockElement(element_type, [token], marked_for_removal))
+
+    def add_embed(self, tokens: Iterable[UsfmToken], marked_for_removal: bool = False) -> None:
+        self._elements.append(
+            UsfmUpdateBlockElement(UsfmUpdateBlockElementType.EMBED, list(tokens), marked_for_removal)
+        )
+
+    def extend_last_element(self, tokens: Iterable[UsfmToken]) -> None:
+        self._elements[-1].tokens.extend(tokens)
+
+    def update_refs(self, refs: Iterable[ScriptureRef]) -> None:
+        self._refs = list(refs)
+
+    def has_no_elements(self) -> bool:
+        return len(self._elements) == 0
+
+    def get_last_element(self) -> UsfmUpdateBlockElement:
+        return self._elements[-1]
+
+    def get_last_paragraph(self) -> Optional[UsfmUpdateBlockElement]:
+        for element in reversed(self._elements):
+            if element.type == UsfmUpdateBlockElementType.PARAGRAPH:
+                return element
+        return None
+
+    def pop(self) -> UsfmUpdateBlockElement:
+        return self._elements.pop()
+
+    def build(self) -> UsfmUpdateBlock:
+        return UsfmUpdateBlock(self._refs, self._elements, rows=self._rows)
+
+
 def _sanitize_verse_data(verse_data: str) -> str:
     return verse_data.replace("\u200F", "")
 
@@ -58,7 +108,7 @@ def __init__(
             self._update_rows_versification = Versification.get_builtin("English")
         self._tokens: List[UsfmToken] = []
         self._updated_text: List[UsfmToken] = []
-        self._update_block_stack: list[UsfmUpdateBlock] = []
+        self._update_block_stack: list[_UsfmUpdateBlockBuilder] = []
         self._embed_tokens: List[UsfmToken] = []
         self._id_text = id_text
         if update_block_handlers is None:
@@ -103,7 +153,7 @@ def start_book(self, state: UsfmParserState, marker: str, code: str) -> None:
             self._update_verse_rows()
 
         self._collect_readonly_tokens(state)
-        self._update_block_stack.append(UsfmUpdateBlock())
+        self._update_block_stack.append(_UsfmUpdateBlockBuilder())
         start_book_tokens: List[UsfmToken] = []
         if self._id_text is not None:
             start_book_tokens.append(UsfmToken(UsfmTokenType.TEXT, text=self._id_text + " "))
@@ -114,7 +164,7 @@ def start_book(self, state: UsfmParserState, marker: str, code: str) -> None:
     def end_book(self, state: UsfmParserState, marker: str) -> None:
         self._use_updated_text()
         self._pop_new_tokens()
-        update_block = self._update_block_stack.pop()
+        update_block = self._update_block_stack.pop().build()
         self._tokens.extend(update_block.get_tokens())
 
         super().end_book(state, marker)
@@ -379,9 +429,8 @@ def get_usfm(self, stylesheet: Union[str, UsfmStylesheet] = "usfm.sty") -> str:
                         tokens[index:index] = remark_tokens
         return tokenizer.detokenize(tokens)
 
-    def _advance_rows(self, seg_scr_refs: Sequence[ScriptureRef]) -> Tuple[List[str], Optional[dict[str, object]]]:
-        row_texts: List[str] = []
-        row_metadata = None
+    def _advance_rows(self, seg_scr_refs: Sequence[ScriptureRef]) -> List[UsfmUpdateBlockRow]:
+        block_rows: List[UsfmUpdateBlockRow] = []
         source_index: int = 0
 
         # handle the special case of verse 0, which although first in the rows,
@@ -406,13 +455,12 @@ def _advance_rows(self, seg_scr_refs: Sequence[ScriptureRef]) -> Tuple[List[str]
                 if compare == 0:
                     # source and row match
                     # grab the text - both source and row will be incremented in due time...
-                    row_texts.append(text)
-                    row_metadata = metadata
+                    block_rows.append(UsfmUpdateBlockRow(text, metadata if metadata is not None else {}))
                     break
             if compare <= 0:
                 # source is ahead of row, increment row
                 self._verse_row_index += 1
-        return row_texts, row_metadata
+        return block_rows
 
     def _collect_updatable_tokens(self, state: UsfmParserState) -> None:
         self._use_updated_text()
@@ -485,23 +533,24 @@ def _has_new_text(self) -> bool:
         return any(self._replace_stack) and self._replace_stack[-1]
 
     def _start_update_block(self, scripture_refs: Sequence[ScriptureRef]) -> None:
-        row_texts, metadata = self._advance_rows(scripture_refs)
-        self._update_block_stack.append(
-            UsfmUpdateBlock(scripture_refs, metadata=metadata if metadata is not None else {})
-        )
-        self._push_updated_text([UsfmToken(UsfmTokenType.TEXT, text=t + " ") for t in row_texts])
+        block_rows = self._advance_rows(scripture_refs)
+        self._update_block_stack.append(_UsfmUpdateBlockBuilder(scripture_refs, block_rows))
+        self._push_updated_text([UsfmToken(UsfmTokenType.TEXT, text=row.text + " ") for row in block_rows])
 
     def _end_update_block(self, state: UsfmParserState, scripture_refs: Sequence[ScriptureRef]) -> None:
         self._use_updated_text()
         self._pop_new_tokens()
-        update_block = self._update_block_stack.pop()
-        update_block.update_refs(scripture_refs)
+        update_block_builder = self._update_block_stack.pop()
+        update_block_builder.update_refs(scripture_refs)
 
         # Strip off any non-verse paragraphs that are at the end of the update block
         para_elems: list[UsfmUpdateBlockElement] = []
-        while len(update_block.elements) > 0 and _is_nonverse_paragraph(state, update_block.elements[-1]):
-            para_elems.append(update_block.pop())
+        while not update_block_builder.has_no_elements() and _is_nonverse_paragraph(
+            state, update_block_builder.get_last_element()
+        ):
+            para_elems.append(update_block_builder.pop())
 
+        update_block = update_block_builder.build()
         for handler in self._update_block_handlers:
             try:
                 update_block = handler.process_block(update_block)
@@ -515,7 +564,7 @@ def _end_update_block(self, state: UsfmParserState, scripture_refs: Sequence[Scr
             tokens.extend(elem.get_tokens())
         if (
             len(self._update_block_stack) > 0
-            and self._update_block_stack[-1].elements[-1].type == UsfmUpdateBlockElementType.PARAGRAPH
+            and self._update_block_stack[-1].get_last_element().type == UsfmUpdateBlockElementType.PARAGRAPH
         ):
             self._update_block_stack[-1].extend_last_element(tokens)
         else: