From a346939ad39666aa004559e7f61e6458dbfb6a93 Mon Sep 17 00:00:00 2001 From: Ben King Date: Tue, 15 Sep 2026 13:55:17 -0400 Subject: [PATCH 1/5] Ignore rows that contributed no text when placing markers A verse range in the USFM being updated can be matched by more than one row. The updater concatenated all of their texts but kept only the last row's metadata, so the place markers handler received alignment info that did not describe the text it was given. Extracting a verse range puts all of its text on the first verse and leaves the rest empty, so the common shape is one row with text followed by empty rows. The empty row's metadata won, its alignment matrix was empty, and the handler silently placed no markers at all for that range. The block now carries every matched row's metadata alongside the existing single dict, which is left as it was so that existing handlers reading block.metadata are unaffected. The place markers handler ignores rows that have no alignment info or an empty alignment matrix, since they contributed no text to align. If more than one row is left it warns, as only one row's alignment can be used and it describes only part of the block's text. Co-Authored-By: Claude Opus 5 --- ...place_markers_usfm_update_block_handler.py | 33 +++++++- machine/corpora/update_usfm_parser_handler.py | 14 ++-- machine/corpora/usfm_update_block.py | 17 +++- ...place_markers_usfm_update_block_handler.py | 82 +++++++++++++++++++ 4 files changed, 136 insertions(+), 10 deletions(-) diff --git a/machine/corpora/place_markers_usfm_update_block_handler.py b/machine/corpora/place_markers_usfm_update_block_handler.py index 421d8b2b..1b8bcd55 100644 --- a/machine/corpora/place_markers_usfm_update_block_handler.py +++ b/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 List, Optional, 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,30 @@ class PlaceMarkersAlignmentInfo(TypedDict): style_behavior: UpdateUsfmMarkerBehavior +def _get_alignment_info(block: UsfmUpdateBlock) -> Optional[PlaceMarkersAlignmentInfo]: + if len(block.row_metadata) > 1: + # Verse ranges put all of their text on the first row + infos = [ + info + for info in ( + cast(Optional[PlaceMarkersAlignmentInfo], metadata.get(PLACE_MARKERS_ALIGNMENT_INFO_KEY)) + for metadata in block.row_metadata + ) + if info is not None and info["alignment"].row_count > 0 and info["alignment"].column_count > 0 + ] + if len(infos) > 1: + # Only the first row should have alignment info + logger.warning( + "Expected at most one row with alignment info for %s, but found %d. Markers may be misplaced.", + ", ".join(str(ref) for ref in block.refs), + len(infos), + ) + return infos[-1] if len(infos) > 0 else None + if PLACE_MARKERS_ALIGNMENT_INFO_KEY not in block.metadata: + return None + return cast(PlaceMarkersAlignmentInfo, block.metadata[PLACE_MARKERS_ALIGNMENT_INFO_KEY]) + + class PlaceMarkersUsfmUpdateBlockHandler(UsfmUpdateBlockHandler): def __init__(self, *args): super().__init__(*args) @@ -30,10 +57,10 @@ 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: + alignment_info = _get_alignment_info(block) + if alignment_info is None: return block - alignment_info = cast(PlaceMarkersAlignmentInfo, block.metadata[PLACE_MARKERS_ALIGNMENT_INFO_KEY]) if ( len(elements) == 0 or alignment_info["alignment"].row_count == 0 diff --git a/machine/corpora/update_usfm_parser_handler.py b/machine/corpora/update_usfm_parser_handler.py index 87689d8a..700b10e5 100644 --- a/machine/corpora/update_usfm_parser_handler.py +++ b/machine/corpora/update_usfm_parser_handler.py @@ -379,9 +379,9 @@ 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]]]: + def _advance_rows(self, seg_scr_refs: Sequence[ScriptureRef]) -> Tuple[List[str], List[dict[str, object]]]: row_texts: List[str] = [] - row_metadata = None + row_metadata: List[dict[str, object]] = [] source_index: int = 0 # handle the special case of verse 0, which although first in the rows, @@ -407,7 +407,7 @@ def _advance_rows(self, seg_scr_refs: Sequence[ScriptureRef]) -> Tuple[List[str] # source and row match # grab the text - both source and row will be incremented in due time... row_texts.append(text) - row_metadata = metadata + row_metadata.append(metadata if metadata is not None else {}) break if compare <= 0: # source is ahead of row, increment row @@ -485,9 +485,13 @@ 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) + row_texts, row_metadata = self._advance_rows(scripture_refs) self._update_block_stack.append( - UsfmUpdateBlock(scripture_refs, metadata=metadata if metadata is not None else {}) + UsfmUpdateBlock( + scripture_refs, + metadata=row_metadata[-1] if len(row_metadata) > 0 else {}, + row_metadata=row_metadata, + ) ) self._push_updated_text([UsfmToken(UsfmTokenType.TEXT, text=t + " ") for t in row_texts]) diff --git a/machine/corpora/usfm_update_block.py b/machine/corpora/usfm_update_block.py index 977e82b2..0f233563 100644 --- a/machine/corpora/usfm_update_block.py +++ b/machine/corpora/usfm_update_block.py @@ -13,10 +13,14 @@ def __init__( refs: Iterable[ScriptureRef] = [], elements: Iterable[UsfmUpdateBlockElement] = [], metadata: dict[str, object] = {}, + row_metadata: Iterable[dict[str, object]] = [], ) -> None: self._refs: list[ScriptureRef] = list(refs) self._elements: list[UsfmUpdateBlockElement] = list(elements) self._metadata: dict[str, object] = metadata + # One entry per row matched to this block, in order. A verse range can be matched by + # several rows, in which case this block's text is those rows' texts concatenated. + self._row_metadata: list[dict[str, object]] = list(row_metadata) @property def refs(self) -> Sequence[ScriptureRef]: @@ -30,6 +34,10 @@ def elements(self) -> Sequence[UsfmUpdateBlockElement]: def metadata(self) -> dict[str, object]: return self._metadata + @property + def row_metadata(self) -> Sequence[dict[str, object]]: + return self._row_metadata + def add_text(self, tokens: Iterable[UsfmToken]) -> None: self._elements.append(UsfmUpdateBlockElement(UsfmUpdateBlockElementType.TEXT, list(tokens))) @@ -68,7 +76,12 @@ def get_tokens(self) -> list[UsfmToken]: return [token for element in self._elements for token in element.get_tokens()] def __eq__(self, other: UsfmUpdateBlock) -> bool: - return self._refs == other._refs and self._elements == other._elements and self._metadata == other._metadata + return ( + self._refs == other._refs + and self._elements == other._elements + and self._metadata == other._metadata + and self._row_metadata == other._row_metadata + ) def copy(self) -> UsfmUpdateBlock: - return UsfmUpdateBlock(self._refs, self._elements, self._metadata) + return UsfmUpdateBlock(self._refs, self._elements, self._metadata, self._row_metadata) diff --git a/tests/corpora/test_place_markers_usfm_update_block_handler.py b/tests/corpora/test_place_markers_usfm_update_block_handler.py index fe26a15d..aa2ba333 100644 --- a/tests/corpora/test_place_markers_usfm_update_block_handler.py +++ b/tests/corpora/test_place_markers_usfm_update_block_handler.py @@ -1,5 +1,8 @@ +import logging from typing import List, Optional, Sequence +import pytest + from machine.corpora import ( AlignedWordPair, PlaceMarkersAlignmentInfo, @@ -940,6 +943,85 @@ def test_anusvara_tokenization() -> None: assert_usfm_equals(target, result) +def test_verse_range_with_empty_trailing_row(caplog: pytest.LogCaptureFixture) -> None: + # Verse ranges consist of multiple rows, but only the first one is non-empty and has a non-empty alignment matrix. + # An empty alignment matrix must not clobber any non-empty matrices. + source = "This is the first part. This is the second part." + pretranslation = "Esta es la primera parte. Esta es la segunda parte." + align_info = PlaceMarkersAlignmentInfo( + source_tokens=[t for t in TOKENIZER.tokenize(source)], + translation_tokens=[t for t in TOKENIZER.tokenize(pretranslation)], + alignment=to_word_alignment_matrix("0-0 1-1 2-2 3-3 4-4 5-5 6-6 7-7 8-8 9-9 10-10 11-11"), + paragraph_behavior=UpdateUsfmMarkerBehavior.PRESERVE, + style_behavior=UpdateUsfmMarkerBehavior.STRIP, + ) + empty_align_info = PlaceMarkersAlignmentInfo( + source_tokens=[], + translation_tokens=[], + alignment=to_word_alignment_matrix(""), + paragraph_behavior=UpdateUsfmMarkerBehavior.PRESERVE, + style_behavior=UpdateUsfmMarkerBehavior.STRIP, + ) + usfm = r"""\id MAT +\c 1 +\v 1-2 This is the first part. +\p This is the second part. +""" + result = r"""\id MAT +\c 1 +\v 1-2 Esta es la primera parte. +\p Esta es la segunda parte. +""" + + # the result should be the same whether the second row has an empty alignment matrix or no alignment matrix at all + for empty_row in [ + UpdateUsfmRow(scr_ref("MAT 1:2"), "", metadata={"alignment_info": empty_align_info}), + UpdateUsfmRow(scr_ref("MAT 1:2"), ""), + ]: + rows = [ + UpdateUsfmRow(scr_ref("MAT 1:1"), str(pretranslation), metadata={"alignment_info": align_info}), + empty_row, + ] + with caplog.at_level(logging.WARNING): + target = update_usfm(rows, usfm, update_block_handlers=[PlaceMarkersUsfmUpdateBlockHandler()]) + assert_usfm_equals(target, result) + # a row that contributed no text is expected, so it must not be reported as unexpected + assert "Expected at most one row with alignment info" not in caplog.text + + +def test_multiple_rows_with_alignment_info_warns(caplog: pytest.LogCaptureFixture) -> None: + # Only one row's alignment info can be used, so a block matched by several rows that each have + # text of their own is outside what the handler can place markers for + first_align_info = PlaceMarkersAlignmentInfo( + source_tokens=[t for t in TOKENIZER.tokenize("This is the first part.")], + translation_tokens=[t for t in TOKENIZER.tokenize("Esta es la primera parte.")], + alignment=to_word_alignment_matrix("0-0 1-1 2-2 3-3 4-4 5-5"), + paragraph_behavior=UpdateUsfmMarkerBehavior.PRESERVE, + style_behavior=UpdateUsfmMarkerBehavior.STRIP, + ) + second_align_info = PlaceMarkersAlignmentInfo( + source_tokens=[t for t in TOKENIZER.tokenize("This is the second part.")], + translation_tokens=[t for t in TOKENIZER.tokenize("Esta es la segunda parte.")], + alignment=to_word_alignment_matrix("0-0 1-1 2-2 3-3 4-4 5-5"), + paragraph_behavior=UpdateUsfmMarkerBehavior.PRESERVE, + style_behavior=UpdateUsfmMarkerBehavior.STRIP, + ) + rows = [ + UpdateUsfmRow(scr_ref("MAT 1:1"), "Esta es la primera parte.", metadata={"alignment_info": first_align_info}), + UpdateUsfmRow(scr_ref("MAT 1:2"), "Esta es la segunda parte.", metadata={"alignment_info": second_align_info}), + ] + usfm = r"""\id MAT +\c 1 +\v 1-2 This is the first part. +\p This is the second part. +""" + + with caplog.at_level(logging.WARNING): + update_usfm(rows, usfm, update_block_handlers=[PlaceMarkersUsfmUpdateBlockHandler()]) + + assert "Expected at most one row with alignment info for MAT 1:1, MAT 1:2, but found 2" in caplog.text + + def scr_ref(*refs: str) -> List[ScriptureRef]: return [ScriptureRef.parse(ref) for ref in refs] From fb1d80e0a89b9b71204902998df7b9a99fe720fb Mon Sep 17 00:00:00 2001 From: Ben King Date: Tue, 15 Sep 2026 15:25:55 -0400 Subject: [PATCH 2/5] Simplify `UsfmUpdateBlock` constructor --- machine/corpora/update_usfm_parser_handler.py | 8 +------- machine/corpora/usfm_update_block.py | 7 ++----- 2 files changed, 3 insertions(+), 12 deletions(-) diff --git a/machine/corpora/update_usfm_parser_handler.py b/machine/corpora/update_usfm_parser_handler.py index 700b10e5..7e384fc0 100644 --- a/machine/corpora/update_usfm_parser_handler.py +++ b/machine/corpora/update_usfm_parser_handler.py @@ -486,13 +486,7 @@ def _has_new_text(self) -> bool: def _start_update_block(self, scripture_refs: Sequence[ScriptureRef]) -> None: row_texts, row_metadata = self._advance_rows(scripture_refs) - self._update_block_stack.append( - UsfmUpdateBlock( - scripture_refs, - metadata=row_metadata[-1] if len(row_metadata) > 0 else {}, - row_metadata=row_metadata, - ) - ) + self._update_block_stack.append(UsfmUpdateBlock(scripture_refs, row_metadata=row_metadata)) self._push_updated_text([UsfmToken(UsfmTokenType.TEXT, text=t + " ") for t in row_texts]) def _end_update_block(self, state: UsfmParserState, scripture_refs: Sequence[ScriptureRef]) -> None: diff --git a/machine/corpora/usfm_update_block.py b/machine/corpora/usfm_update_block.py index 0f233563..094ad17e 100644 --- a/machine/corpora/usfm_update_block.py +++ b/machine/corpora/usfm_update_block.py @@ -12,12 +12,10 @@ def __init__( self, refs: Iterable[ScriptureRef] = [], elements: Iterable[UsfmUpdateBlockElement] = [], - metadata: dict[str, object] = {}, row_metadata: Iterable[dict[str, object]] = [], ) -> None: self._refs: list[ScriptureRef] = list(refs) self._elements: list[UsfmUpdateBlockElement] = list(elements) - self._metadata: dict[str, object] = metadata # One entry per row matched to this block, in order. A verse range can be matched by # several rows, in which case this block's text is those rows' texts concatenated. self._row_metadata: list[dict[str, object]] = list(row_metadata) @@ -32,7 +30,7 @@ def elements(self) -> Sequence[UsfmUpdateBlockElement]: @property def metadata(self) -> dict[str, object]: - return self._metadata + return self._row_metadata[-1] if len(self._row_metadata) > 0 else {} @property def row_metadata(self) -> Sequence[dict[str, object]]: @@ -79,9 +77,8 @@ def __eq__(self, other: UsfmUpdateBlock) -> bool: return ( self._refs == other._refs and self._elements == other._elements - and self._metadata == other._metadata and self._row_metadata == other._row_metadata ) def copy(self) -> UsfmUpdateBlock: - return UsfmUpdateBlock(self._refs, self._elements, self._metadata, self._row_metadata) + return UsfmUpdateBlock(self._refs, self._elements, self._row_metadata) From aef9ece4664cac673d6d58d054c23b43b0f4d364 Mon Sep 17 00:00:00 2001 From: Ben King Date: Thu, 1 Oct 2026 11:41:32 -0400 Subject: [PATCH 3/5] Implement support for multiple text rows and multiple alignment matrices within a single update block. --- machine/corpora/__init__.py | 3 +- ...place_markers_usfm_update_block_handler.py | 108 ++++-- machine/corpora/update_usfm_parser_handler.py | 87 ++++- machine/corpora/usfm_update_block.py | 65 +--- ...place_markers_usfm_update_block_handler.py | 337 +++++++++++++++++- 5 files changed, 483 insertions(+), 117 deletions(-) diff --git a/machine/corpora/__init__.py b/machine/corpora/__init__.py index 20f3a1da..8f14308b 100644 --- a/machine/corpora/__init__.py +++ b/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", diff --git a/machine/corpora/place_markers_usfm_update_block_handler.py b/machine/corpora/place_markers_usfm_update_block_handler.py index 1b8bcd55..03dcb277 100644 --- a/machine/corpora/place_markers_usfm_update_block_handler.py +++ b/machine/corpora/place_markers_usfm_update_block_handler.py @@ -1,7 +1,7 @@ from __future__ import annotations import logging -from typing import List, Optional, TypedDict, cast +from typing import Dict, List, Optional, Sequence, Tuple, TypedDict, cast from ..translation.word_alignment_matrix import WordAlignmentMatrix from .segment_boundary_adjuster import SegmentBoundaryAdjuster @@ -25,27 +25,64 @@ class PlaceMarkersAlignmentInfo(TypedDict): def _get_alignment_info(block: UsfmUpdateBlock) -> Optional[PlaceMarkersAlignmentInfo]: - if len(block.row_metadata) > 1: - # Verse ranges put all of their text on the first row - infos = [ - info - for info in ( - cast(Optional[PlaceMarkersAlignmentInfo], metadata.get(PLACE_MARKERS_ALIGNMENT_INFO_KEY)) - for metadata in block.row_metadata - ) - if info is not None and info["alignment"].row_count > 0 and info["alignment"].column_count > 0 - ] - if len(infos) > 1: - # Only the first row should have alignment info + 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( - "Expected at most one row with alignment info for %s, but found %d. Markers may be misplaced.", - ", ".join(str(ref) for ref in block.refs), - len(infos), + "Markers were not placed for %s because a row with text has no alignment info.", _format_refs(block) ) - return infos[-1] if len(infos) > 0 else None - if PLACE_MARKERS_ALIGNMENT_INFO_KEY not in block.metadata: + return None + # The same alignment info on several rows already describes their combined text + if not any(info is other for other in infos): + infos.append(info) + + if len(infos) == 0: return None - return cast(PlaceMarkersAlignmentInfo, block.metadata[PLACE_MARKERS_ALIGNMENT_INFO_KEY]) + # A block matched by several rows, e.g. a verse range, has those rows' texts concatenated + alignment_info = infos[0] if len(infos) == 1 else _merge_alignment_infos(block, infos) + alignment = alignment_info["alignment"] + if not any(alignment.is_row_aligned(i) for i in range(alignment.row_count)): + # Markers are still placed, but with nothing to align them to, they all go at the end of the text + 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 _merge_alignment_infos( + block: UsfmUpdateBlock, 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(): + 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)}.", + ) + 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): @@ -61,13 +98,8 @@ def process_block(self, block: UsfmUpdateBlock) -> UsfmUpdateBlock: if alignment_info is None: return block - 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 - ) + if len(elements) == 0 or not any( + e.is_placeable(alignment_info["paragraph_behavior"], alignment_info["style_behavior"]) for e in elements ): return block @@ -155,6 +187,8 @@ def process_block(self, block: UsfmUpdateBlock) -> UsfmUpdateBlock: # Predict marker placements and get insertion order to_insert = [] + # The predicted target token of each open style marker, by marker + open_style_trg_toks: 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) @@ -164,12 +198,19 @@ 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] == "*" - ): - # Insert end tokens directly after the token they follow + 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.get(marker[:-1]) + if opening_trg_toks: + # Move a misplaced closing style marker to one token after its opening marker + adj_trg_tok = max(adj_trg_tok, opening_trg_toks.pop() + 1) + else: + open_style_trg_toks.setdefault(marker, []).append(adj_trg_tok) + + if is_end_style and 0 < adj_trg_tok <= len(trg_toks): + # Place closing style marker at the end of the token 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): trg_str_idx = trg_tok_starts[adj_trg_tok] @@ -208,8 +249,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, block.rows) def _predict_marker_location( self, diff --git a/machine/corpora/update_usfm_parser_handler.py b/machine/corpora/update_usfm_parser_handler.py index 7e384fc0..6084666b 100644 --- a/machine/corpora/update_usfm_parser_handler.py +++ b/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 is_empty(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, 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], List[dict[str, object]]]: - row_texts: List[str] = [] - row_metadata: List[dict[str, object]] = [] + 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.append(metadata if metadata is not None else {}) + 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,21 +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, row_metadata = self._advance_rows(scripture_refs) - self._update_block_stack.append(UsfmUpdateBlock(scripture_refs, row_metadata=row_metadata)) - 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.is_empty() 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) @@ -513,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: diff --git a/machine/corpora/usfm_update_block.py b/machine/corpora/usfm_update_block.py index 094ad17e..b605d960 100644 --- a/machine/corpora/usfm_update_block.py +++ b/machine/corpora/usfm_update_block.py @@ -1,10 +1,17 @@ from __future__ import annotations +from dataclasses import dataclass, field from typing import Iterable, Sequence from .scripture_ref import ScriptureRef -from .usfm_token import UsfmToken, UsfmTokenType -from .usfm_update_block_element import UsfmUpdateBlockElement, UsfmUpdateBlockElementType +from .usfm_token import UsfmToken +from .usfm_update_block_element import UsfmUpdateBlockElement + + +@dataclass +class UsfmUpdateBlockRow: + text: str + metadata: dict[str, object] = field(default_factory=dict) class UsfmUpdateBlock: @@ -12,13 +19,13 @@ def __init__( self, refs: Iterable[ScriptureRef] = [], elements: Iterable[UsfmUpdateBlockElement] = [], - row_metadata: Iterable[dict[str, object]] = [], + rows: Iterable[UsfmUpdateBlockRow] = [], ) -> None: self._refs: list[ScriptureRef] = list(refs) self._elements: list[UsfmUpdateBlockElement] = list(elements) # One entry per row matched to this block, in order. A verse range can be matched by # several rows, in which case this block's text is those rows' texts concatenated. - self._row_metadata: list[dict[str, object]] = list(row_metadata) + self._rows: list[UsfmUpdateBlockRow] = list(rows) @property def refs(self) -> Sequence[ScriptureRef]: @@ -29,56 +36,14 @@ def elements(self) -> Sequence[UsfmUpdateBlockElement]: return self._elements @property - def metadata(self) -> dict[str, object]: - return self._row_metadata[-1] if len(self._row_metadata) > 0 else {} - - @property - def row_metadata(self) -> Sequence[dict[str, object]]: - return self._row_metadata - - 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 get_last_paragraph(self) -> UsfmUpdateBlockElement | None: - for element in reversed(self._elements): - if element.type == UsfmUpdateBlockElementType.PARAGRAPH: - return element - return None - - def pop(self) -> UsfmUpdateBlockElement: - return self._elements.pop() + def rows(self) -> Sequence[UsfmUpdateBlockRow]: + return self._rows def get_tokens(self) -> list[UsfmToken]: return [token for element in self._elements for token in element.get_tokens()] def __eq__(self, other: UsfmUpdateBlock) -> bool: - return ( - self._refs == other._refs - and self._elements == other._elements - and self._row_metadata == other._row_metadata - ) + return self._refs == other._refs and self._elements == other._elements and self._rows == other._rows def copy(self) -> UsfmUpdateBlock: - return UsfmUpdateBlock(self._refs, self._elements, self._row_metadata) + return UsfmUpdateBlock(self._refs, self._elements, self._rows) diff --git a/tests/corpora/test_place_markers_usfm_update_block_handler.py b/tests/corpora/test_place_markers_usfm_update_block_handler.py index aa2ba333..0d5b146b 100644 --- a/tests/corpora/test_place_markers_usfm_update_block_handler.py +++ b/tests/corpora/test_place_markers_usfm_update_block_handler.py @@ -1,8 +1,5 @@ -import logging from typing import List, Optional, Sequence -import pytest - from machine.corpora import ( AlignedWordPair, PlaceMarkersAlignmentInfo, @@ -12,9 +9,12 @@ UpdateUsfmParserHandler, UpdateUsfmRow, UpdateUsfmTextBehavior, + UsfmUpdateBlock, UsfmUpdateBlockHandler, + UsfmUpdateBlockRow, parse_usfm, ) +from machine.corpora.place_markers_usfm_update_block_handler import _get_alignment_info from machine.tokenization import LatinWordTokenizer from machine.translation import WordAlignmentMatrix @@ -943,7 +943,7 @@ def test_anusvara_tokenization() -> None: assert_usfm_equals(target, result) -def test_verse_range_with_empty_trailing_row(caplog: pytest.LogCaptureFixture) -> None: +def test_verse_range_with_empty_trailing_row() -> None: # Verse ranges consist of multiple rows, but only the first one is non-empty and has a non-empty alignment matrix. # An empty alignment matrix must not clobber any non-empty matrices. source = "This is the first part. This is the second part." @@ -982,16 +982,13 @@ def test_verse_range_with_empty_trailing_row(caplog: pytest.LogCaptureFixture) - UpdateUsfmRow(scr_ref("MAT 1:1"), str(pretranslation), metadata={"alignment_info": align_info}), empty_row, ] - with caplog.at_level(logging.WARNING): - target = update_usfm(rows, usfm, update_block_handlers=[PlaceMarkersUsfmUpdateBlockHandler()]) + target = update_usfm(rows, usfm, update_block_handlers=[PlaceMarkersUsfmUpdateBlockHandler()]) assert_usfm_equals(target, result) - # a row that contributed no text is expected, so it must not be reported as unexpected - assert "Expected at most one row with alignment info" not in caplog.text -def test_multiple_rows_with_alignment_info_warns(caplog: pytest.LogCaptureFixture) -> None: - # Only one row's alignment info can be used, so a block matched by several rows that each have - # text of their own is outside what the handler can place markers for +def test_multiple_rows_with_alignment_info_are_merged() -> None: + # A verse range can be matched by several rows that each have text and alignment info of their own, + # e.g. when the rows keep separate verses that the USFM being updated combines first_align_info = PlaceMarkersAlignmentInfo( source_tokens=[t for t in TOKENIZER.tokenize("This is the first part.")], translation_tokens=[t for t in TOKENIZER.tokenize("Esta es la primera parte.")], @@ -1016,16 +1013,328 @@ def test_multiple_rows_with_alignment_info_warns(caplog: pytest.LogCaptureFixtur \p This is the second part. """ - with caplog.at_level(logging.WARNING): - update_usfm(rows, usfm, update_block_handlers=[PlaceMarkersUsfmUpdateBlockHandler()]) + target = update_usfm(rows, usfm, update_block_handlers=[PlaceMarkersUsfmUpdateBlockHandler()]) + + result = r"""\id MAT +\c 1 +\v 1-2 Esta es la primera parte. +\p Esta es la segunda parte. +""" + assert_usfm_equals(target, result) + + +def test_merged_alignment_shifts_later_rows_past_earlier_tokens() -> None: + block = UsfmUpdateBlock( + scr_ref("MAT 1:1", "MAT 1:2"), + rows=[ + UsfmUpdateBlockRow("x y", {"alignment_info": create_alignment_info("a b", "x y", "0-0 1-1")}), + UsfmUpdateBlockRow("z w v", {"alignment_info": create_alignment_info("c d", "z w v", "0-0 1-2")}), + ], + ) + + alignment_info = _get_alignment_info(block) + + assert alignment_info is not None + assert alignment_info["source_tokens"] == ["a", "b", "c", "d"] + assert alignment_info["translation_tokens"] == ["x", "y", "z", "w", "v"] + assert alignment_info["alignment"] == WordAlignmentMatrix.from_word_pairs(4, 5, [(0, 0), (1, 1), (2, 2), (3, 4)]) + + +def test_merged_alignment_shifts_by_token_count_rather_than_matrix_size() -> None: + # The first row's matrix omits its unaligned final token, so it is narrower than the row's tokens + block = UsfmUpdateBlock( + scr_ref("MAT 1:1", "MAT 1:2"), + rows=[ + UsfmUpdateBlockRow("x y .", {"alignment_info": create_alignment_info("a b", "x y .", "0-0 1-1")}), + UsfmUpdateBlockRow("z w v", {"alignment_info": create_alignment_info("c d", "z w v", "0-0 1-2")}), + ], + ) + + alignment_info = _get_alignment_info(block) + + assert alignment_info is not None + assert alignment_info["translation_tokens"] == ["x", "y", ".", "z", "w", "v"] + assert alignment_info["alignment"] == WordAlignmentMatrix.from_word_pairs(4, 6, [(0, 0), (1, 1), (2, 3), (3, 5)]) + + +def test_merged_alignment_ignores_rows_without_text() -> None: + block = UsfmUpdateBlock( + scr_ref("MAT 1:1", "MAT 1:2", "MAT 1:3"), + rows=[ + UsfmUpdateBlockRow("x y", {"alignment_info": create_alignment_info("a b", "x y", "0-0 1-1")}), + UsfmUpdateBlockRow(""), + UsfmUpdateBlockRow("z w v", {"alignment_info": create_alignment_info("c d", "z w v", "0-0 1-2")}), + ], + ) + + alignment_info = _get_alignment_info(block) + + assert alignment_info is not None + assert alignment_info["source_tokens"] == ["a", "b", "c", "d"] + assert alignment_info["translation_tokens"] == ["x", "y", "z", "w", "v"] + assert alignment_info["alignment"] == WordAlignmentMatrix.from_word_pairs(4, 5, [(0, 0), (1, 1), (2, 2), (3, 4)]) + + +def test_merged_alignment_shifts_later_rows_past_rows_without_aligned_words() -> None: + block = UsfmUpdateBlock( + scr_ref("MAT 1:1", "MAT 1:2", "MAT 1:3"), + rows=[ + UsfmUpdateBlockRow("x y", {"alignment_info": create_alignment_info("a b", "x y", "0-0 1-1")}), + UsfmUpdateBlockRow("u t", {"alignment_info": create_alignment_info("e f", "u t", "")}), + UsfmUpdateBlockRow("z w v", {"alignment_info": create_alignment_info("c d", "z w v", "0-0 1-2")}), + ], + ) + + alignment_info = _get_alignment_info(block) + + assert alignment_info is not None + assert alignment_info["source_tokens"] == ["a", "b", "e", "f", "c", "d"] + assert alignment_info["translation_tokens"] == ["x", "y", "u", "t", "z", "w", "v"] + assert alignment_info["alignment"] == WordAlignmentMatrix.from_word_pairs(6, 7, [(0, 0), (1, 1), (4, 4), (5, 6)]) + + +def test_single_row_without_alignment_info_is_not_placed() -> None: + rows = [UpdateUsfmRow(scr_ref("MAT 1:1"), "Esta es la primera parte. Esta es la segunda parte.")] + usfm = r"""\id MAT +\c 1 +\v 1 This is the first part. +\p This is the second part. +""" + + target = update_usfm(rows, usfm, update_block_handlers=[PlaceMarkersUsfmUpdateBlockHandler()]) + + # no markers are placed, so the paragraph marker is left after the text + result = r"""\id MAT +\c 1 +\v 1 Esta es la primera parte. Esta es la segunda parte. +\p +""" + assert_usfm_equals(target, result) - assert "Expected at most one row with alignment info for MAT 1:1, MAT 1:2, but found 2" in caplog.text + +def test_aligned_row_with_row_without_alignment_info_is_not_placed() -> None: + rows = [ + UpdateUsfmRow( + scr_ref("MAT 1:1"), + "Esta es la primera parte.", + metadata={ + "alignment_info": create_alignment_info( + "This is the first part.", "Esta es la primera parte.", "0-0 1-1 2-2 3-3 4-4 5-5" + ) + }, + ), + UpdateUsfmRow(scr_ref("MAT 1:2"), "Esta es la segunda parte."), + ] + usfm = r"""\id MAT +\c 1 +\v 1-2 This is the first part. +\p This is the second part. +""" + + target = update_usfm(rows, usfm, update_block_handlers=[PlaceMarkersUsfmUpdateBlockHandler()]) + + # no markers are placed, so the paragraph marker is left after the text + result = r"""\id MAT +\c 1 +\v 1-2 Esta es la primera parte. Esta es la segunda parte. +\p +""" + assert_usfm_equals(target, result) + + +def test_aligned_rows_with_row_without_alignment_info_are_not_placed() -> None: + # The two aligned rows would be merged, but the row without alignment info prevents placing any markers + rows = [ + UpdateUsfmRow( + scr_ref("MAT 1:1"), + "Esta es la primera parte.", + metadata={ + "alignment_info": create_alignment_info( + "This is the first part.", "Esta es la primera parte.", "0-0 1-1 2-2 3-3 4-4 5-5" + ) + }, + ), + UpdateUsfmRow( + scr_ref("MAT 1:2"), + "Esta es la segunda parte.", + metadata={ + "alignment_info": create_alignment_info( + "This is the second part.", "Esta es la segunda parte.", "0-0 1-1 2-2 3-3 4-4 5-5" + ) + }, + ), + UpdateUsfmRow(scr_ref("MAT 1:3"), "Esta es la tercera parte."), + ] + usfm = r"""\id MAT +\c 1 +\v 1-3 This is the first part. +\p This is the second part. This is the third part. +""" + + target = update_usfm(rows, usfm, update_block_handlers=[PlaceMarkersUsfmUpdateBlockHandler()]) + + # no markers are placed, so the paragraph marker is left after the text + result = r"""\id MAT +\c 1 +\v 1-3 Esta es la primera parte. Esta es la segunda parte. Esta es la tercera parte. +\p +""" + assert_usfm_equals(target, result) + + +def test_single_row_without_aligned_words_places_markers_at_end() -> None: + source = "This is the first part. This is the second part." + pretranslation = "Esta es la primera parte. Esta es la segunda parte." + source_tokens = [t for t in TOKENIZER.tokenize(source)] + translation_tokens = [t for t in TOKENIZER.tokenize(pretranslation)] + align_info = PlaceMarkersAlignmentInfo( + source_tokens=source_tokens, + translation_tokens=translation_tokens, + alignment=WordAlignmentMatrix.from_word_pairs(len(source_tokens), len(translation_tokens)), + paragraph_behavior=UpdateUsfmMarkerBehavior.PRESERVE, + style_behavior=UpdateUsfmMarkerBehavior.PRESERVE, + ) + rows = [UpdateUsfmRow(scr_ref("MAT 1:1"), pretranslation, metadata={"alignment_info": align_info})] + usfm = r"""\id MAT +\c 1 +\v 1 This is the first part. \f + \ft note\f* +\p This is \w the\w* second part. +""" + + target = update_usfm( + rows, + usfm, + style_behavior=UpdateUsfmMarkerBehavior.PRESERVE, + update_block_handlers=[PlaceMarkersUsfmUpdateBlockHandler()], + ) + + # the paragraph and style markers go at the end of the text, followed by the embed + result = r"""\id MAT +\c 1 +\v 1 Esta es la primera parte. Esta es la segunda parte. +\p \w \w*\f + \ft note\f* +""" + assert_usfm_equals(target, result) + + +def test_single_row_with_empty_alignment_matrix_places_markers_at_end() -> None: + source = "This is the first part. This is the second part." + pretranslation = "Esta es la primera parte. Esta es la segunda parte." + align_info = PlaceMarkersAlignmentInfo( + source_tokens=[t for t in TOKENIZER.tokenize(source)], + translation_tokens=[t for t in TOKENIZER.tokenize(pretranslation)], + alignment=to_word_alignment_matrix(""), + paragraph_behavior=UpdateUsfmMarkerBehavior.PRESERVE, + style_behavior=UpdateUsfmMarkerBehavior.PRESERVE, + ) + rows = [UpdateUsfmRow(scr_ref("MAT 1:1"), pretranslation, metadata={"alignment_info": align_info})] + usfm = r"""\id MAT +\c 1 +\v 1 This is the first part. \f + \ft note\f* +\p This is \w the\w* second part. +""" + + target = update_usfm( + rows, + usfm, + style_behavior=UpdateUsfmMarkerBehavior.PRESERVE, + update_block_handlers=[PlaceMarkersUsfmUpdateBlockHandler()], + ) + + # the paragraph and style markers go at the end of the text, followed by the embed + result = r"""\id MAT +\c 1 +\v 1 Esta es la primera parte. Esta es la segunda parte. +\p \w \w*\f + \ft note\f* +""" + assert_usfm_equals(target, result) + + +def test_merged_rows_without_aligned_words_place_markers_at_end() -> None: + first_align_info = PlaceMarkersAlignmentInfo( + source_tokens=[t for t in TOKENIZER.tokenize("This is the first part.")], + translation_tokens=[t for t in TOKENIZER.tokenize("Esta es la primera parte.")], + alignment=to_word_alignment_matrix(""), + paragraph_behavior=UpdateUsfmMarkerBehavior.PRESERVE, + style_behavior=UpdateUsfmMarkerBehavior.PRESERVE, + ) + second_align_info = PlaceMarkersAlignmentInfo( + source_tokens=[t for t in TOKENIZER.tokenize("This is the second part.")], + translation_tokens=[t for t in TOKENIZER.tokenize("Esta es la segunda parte.")], + alignment=to_word_alignment_matrix(""), + paragraph_behavior=UpdateUsfmMarkerBehavior.PRESERVE, + style_behavior=UpdateUsfmMarkerBehavior.PRESERVE, + ) + rows = [ + UpdateUsfmRow(scr_ref("MAT 1:1"), "Esta es la primera parte.", metadata={"alignment_info": first_align_info}), + UpdateUsfmRow(scr_ref("MAT 1:2"), "Esta es la segunda parte.", metadata={"alignment_info": second_align_info}), + ] + usfm = r"""\id MAT +\c 1 +\v 1-2 This is the first part. \f + \ft note\f* +\p This is \w the\w* second part. +""" + + target = update_usfm( + rows, + usfm, + style_behavior=UpdateUsfmMarkerBehavior.PRESERVE, + update_block_handlers=[PlaceMarkersUsfmUpdateBlockHandler()], + ) + + # the paragraph and style markers go at the end of the text, followed by the embed + result = r"""\id MAT +\c 1 +\v 1-2 Esta es la primera parte. Esta es la segunda parte. +\p \w \w*\f + \ft note\f* +""" + assert_usfm_equals(target, result) + + +def test_end_style_marker_follows_start_marker() -> None: + # Reordered words can predict the end marker before the start marker + source = "This is the big dog." + pretranslation = "Este es el perro grande." + align_info = PlaceMarkersAlignmentInfo( + source_tokens=[t for t in TOKENIZER.tokenize(source)], + translation_tokens=[t for t in TOKENIZER.tokenize(pretranslation)], + alignment=to_word_alignment_matrix("0-0 1-1 2-2 3-4 4-3 5-5"), + paragraph_behavior=UpdateUsfmMarkerBehavior.PRESERVE, + style_behavior=UpdateUsfmMarkerBehavior.PRESERVE, + ) + rows = [UpdateUsfmRow(scr_ref("MAT 1:1"), pretranslation, metadata={"alignment_info": align_info})] + usfm = r"""\id MAT +\c 1 +\v 1 This is the \w big\w* dog. +""" + + target = update_usfm( + rows, + usfm, + style_behavior=UpdateUsfmMarkerBehavior.PRESERVE, + update_block_handlers=[PlaceMarkersUsfmUpdateBlockHandler()], + ) + result = r"""\id MAT +\c 1 +\v 1 Este es el \w perro\w* grande. +""" + assert_usfm_equals(target, result) def scr_ref(*refs: str) -> List[ScriptureRef]: return [ScriptureRef.parse(ref) for ref in refs] +def create_alignment_info(source: str, translation: str, alignment: str) -> PlaceMarkersAlignmentInfo: + return PlaceMarkersAlignmentInfo( + source_tokens=[t for t in TOKENIZER.tokenize(source)], + translation_tokens=[t for t in TOKENIZER.tokenize(translation)], + alignment=to_word_alignment_matrix(alignment), + paragraph_behavior=UpdateUsfmMarkerBehavior.PRESERVE, + style_behavior=UpdateUsfmMarkerBehavior.STRIP, + ) + + def to_word_alignment_matrix(alignment_str: str) -> WordAlignmentMatrix: word_pairs = AlignedWordPair.from_string(alignment_str) row_count = 0 From c8973a59de2b24bb20c4b8d293476199bad18415 Mon Sep 17 00:00:00 2001 From: Ben King Date: Fri, 9 Oct 2026 18:30:23 -0400 Subject: [PATCH 4/5] Address review findings on placing markers for merged rows Each row's alignment info describes only that row's text, so it is now always merged, even when another row has the same info. Deduplicating by identity broke rows that held equal copies, and deduplicating by equality would drop a repeated verse whose text and alignment are identical. Aligned word pairs outside a row's tokens are now rejected for every row, not only when rows are merged. A single row with such a pair used to place markers silently or fail with an IndexError that bypassed the error handler. Blocks with no markers to place return before their alignment is read, so they no longer log warnings about missing alignment info. The rows argument of UsfmUpdateBlock is keyword-only, so a metadata dict passed where it used to go raises a TypeError instead of being taken as rows. Co-Authored-By: Claude Opus 5.5 (1M context) --- ...place_markers_usfm_update_block_handler.py | 42 +++--- machine/corpora/update_usfm_parser_handler.py | 6 +- machine/corpora/usfm_update_block.py | 4 +- ...place_markers_usfm_update_block_handler.py | 129 ++++++++++++------ 4 files changed, 116 insertions(+), 65 deletions(-) diff --git a/machine/corpora/place_markers_usfm_update_block_handler.py b/machine/corpora/place_markers_usfm_update_block_handler.py index 03dcb277..8822286f 100644 --- a/machine/corpora/place_markers_usfm_update_block_handler.py +++ b/machine/corpora/place_markers_usfm_update_block_handler.py @@ -36,14 +36,15 @@ def _get_alignment_info(block: UsfmUpdateBlock) -> Optional[PlaceMarkersAlignmen "Markers were not placed for %s because a row with text has no alignment info.", _format_refs(block) ) return None - # The same alignment info on several rows already describes their combined text - if not any(info is other for other in infos): - infos.append(info) + _check_aligned_word_pairs(block, info) + # Each row's alignment info describes only that row's text, so it is merged even if another row has the + # same info, e.g. for repeated text + infos.append(info) if len(infos) == 0: return None # A block matched by several rows, e.g. a verse range, has those rows' texts concatenated - alignment_info = infos[0] if len(infos) == 1 else _merge_alignment_infos(block, infos) + alignment_info = infos[0] if len(infos) == 1 else _merge_alignment_infos(infos) alignment = alignment_info["alignment"] if not any(alignment.is_row_aligned(i) for i in range(alignment.row_count)): # Markers are still placed, but with nothing to align them to, they all go at the end of the text @@ -54,21 +55,23 @@ def _get_alignment_info(block: UsfmUpdateBlock) -> Optional[PlaceMarkersAlignmen return alignment_info -def _merge_alignment_infos( - block: UsfmUpdateBlock, infos: Sequence[PlaceMarkersAlignmentInfo] -) -> PlaceMarkersAlignmentInfo: +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(): - 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)}.", - ) 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"]) @@ -93,12 +96,19 @@ def __init__(self, *args): 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 + # Nothing to do if there are no markers to place or no alignment to use. + # Check for markers first, so that blocks without any don't log warnings about their alignment, + # then again with the alignment info's behaviors, which can only rule out more markers. + if not any( + e.is_placeable(UpdateUsfmMarkerBehavior.PRESERVE, UpdateUsfmMarkerBehavior.PRESERVE) for e in elements + ): + return block + alignment_info = _get_alignment_info(block) if alignment_info is None: return block - if len(elements) == 0 or not any( + if not any( e.is_placeable(alignment_info["paragraph_behavior"], alignment_info["style_behavior"]) for e in elements ): return block @@ -249,7 +259,7 @@ def process_block(self, block: UsfmUpdateBlock) -> UsfmUpdateBlock: while len(header_elements) > 0: placed_elements.append(header_elements.pop(0)[1]) - return UsfmUpdateBlock(block.refs, placed_elements + ignored_elements, block.rows) + return UsfmUpdateBlock(block.refs, placed_elements + ignored_elements, rows=block.rows) def _predict_marker_location( self, diff --git a/machine/corpora/update_usfm_parser_handler.py b/machine/corpora/update_usfm_parser_handler.py index 6084666b..ff85e841 100644 --- a/machine/corpora/update_usfm_parser_handler.py +++ b/machine/corpora/update_usfm_parser_handler.py @@ -58,7 +58,7 @@ def extend_last_element(self, tokens: Iterable[UsfmToken]) -> None: def update_refs(self, refs: Iterable[ScriptureRef]) -> None: self._refs = list(refs) - def is_empty(self) -> bool: + def has_no_elements(self) -> bool: return len(self._elements) == 0 def get_last_element(self) -> UsfmUpdateBlockElement: @@ -74,7 +74,7 @@ def pop(self) -> UsfmUpdateBlockElement: return self._elements.pop() def build(self) -> UsfmUpdateBlock: - return UsfmUpdateBlock(self._refs, self._elements, self._rows) + return UsfmUpdateBlock(self._refs, self._elements, rows=self._rows) def _sanitize_verse_data(verse_data: str) -> str: @@ -545,7 +545,7 @@ def _end_update_block(self, state: UsfmParserState, scripture_refs: Sequence[Scr # Strip off any non-verse paragraphs that are at the end of the update block para_elems: list[UsfmUpdateBlockElement] = [] - while not update_block_builder.is_empty() and _is_nonverse_paragraph( + 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()) diff --git a/machine/corpora/usfm_update_block.py b/machine/corpora/usfm_update_block.py index b605d960..7501cf21 100644 --- a/machine/corpora/usfm_update_block.py +++ b/machine/corpora/usfm_update_block.py @@ -19,6 +19,8 @@ def __init__( self, refs: Iterable[ScriptureRef] = [], elements: Iterable[UsfmUpdateBlockElement] = [], + # Keyword-only, so that a metadata dict passed where it used to go is rejected + *, rows: Iterable[UsfmUpdateBlockRow] = [], ) -> None: self._refs: list[ScriptureRef] = list(refs) @@ -46,4 +48,4 @@ def __eq__(self, other: UsfmUpdateBlock) -> bool: return self._refs == other._refs and self._elements == other._elements and self._rows == other._rows def copy(self) -> UsfmUpdateBlock: - return UsfmUpdateBlock(self._refs, self._elements, self._rows) + return UsfmUpdateBlock(self._refs, self._elements, rows=self._rows) diff --git a/tests/corpora/test_place_markers_usfm_update_block_handler.py b/tests/corpora/test_place_markers_usfm_update_block_handler.py index 0d5b146b..5ab9bcfa 100644 --- a/tests/corpora/test_place_markers_usfm_update_block_handler.py +++ b/tests/corpora/test_place_markers_usfm_update_block_handler.py @@ -1,5 +1,8 @@ +import logging from typing import List, Optional, Sequence +from pytest import LogCaptureFixture, raises + from machine.corpora import ( AlignedWordPair, PlaceMarkersAlignmentInfo, @@ -15,6 +18,7 @@ parse_usfm, ) from machine.corpora.place_markers_usfm_update_block_handler import _get_alignment_info +from machine.corpora.usfm_update_block_handler import UsfmUpdateBlockHandlerError from machine.tokenization import LatinWordTokenizer from machine.translation import WordAlignmentMatrix @@ -874,18 +878,23 @@ def test_other_elements_do_not_affect_embed_placement() -> None: def test_multiple_text_rows_in_verse_ranges_are_updated() -> None: # Verse ranges contain multiple text rows, which must be processed as if they were a single row - source = "This is the first part. This is the second part." - pretranslation = "Esta es la primera parte. Esta es la segunda parte." - align_info = PlaceMarkersAlignmentInfo( - source_tokens=[t for t in TOKENIZER.tokenize(source)], - translation_tokens=[t for t in TOKENIZER.tokenize(pretranslation)], - alignment=to_word_alignment_matrix("0-0 1-1 2-2 3-3 4-4 5-5 6-6 7-7 8-8 9-9 10-10"), + first_align_info = PlaceMarkersAlignmentInfo( + source_tokens=[t for t in TOKENIZER.tokenize("This is the first part.")], + translation_tokens=[t for t in TOKENIZER.tokenize("Esta es la primera parte.")], + alignment=to_word_alignment_matrix("0-0 1-1 2-2 3-3 4-4 5-5"), + paragraph_behavior=UpdateUsfmMarkerBehavior.PRESERVE, + style_behavior=UpdateUsfmMarkerBehavior.STRIP, + ) + second_align_info = PlaceMarkersAlignmentInfo( + source_tokens=[t for t in TOKENIZER.tokenize("This is the second part.")], + translation_tokens=[t for t in TOKENIZER.tokenize("Esta es la segunda parte.")], + alignment=to_word_alignment_matrix("0-0 1-1 2-2 3-3 4-4 5-5"), paragraph_behavior=UpdateUsfmMarkerBehavior.PRESERVE, style_behavior=UpdateUsfmMarkerBehavior.STRIP, ) rows = [ - UpdateUsfmRow(scr_ref("MAT 1:1"), "Esta es la primera parte.", metadata={"alignment_info": align_info}), - UpdateUsfmRow(scr_ref("MAT 1:2"), "Esta es la segunda parte.", metadata={"alignment_info": align_info}), + UpdateUsfmRow(scr_ref("MAT 1:1"), "Esta es la primera parte.", metadata={"alignment_info": first_align_info}), + UpdateUsfmRow(scr_ref("MAT 1:2"), "Esta es la segunda parte.", metadata={"alignment_info": second_align_info}), ] usfm = r"""\id MAT \c 1 @@ -902,6 +911,28 @@ def test_multiple_text_rows_in_verse_ranges_are_updated() -> None: assert_usfm_equals(target, result) +def test_rows_with_the_same_alignment_info_are_each_merged() -> None: + # Rows with repeated text can share an alignment info, which still describes only one row's text + align_info = create_alignment_info("Amen.", "Amén.", "0-0 1-1") + rows = [ + UpdateUsfmRow(scr_ref("MAT 1:1"), "Amén.", metadata={"alignment_info": align_info}), + UpdateUsfmRow(scr_ref("MAT 1:2"), "Amén.", metadata={"alignment_info": align_info}), + ] + usfm = r"""\id MAT +\c 1 +\v 1-2 Amen. +\p Amen. +""" + + target = update_usfm(rows, usfm, update_block_handlers=[PlaceMarkersUsfmUpdateBlockHandler()]) + result = r"""\id MAT +\c 1 +\v 1-2 Amén. +\p Amén. +""" + assert_usfm_equals(target, result) + + def test_anusvara_tokenization() -> None: # The anusvara ("ं") is the third to last character in the update rows string source = ( @@ -986,43 +1017,6 @@ def test_verse_range_with_empty_trailing_row() -> None: assert_usfm_equals(target, result) -def test_multiple_rows_with_alignment_info_are_merged() -> None: - # A verse range can be matched by several rows that each have text and alignment info of their own, - # e.g. when the rows keep separate verses that the USFM being updated combines - first_align_info = PlaceMarkersAlignmentInfo( - source_tokens=[t for t in TOKENIZER.tokenize("This is the first part.")], - translation_tokens=[t for t in TOKENIZER.tokenize("Esta es la primera parte.")], - alignment=to_word_alignment_matrix("0-0 1-1 2-2 3-3 4-4 5-5"), - paragraph_behavior=UpdateUsfmMarkerBehavior.PRESERVE, - style_behavior=UpdateUsfmMarkerBehavior.STRIP, - ) - second_align_info = PlaceMarkersAlignmentInfo( - source_tokens=[t for t in TOKENIZER.tokenize("This is the second part.")], - translation_tokens=[t for t in TOKENIZER.tokenize("Esta es la segunda parte.")], - alignment=to_word_alignment_matrix("0-0 1-1 2-2 3-3 4-4 5-5"), - paragraph_behavior=UpdateUsfmMarkerBehavior.PRESERVE, - style_behavior=UpdateUsfmMarkerBehavior.STRIP, - ) - rows = [ - UpdateUsfmRow(scr_ref("MAT 1:1"), "Esta es la primera parte.", metadata={"alignment_info": first_align_info}), - UpdateUsfmRow(scr_ref("MAT 1:2"), "Esta es la segunda parte.", metadata={"alignment_info": second_align_info}), - ] - usfm = r"""\id MAT -\c 1 -\v 1-2 This is the first part. -\p This is the second part. -""" - - target = update_usfm(rows, usfm, update_block_handlers=[PlaceMarkersUsfmUpdateBlockHandler()]) - - result = r"""\id MAT -\c 1 -\v 1-2 Esta es la primera parte. -\p Esta es la segunda parte. -""" - assert_usfm_equals(target, result) - - def test_merged_alignment_shifts_later_rows_past_earlier_tokens() -> None: block = UsfmUpdateBlock( scr_ref("MAT 1:1", "MAT 1:2"), @@ -1093,6 +1087,51 @@ def test_merged_alignment_shifts_later_rows_past_rows_without_aligned_words() -> assert alignment_info["alignment"] == WordAlignmentMatrix.from_word_pairs(6, 7, [(0, 0), (1, 1), (4, 4), (5, 6)]) +def test_aligned_word_pair_outside_tokens_of_single_row_is_rejected() -> None: + block = UsfmUpdateBlock( + scr_ref("MAT 1:1"), + rows=[UsfmUpdateBlockRow("x y", {"alignment_info": create_alignment_info("a b", "x y", "0-0 1-5")})], + ) + + with raises(UsfmUpdateBlockHandlerError, match="1-5"): + _get_alignment_info(block) + + +def test_aligned_word_pair_outside_tokens_of_merged_row_is_rejected() -> None: + # Merging would otherwise shift the pair into the next row's tokens + block = UsfmUpdateBlock( + scr_ref("MAT 1:1", "MAT 1:2"), + rows=[ + UsfmUpdateBlockRow("x y", {"alignment_info": create_alignment_info("a b", "x y", "0-0 2-1")}), + UsfmUpdateBlockRow("z w", {"alignment_info": create_alignment_info("c d", "z w", "0-0 1-1")}), + ], + ) + + with raises(UsfmUpdateBlockHandlerError, match="2-1"): + _get_alignment_info(block) + + +def test_only_blocks_with_markers_warn_about_missing_alignment_info(caplog: LogCaptureFixture) -> None: + rows = [ + UpdateUsfmRow(scr_ref("MAT 1:1"), "Uno."), + UpdateUsfmRow(scr_ref("MAT 1:2"), "Dos. Tres."), + ] + usfm = r"""\id MAT +\c 1 +\v 1 One. +\v 2 Two. +\p Three. +""" + + with caplog.at_level(logging.WARNING): + update_usfm(rows, usfm, update_block_handlers=[PlaceMarkersUsfmUpdateBlockHandler()]) + + # verse 1 has no markers to place, so only verse 2 warns + assert [r.getMessage() for r in caplog.records] == [ + "Markers were not placed for MAT 1:2 because a row with text has no alignment info." + ] + + def test_single_row_without_alignment_info_is_not_placed() -> None: rows = [UpdateUsfmRow(scr_ref("MAT 1:1"), "Esta es la primera parte. Esta es la segunda parte.")] usfm = r"""\id MAT From 414db9d19604c58722a01cc1112f1a9088094d85 Mon Sep 17 00:00:00 2001 From: Ben King Date: Tue, 13 Oct 2026 13:29:37 -0400 Subject: [PATCH 5/5] Check the alignment only for blocks with markers to place A block whose alignment info strips every marker it has is left alone, so its alignment is never used. Checking the aligned word pairs while reading the alignment info, before the info's behaviors were known, let such a block fail the whole update. The pairs are now checked only once there are markers to place, and still before rows are merged. Also trim the comments added on this branch to the non-obvious reasons behind the code. Co-Authored-By: Claude Opus 5.5 (1M context) --- ...place_markers_usfm_update_block_handler.py | 51 +++++++++-------- machine/corpora/usfm_update_block.py | 4 +- ...place_markers_usfm_update_block_handler.py | 57 ++++++++++++------- 3 files changed, 66 insertions(+), 46 deletions(-) diff --git a/machine/corpora/place_markers_usfm_update_block_handler.py b/machine/corpora/place_markers_usfm_update_block_handler.py index 8822286f..31165cf3 100644 --- a/machine/corpora/place_markers_usfm_update_block_handler.py +++ b/machine/corpora/place_markers_usfm_update_block_handler.py @@ -24,7 +24,7 @@ class PlaceMarkersAlignmentInfo(TypedDict): style_behavior: UpdateUsfmMarkerBehavior -def _get_alignment_info(block: UsfmUpdateBlock) -> Optional[PlaceMarkersAlignmentInfo]: +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 @@ -36,18 +36,21 @@ def _get_alignment_info(block: UsfmUpdateBlock) -> Optional[PlaceMarkersAlignmen "Markers were not placed for %s because a row with text has no alignment info.", _format_refs(block) ) return None - _check_aligned_word_pairs(block, info) - # Each row's alignment info describes only that row's text, so it is merged even if another row has the - # same info, e.g. for repeated text infos.append(info) + return infos if len(infos) > 0 else None + - if len(infos) == 0: - return None - # A block matched by several rows, e.g. a verse range, has those rows' texts concatenated - alignment_info = infos[0] if len(infos) == 1 else _merge_alignment_infos(infos) +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)): - # Markers are still placed, but with nothing to align them to, they all go at the end of the text logger.warning( "The alignment for %s has no aligned words, so its markers were placed at the end of its text.", _format_refs(block), @@ -96,23 +99,26 @@ def __init__(self, *args): 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. - # Check for markers first, so that blocks without any don't log warnings about their alignment, - # then again with the alignment info's behaviors, which can only rule out more markers. + # Nothing to do if there are no markers to place or no alignment to use + # 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 = _get_alignment_info(block) - if alignment_info is None: + row_alignment_infos = _get_row_alignment_infos(block) + if row_alignment_infos is None: return block - if not any( - e.is_placeable(alignment_info["paragraph_behavior"], alignment_info["style_behavior"]) for e in elements - ): + # 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 = [] @@ -197,8 +203,7 @@ def process_block(self, block: UsfmUpdateBlock) -> UsfmUpdateBlock: # Predict marker placements and get insertion order to_insert = [] - # The predicted target token of each open style marker, by marker - open_style_trg_toks: Dict[str, List[int]] = {} + 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) @@ -212,15 +217,15 @@ def process_block(self, block: UsfmUpdateBlock) -> UsfmUpdateBlock: if element.type == UsfmUpdateBlockElementType.STYLE: marker = element.tokens[0].marker if is_end_style: - opening_trg_toks = open_style_trg_toks.get(marker[:-1]) + opening_trg_toks = open_style_trg_toks_by_marker.get(marker[:-1]) if opening_trg_toks: - # Move a misplaced closing style marker to one token after its opening marker + # 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.setdefault(marker, []).append(adj_trg_tok) + open_style_trg_toks_by_marker.setdefault(marker, []).append(adj_trg_tok) if is_end_style and 0 < adj_trg_tok <= len(trg_toks): - # Place closing style marker at the end of the token + # 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): trg_str_idx = trg_tok_starts[adj_trg_tok] diff --git a/machine/corpora/usfm_update_block.py b/machine/corpora/usfm_update_block.py index 7501cf21..a0086987 100644 --- a/machine/corpora/usfm_update_block.py +++ b/machine/corpora/usfm_update_block.py @@ -19,14 +19,12 @@ def __init__( self, refs: Iterable[ScriptureRef] = [], elements: Iterable[UsfmUpdateBlockElement] = [], - # Keyword-only, so that a metadata dict passed where it used to go is rejected *, rows: Iterable[UsfmUpdateBlockRow] = [], ) -> None: self._refs: list[ScriptureRef] = list(refs) self._elements: list[UsfmUpdateBlockElement] = list(elements) - # One entry per row matched to this block, in order. A verse range can be matched by - # several rows, in which case this block's text is those rows' texts concatenated. + # Several rows can match a verse range, and this block's text is their texts concatenated in order self._rows: list[UsfmUpdateBlockRow] = list(rows) @property diff --git a/tests/corpora/test_place_markers_usfm_update_block_handler.py b/tests/corpora/test_place_markers_usfm_update_block_handler.py index 5ab9bcfa..e5e74781 100644 --- a/tests/corpora/test_place_markers_usfm_update_block_handler.py +++ b/tests/corpora/test_place_markers_usfm_update_block_handler.py @@ -17,7 +17,7 @@ UsfmUpdateBlockRow, parse_usfm, ) -from machine.corpora.place_markers_usfm_update_block_handler import _get_alignment_info +from machine.corpora.place_markers_usfm_update_block_handler import _get_block_alignment_info, _get_row_alignment_infos from machine.corpora.usfm_update_block_handler import UsfmUpdateBlockHandlerError from machine.tokenization import LatinWordTokenizer from machine.translation import WordAlignmentMatrix @@ -846,6 +846,31 @@ def test_marker_behavior_disagrees_with_alignment_info() -> None: assert_usfm_equals(target, result) +def test_alignment_is_not_checked_when_alignment_info_behaviors_leave_nothing_to_place() -> None: + # The updater keeps the paragraph marker that the info strips, and pair 1-9 is outside the tokens + align_info = PlaceMarkersAlignmentInfo( + source_tokens=[t for t in TOKENIZER.tokenize("One two. Three.")], + translation_tokens=[t for t in TOKENIZER.tokenize("Uno dos. Tres.")], + alignment=to_word_alignment_matrix("0-0 1-9"), + paragraph_behavior=UpdateUsfmMarkerBehavior.STRIP, + style_behavior=UpdateUsfmMarkerBehavior.STRIP, + ) + rows = [UpdateUsfmRow(scr_ref("MAT 1:1"), "Uno dos. Tres.", metadata={"alignment_info": align_info})] + usfm = r"""\id MAT +\c 1 +\v 1 One two. +\p Three. +""" + + target = update_usfm(rows, usfm, update_block_handlers=[PlaceMarkersUsfmUpdateBlockHandler()]) + result = r"""\id MAT +\c 1 +\v 1 Uno dos. Tres. +\p +""" + assert_usfm_equals(target, result) + + def test_other_elements_do_not_affect_embed_placement() -> None: # Attributes, milestones and the like are never transferred, so their presence must not # change where anything else lands - here, an embed before end-of-verse paragraph markers. @@ -975,8 +1000,6 @@ def test_anusvara_tokenization() -> None: def test_verse_range_with_empty_trailing_row() -> None: - # Verse ranges consist of multiple rows, but only the first one is non-empty and has a non-empty alignment matrix. - # An empty alignment matrix must not clobber any non-empty matrices. source = "This is the first part. This is the second part." pretranslation = "Esta es la primera parte. Esta es la segunda parte." align_info = PlaceMarkersAlignmentInfo( @@ -1004,7 +1027,6 @@ def test_verse_range_with_empty_trailing_row() -> None: \p Esta es la segunda parte. """ - # the result should be the same whether the second row has an empty alignment matrix or no alignment matrix at all for empty_row in [ UpdateUsfmRow(scr_ref("MAT 1:2"), "", metadata={"alignment_info": empty_align_info}), UpdateUsfmRow(scr_ref("MAT 1:2"), ""), @@ -1026,7 +1048,7 @@ def test_merged_alignment_shifts_later_rows_past_earlier_tokens() -> None: ], ) - alignment_info = _get_alignment_info(block) + alignment_info = get_alignment_info(block) assert alignment_info is not None assert alignment_info["source_tokens"] == ["a", "b", "c", "d"] @@ -1044,7 +1066,7 @@ def test_merged_alignment_shifts_by_token_count_rather_than_matrix_size() -> Non ], ) - alignment_info = _get_alignment_info(block) + alignment_info = get_alignment_info(block) assert alignment_info is not None assert alignment_info["translation_tokens"] == ["x", "y", ".", "z", "w", "v"] @@ -1061,7 +1083,7 @@ def test_merged_alignment_ignores_rows_without_text() -> None: ], ) - alignment_info = _get_alignment_info(block) + alignment_info = get_alignment_info(block) assert alignment_info is not None assert alignment_info["source_tokens"] == ["a", "b", "c", "d"] @@ -1079,7 +1101,7 @@ def test_merged_alignment_shifts_later_rows_past_rows_without_aligned_words() -> ], ) - alignment_info = _get_alignment_info(block) + alignment_info = get_alignment_info(block) assert alignment_info is not None assert alignment_info["source_tokens"] == ["a", "b", "e", "f", "c", "d"] @@ -1094,11 +1116,10 @@ def test_aligned_word_pair_outside_tokens_of_single_row_is_rejected() -> None: ) with raises(UsfmUpdateBlockHandlerError, match="1-5"): - _get_alignment_info(block) + get_alignment_info(block) def test_aligned_word_pair_outside_tokens_of_merged_row_is_rejected() -> None: - # Merging would otherwise shift the pair into the next row's tokens block = UsfmUpdateBlock( scr_ref("MAT 1:1", "MAT 1:2"), rows=[ @@ -1108,7 +1129,7 @@ def test_aligned_word_pair_outside_tokens_of_merged_row_is_rejected() -> None: ) with raises(UsfmUpdateBlockHandlerError, match="2-1"): - _get_alignment_info(block) + get_alignment_info(block) def test_only_blocks_with_markers_warn_about_missing_alignment_info(caplog: LogCaptureFixture) -> None: @@ -1126,7 +1147,6 @@ def test_only_blocks_with_markers_warn_about_missing_alignment_info(caplog: LogC with caplog.at_level(logging.WARNING): update_usfm(rows, usfm, update_block_handlers=[PlaceMarkersUsfmUpdateBlockHandler()]) - # verse 1 has no markers to place, so only verse 2 warns assert [r.getMessage() for r in caplog.records] == [ "Markers were not placed for MAT 1:2 because a row with text has no alignment info." ] @@ -1142,7 +1162,6 @@ def test_single_row_without_alignment_info_is_not_placed() -> None: target = update_usfm(rows, usfm, update_block_handlers=[PlaceMarkersUsfmUpdateBlockHandler()]) - # no markers are placed, so the paragraph marker is left after the text result = r"""\id MAT \c 1 \v 1 Esta es la primera parte. Esta es la segunda parte. @@ -1172,7 +1191,6 @@ def test_aligned_row_with_row_without_alignment_info_is_not_placed() -> None: target = update_usfm(rows, usfm, update_block_handlers=[PlaceMarkersUsfmUpdateBlockHandler()]) - # no markers are placed, so the paragraph marker is left after the text result = r"""\id MAT \c 1 \v 1-2 Esta es la primera parte. Esta es la segunda parte. @@ -1182,7 +1200,6 @@ def test_aligned_row_with_row_without_alignment_info_is_not_placed() -> None: def test_aligned_rows_with_row_without_alignment_info_are_not_placed() -> None: - # The two aligned rows would be merged, but the row without alignment info prevents placing any markers rows = [ UpdateUsfmRow( scr_ref("MAT 1:1"), @@ -1212,7 +1229,6 @@ def test_aligned_rows_with_row_without_alignment_info_are_not_placed() -> None: target = update_usfm(rows, usfm, update_block_handlers=[PlaceMarkersUsfmUpdateBlockHandler()]) - # no markers are placed, so the paragraph marker is left after the text result = r"""\id MAT \c 1 \v 1-3 Esta es la primera parte. Esta es la segunda parte. Esta es la tercera parte. @@ -1247,7 +1263,6 @@ def test_single_row_without_aligned_words_places_markers_at_end() -> None: update_block_handlers=[PlaceMarkersUsfmUpdateBlockHandler()], ) - # the paragraph and style markers go at the end of the text, followed by the embed result = r"""\id MAT \c 1 \v 1 Esta es la primera parte. Esta es la segunda parte. @@ -1280,7 +1295,6 @@ def test_single_row_with_empty_alignment_matrix_places_markers_at_end() -> None: update_block_handlers=[PlaceMarkersUsfmUpdateBlockHandler()], ) - # the paragraph and style markers go at the end of the text, followed by the embed result = r"""\id MAT \c 1 \v 1 Esta es la primera parte. Esta es la segunda parte. @@ -1321,7 +1335,6 @@ def test_merged_rows_without_aligned_words_place_markers_at_end() -> None: update_block_handlers=[PlaceMarkersUsfmUpdateBlockHandler()], ) - # the paragraph and style markers go at the end of the text, followed by the embed result = r"""\id MAT \c 1 \v 1-2 Esta es la primera parte. Esta es la segunda parte. @@ -1331,7 +1344,6 @@ def test_merged_rows_without_aligned_words_place_markers_at_end() -> None: def test_end_style_marker_follows_start_marker() -> None: - # Reordered words can predict the end marker before the start marker source = "This is the big dog." pretranslation = "Este es el perro grande." align_info = PlaceMarkersAlignmentInfo( @@ -1364,6 +1376,11 @@ def scr_ref(*refs: str) -> List[ScriptureRef]: return [ScriptureRef.parse(ref) for ref in refs] +def get_alignment_info(block: UsfmUpdateBlock) -> Optional[PlaceMarkersAlignmentInfo]: + row_alignment_infos = _get_row_alignment_infos(block) + return None if row_alignment_infos is None else _get_block_alignment_info(block, row_alignment_infos) + + def create_alignment_info(source: str, translation: str, alignment: str) -> PlaceMarkersAlignmentInfo: return PlaceMarkersAlignmentInfo( source_tokens=[t for t in TOKENIZER.tokenize(source)],