From faa70545eb40533d40d7f99717fb0ceb1006db21 Mon Sep 17 00:00:00 2001 From: Research Assistant Date: Fri, 26 Jun 2026 18:36:04 +0800 Subject: [PATCH] fix: resolve figure_id collisions via name mutation When two figures share the same figure_id (e.g. supplementary "Figure S.1" and main "Figure 1" both mapped to "figure_001"), the second occurrence is renamed with an "s" prefix: figure_001 -> figure_s001, figure_ss001, etc. This avoids file overwrite in ocr_objects.py and ensures both body and supplementary figures appear at their correct positions in the rendered fulltext. Changes: - _resolve_figure_id_collisions() in ocr_figures.py - figure_id passthrough in ocr_figure_reader.py (reader + normalize) - _reader_figure_embed_target prefers explicit figure_id - 11 unit tests covering single/triple/multi collisions, mixed buckets, empty IDs, realistic body+supplementary layout --- paperforge/worker/ocr_figure_reader.py | 2 + paperforge/worker/ocr_figures.py | 19 +++++ paperforge/worker/ocr_render.py | 5 +- tests/test_ocr_figures.py | 109 +++++++++++++++++++++++++ 4 files changed, 134 insertions(+), 1 deletion(-) diff --git a/paperforge/worker/ocr_figure_reader.py b/paperforge/worker/ocr_figure_reader.py index c69f24a1..0215e97f 100644 --- a/paperforge/worker/ocr_figure_reader.py +++ b/paperforge/worker/ocr_figure_reader.py @@ -128,6 +128,7 @@ def _normalize_bucket( normalized.append( { + "figure_id": source_item.get("figure_id", ""), "figure_number": source_item.get("figure_number") or source_marker.get("number") or _inferred_figure_number, @@ -272,6 +273,7 @@ def _materialize_reader_figure( first_asset_block_id=asset_ids[0] if asset_ids else None, ordinal=ordinal, ), + "figure_id": normalized_item.get("figure_id", ""), "figure_number": figure_number, "reader_status": reader_status, "strict_status": strict_status, diff --git a/paperforge/worker/ocr_figures.py b/paperforge/worker/ocr_figures.py index a2231aff..6594b30b 100644 --- a/paperforge/worker/ocr_figures.py +++ b/paperforge/worker/ocr_figures.py @@ -2601,6 +2601,23 @@ def _settle_cross_page_reserved_objects( ) +def _resolve_figure_id_collisions(figure_inventory: dict) -> None: + _collision_seen: dict[str, int] = {} + for _fig in ( + *figure_inventory.get("matched_figures", []), + *figure_inventory.get("held_figures", []), + *figure_inventory.get("ambiguous_figures", []), + ): + _fig_id = _fig.get("figure_id", "") + if not _fig_id: + continue + if _fig_id in _collision_seen: + _collision_seen[_fig_id] += 1 + _fig["figure_id"] = f"figure_{'s' * _collision_seen[_fig_id]}{_fig_id.removeprefix('figure_')}" + else: + _collision_seen[_fig_id] = 0 + + def build_figure_inventory(structured_blocks: list[dict], page_width: float = 1200) -> dict[str, Any]: legends: list[dict] = [] held_figures: list[dict] = [] @@ -4259,6 +4276,8 @@ def build_figure_inventory(structured_blocks: list[dict], page_width: float = 12 inventory, ) + _resolve_figure_id_collisions(inventory) + return inventory diff --git a/paperforge/worker/ocr_render.py b/paperforge/worker/ocr_render.py index 5a1bf326..5a1d6624 100644 --- a/paperforge/worker/ocr_render.py +++ b/paperforge/worker/ocr_render.py @@ -880,9 +880,12 @@ def _render_reader_figure_card(figure: dict) -> list[str]: def _reader_figure_embed_target(figure: dict) -> str | None: status = str(figure.get("reader_status") or "") - figure_number = figure.get("figure_number") if status not in {"EXACT_MATCH", "SEQUENCE_MATCH"}: return None + figure_id = figure.get("figure_id", "") + if figure_id: + return figure_id + figure_number = figure.get("figure_number") if figure_number is None: return None return f"figure_{int(figure_number):03d}" diff --git a/tests/test_ocr_figures.py b/tests/test_ocr_figures.py index 7d263b81..a08c2580 100644 --- a/tests/test_ocr_figures.py +++ b/tests/test_ocr_figures.py @@ -5353,3 +5353,112 @@ def test_page_assets_count_uses_ordered_legends_not_stale_deduped() -> None: assert result2["decision"] == "rejected" assert "multiple_numbered_legends" in result2.get("evidence", []) + +class TestResolveFigureIdCollisions: + """_resolve_figure_id_collisions must never produce duplicate figure_ids.""" + + def _make_fig(self, figure_id: str, **kw: object) -> dict: + return {"figure_id": figure_id, "page": 1, **kw} + + def _run(self, figs: list[dict]) -> list[dict]: + from paperforge.worker.ocr_figures import _resolve_figure_id_collisions + inventory = {"matched_figures": list(figs), "held_figures": [], "ambiguous_figures": []} + _resolve_figure_id_collisions(inventory) + return inventory["matched_figures"] + + def _ids(self, figs: list[dict]) -> list[str]: + return [f["figure_id"] for f in figs] + + def test_no_collision(self) -> None: + figs = [self._make_fig("figure_001"), self._make_fig("figure_002"), self._make_fig("figure_003")] + result = self._run(figs) + assert self._ids(result) == ["figure_001", "figure_002", "figure_003"] + + def test_single_collision(self) -> None: + figs = [self._make_fig("figure_001"), self._make_fig("figure_001")] + result = self._run(figs) + assert self._ids(result) == ["figure_001", "figure_s001"] + + def test_triple_collision(self) -> None: + figs = [self._make_fig("figure_001"), self._make_fig("figure_001"), self._make_fig("figure_001")] + result = self._run(figs) + assert self._ids(result) == ["figure_001", "figure_s001", "figure_ss001"] + + def test_multiple_independent_collisions(self) -> None: + figs = [ + self._make_fig("figure_001"), + self._make_fig("figure_002"), + self._make_fig("figure_001"), + self._make_fig("figure_003"), + self._make_fig("figure_002"), + ] + result = self._run(figs) + assert self._ids(result) == ["figure_001", "figure_002", "figure_s001", "figure_003", "figure_s002"] + + def test_mixed_buckets_deduplicated(self) -> None: + from paperforge.worker.ocr_figures import _resolve_figure_id_collisions + + mf = [self._make_fig("figure_001")] + hf = [self._make_fig("figure_001")] + af = [self._make_fig("figure_001")] + inventory = {"matched_figures": mf, "held_figures": hf, "ambiguous_figures": af} + _resolve_figure_id_collisions(inventory) + assert inventory["matched_figures"][0]["figure_id"] == "figure_001" + assert inventory["held_figures"][0]["figure_id"] == "figure_s001" + assert inventory["ambiguous_figures"][0]["figure_id"] == "figure_ss001" + + def test_empty_figure_id(self) -> None: + figs = [self._make_fig(""), self._make_fig("")] + result = self._run(figs) + assert self._ids(result) == ["", ""] + + def test_no_figure_id_field(self) -> None: + figs: list[dict] = [{"page": 1}, {"page": 2}] + result = self._run(figs) + assert all(f.get("figure_id", "") == "" for f in result) + + def test_non_figure_prefixes_untouched(self) -> None: + figs = [ + self._make_fig("held_figure_001"), + self._make_fig("held_figure_002"), + self._make_fig("cluster_001"), + ] + result = self._run(figs) + assert self._ids(result) == ["held_figure_001", "held_figure_002", "cluster_001"] + + def test_held_figure_prefix_collision(self) -> None: + figs = [ + self._make_fig("held_figure_001"), + self._make_fig("held_figure_001"), + ] + result = self._run(figs) + assert self._ids(result) == ["held_figure_001", "figure_sheld_figure_001"] + + def test_realistic_body_plus_supplementary(self) -> None: + figs = [ + self._make_fig("figure_001", page=6, caption="Figure 1: Main body figure"), + self._make_fig("figure_002", page=9, caption="Figure 2: Another body figure"), + self._make_fig("figure_003", page=11, caption="Figure 3: Histology results"), + self._make_fig("figure_004", page=15, caption="Figure 4: Osteogenesis results"), + self._make_fig("figure_001", page=37, caption="Figure S.1: Supplementary confocal"), + self._make_fig("figure_002", page=38, caption="Figure S.2: Cell number data"), + self._make_fig("figure_003", page=39, caption="Figure S.3: Osteogenesis confocal"), + self._make_fig("figure_004", page=40, caption="Figure S.4: Cell number data"), + ] + result = self._run(figs) + assert self._ids(result) == [ + "figure_001", "figure_002", "figure_003", "figure_004", + "figure_s001", "figure_s002", "figure_s003", "figure_s004", + ] + # Verify originals are untouched + assert len({f["figure_id"] for f in result}) == 8 # all unique + + def test_promoted_sequence_matches_also_resolved(self) -> None: + # Sequence-promoted figures use _format_figure_id(ns, fn) which can collide + figs = [ + self._make_fig("figure_001", strict_status="sequence_match"), + self._make_fig("figure_001", strict_status="matched"), + ] + result = self._run(figs) + assert self._ids(result) == ["figure_001", "figure_s001"] +