From 2d3d275bf9ca47d3433490d50b81db6cacb4ec0f Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Tue, 1 Sep 2026 18:06:42 +0000 Subject: [PATCH] =?UTF-8?q?fix(tables):=20finish=20GH-352=20leftovers=20?= =?UTF-8?q?=E2=80=94=20shared=20column=20helper=20and=20scoreboard?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Collapse _project_candidate_data_columns and _candidate_data_column_indices onto _candidate_data_column_indices as the single source of truth so the projected grid and bind() remap cannot drift apart. - Record model_unbound_nonempty on CoverageRecord and surface it on the whole-page #330 scoreboard summary (region-scoped already had it). PR #430 already landed the column_binding_unverifiable walk gate; left intact. Co-authored-by: Rubén Fernández-Fuertes --- src/socr/benchmark/binding_coverage.py | 5 +- src/socr/tables/binding.py | 40 +--- ...h330_binding_coverage_stage_fallthrough.py | 1 + tests/test_gh352_leftovers.py | 214 ++++++++++++++++++ 4 files changed, 231 insertions(+), 29 deletions(-) create mode 100644 tests/test_gh352_leftovers.py diff --git a/src/socr/benchmark/binding_coverage.py b/src/socr/benchmark/binding_coverage.py index 8906017..725ed44 100644 --- a/src/socr/benchmark/binding_coverage.py +++ b/src/socr/benchmark/binding_coverage.py @@ -74,6 +74,7 @@ class CoverageRecord: native_valueless_unbound: int native_unbound_count: int model_unbound_count: int + model_unbound_nonempty: bool cell_contradiction_count: int = 0 row_label_contradiction_count: int = 0 contradiction_count: int = 0 @@ -335,6 +336,7 @@ def _record( contradiction_count=len(result.contradicted_cells) + len(result.row_label_contradictions), native_unbound_count=len(result.native_unbound), model_unbound_count=len(result.model_unbound), + model_unbound_nonempty=bool(result.model_unbound), cell_contradiction_count=len(result.contradicted_cells), row_label_contradiction_count=len(result.row_label_contradictions), ) @@ -365,7 +367,7 @@ def _aggregate(records: tuple[CoverageRecord, ...], denominator: int) -> dict[st ), "contradiction_nonempty": sum(record.contradiction_count > 0 for record in records), "native_unbound_nonempty": sum(record.native_unbound_count > 0 for record in records), - "model_unbound_nonempty": sum(record.model_unbound_count > 0 for record in records), + "model_unbound_nonempty": sum(record.model_unbound_nonempty for record in records), } @@ -509,5 +511,6 @@ def summary_text(report: CoverageReport) -> str: "Selected whole-page results:", f" fully checked: {whole['fully_checked']}/{whole['denominator']}", f" structural agreement: {whole['structural_agreement']}/{whole['denominator']}", + f" model unbound non-empty: {whole['model_unbound_nonempty']}/{whole['denominator']}", ] return "\n".join(lines) + "\n" diff --git a/src/socr/tables/binding.py b/src/socr/tables/binding.py index 1b83213..ed70cef 100644 --- a/src/socr/tables/binding.py +++ b/src/socr/tables/binding.py @@ -211,8 +211,8 @@ def parse_grid(markdown: str) -> Grid | None: return None -def _project_candidate_data_columns(grid: Grid) -> Grid: - """Keep candidate columns that contain a genuine numeric data token. +def _candidate_data_column_indices(grid: Grid) -> tuple[int, ...]: + """Return original indexes for candidate columns with genuine numeric data. The rowizer emits a fixed lane grid before ``_clean_grid`` sees it. A lane can consequently remain in the markdown when its header is populated @@ -220,6 +220,9 @@ def _project_candidate_data_columns(grid: Grid) -> Grid: the same header-only shape. Such a column has no numeric binding claim. Projecting it out keeps the binder's column space aligned with the candidate's numeric data space without selecting a native lane by value. + + ``_project_candidate_data_columns`` and ``bind()``'s column remap both + consume this tuple so the projected grid and index mapping cannot drift. """ numeric_columns_by_row: list[set[int]] = [] for column in range(1, grid.n_cols): @@ -236,11 +239,17 @@ def _project_candidate_data_columns(grid: Grid) -> Grid: numeric_columns_by_row[row_number].add(column) if not numeric_columns_by_row: - return grid + return tuple() widest_rows = max(len(columns) for columns in numeric_columns_by_row) data_columns = sorted( set().union(*(columns for columns in numeric_columns_by_row if len(columns) == widest_rows)) ) + return tuple(data_columns) + + +def _project_candidate_data_columns(grid: Grid) -> Grid: + """Project *grid* down to stub plus the numeric data columns from the helper.""" + data_columns = _candidate_data_column_indices(grid) if not data_columns: return grid keep = (0, *data_columns) @@ -250,31 +259,6 @@ def _project_candidate_data_columns(grid: Grid) -> Grid: ) -def _candidate_data_column_indices(grid: Grid) -> tuple[int, ...]: - """Return original indexes for the projected candidate data columns.""" - numeric_columns_by_row: list[set[int]] = [] - for row in grid.rows: - columns = set() - for column, cell in enumerate(row[1:], start=1): - if any( - is_numeric_token(token) and not _SPEC_NUMBER_RE.match(strip_presentation(token)) - for token in re.split(r"\s+", cell.strip()) - if token - ): - columns.add(column) - numeric_columns_by_row.append(columns) - if not numeric_columns_by_row: - return tuple() - widest_rows = max(len(columns) for columns in numeric_columns_by_row) - return tuple( - sorted( - set().union( - *(columns for columns in numeric_columns_by_row if len(columns) == widest_rows) - ) - ) - ) - - # -------------------------------------------------------------------------- # Native geometry — row bands, lanes, row paths # -------------------------------------------------------------------------- diff --git a/tests/test_gh330_binding_coverage_stage_fallthrough.py b/tests/test_gh330_binding_coverage_stage_fallthrough.py index 9eb468e..b6d95ca 100644 --- a/tests/test_gh330_binding_coverage_stage_fallthrough.py +++ b/tests/test_gh330_binding_coverage_stage_fallthrough.py @@ -241,6 +241,7 @@ def test_coverage_record_fields_and_types(): contradiction_count=0, native_unbound_count=0, model_unbound_count=0, + model_unbound_nonempty=False, ) assert record.row_labels_checked == 5 assert record.candidate_valueless_unbound == 0 diff --git a/tests/test_gh352_leftovers.py b/tests/test_gh352_leftovers.py new file mode 100644 index 0000000..3a07115 --- /dev/null +++ b/tests/test_gh352_leftovers.py @@ -0,0 +1,214 @@ +"""GH-352 leftovers: one column-projection helper and scoreboard model_unbound.""" + +from __future__ import annotations + +from socr.benchmark.binding_coverage import ( + CoverageRecord, + CoverageReport, + _aggregate, + summary_text, +) +from socr.tables.binding import ( + Grid, + _candidate_data_column_indices, + _project_candidate_data_columns, + bind, + parse_grid, +) + + +def _w(x0, y0, x1, y1, text): + return (x0, y0, x1, y1, text, 0, 0, 0) + + +def _two_lane_page(): + return [ + _w(50, 100, 90, 110, "Coef"), + _w(150, 100, 180, 110, "1.10"), + _w(250, 100, 280, 110, "1.11"), + _w(50, 130, 90, 140, "SE"), + _w(150, 130, 180, 140, "0.05"), + _w(250, 130, 280, 140, "0.06"), + ] + + +_WIDER_THAN_THE_PAGE = "\n".join( + [ + "| | OLS | note | IV | note2 |", + "| --- | --- | --- | --- | --- |", + "| Coef | 1.10 | | 1.11 | |", + "| SE | 0.05 | | 0.06 | |", + ] +) + + +class TestCandidateDataColumnHelperIsSingleSourceOfTruth: + def test_projected_grid_columns_match_index_helper(self) -> None: + grid = parse_grid(_WIDER_THAN_THE_PAGE) + assert grid is not None + + indices = _candidate_data_column_indices(grid) + projected = _project_candidate_data_columns(grid) + + assert indices == (1, 3) + assert projected.n_cols - 1 == len(indices) + for projected_col, original_col in enumerate(indices, start=1): + for row_idx, row in enumerate(grid.rows): + projected_row = projected.rows[row_idx] + if original_col < len(row): + assert projected_row[projected_col] == row[original_col] + + def test_widest_row_union_keeps_columns_from_the_widest_numeric_rows(self) -> None: + """A narrower row must not pull columns into the remap that projection drops.""" + markdown = "\n".join( + [ + "| | A | B | C |", + "| --- | --- | --- | --- |", + "| wide | 1.0 | 2.0 | 3.0 |", + "| narrow | 4.0 | | |", + ] + ) + grid = parse_grid(markdown) + assert grid is not None + + assert _candidate_data_column_indices(grid) == (1, 2, 3) + assert _project_candidate_data_columns(grid).n_cols == 4 + + def test_bind_remap_uses_the_same_indices_as_projection(self) -> None: + """Regression: remap drift would surface invented model_unbound on self-bind.""" + result = bind(_two_lane_page(), _WIDER_THAN_THE_PAGE) + + assert result.column_binding_unverifiable is True + assert {(cell.row_path[-1], cell.token) for cell in result.model_unbound} == set() + + +class TestModelUnboundNonemptyOnScoreboard: + def test_coverage_record_carries_model_unbound_nonempty(self) -> None: + record = CoverageRecord( + paper="test_paper", + page=1, + source_stage="find_tables_lines", + region_ordinal=1, + selected_primary=True, + grid_rows=2, + grid_columns=3, + fully_checked=False, + structural_agreement=False, + row_binding_unverifiable=True, + column_binding_unverifiable=True, + row_label_unverifiable=False, + row_labels_checked=2, + ambiguous_count=0, + candidate_valueless_unbound=0, + native_valueless_unbound=0, + contradiction_count=2, + native_unbound_count=0, + model_unbound_count=2, + model_unbound_nonempty=True, + ) + + assert record.model_unbound_nonempty is True + assert record.model_unbound_count == 2 + + def test_aggregate_counts_model_unbound_nonempty_from_record_flag(self) -> None: + empty = CoverageRecord( + paper="a", + page=1, + source_stage="find_tables_lines", + region_ordinal=1, + selected_primary=True, + grid_rows=1, + grid_columns=2, + fully_checked=False, + structural_agreement=False, + row_binding_unverifiable=True, + column_binding_unverifiable=True, + row_label_unverifiable=False, + row_labels_checked=0, + ambiguous_count=0, + candidate_valueless_unbound=0, + native_valueless_unbound=0, + contradiction_count=0, + native_unbound_count=0, + model_unbound_count=0, + model_unbound_nonempty=False, + ) + nonempty = CoverageRecord( + paper="b", + page=1, + source_stage="find_tables_lines", + region_ordinal=1, + selected_primary=True, + grid_rows=1, + grid_columns=2, + fully_checked=False, + structural_agreement=False, + row_binding_unverifiable=True, + column_binding_unverifiable=True, + row_label_unverifiable=False, + row_labels_checked=0, + ambiguous_count=0, + candidate_valueless_unbound=0, + native_valueless_unbound=0, + contradiction_count=0, + native_unbound_count=0, + model_unbound_count=3, + model_unbound_nonempty=True, + ) + + totals = _aggregate((empty, nonempty), denominator=2) + + assert totals["model_unbound_nonempty"] == 1 + + def test_whole_page_summary_includes_model_unbound_nonempty(self) -> None: + report = CoverageReport( + summary={ + "total_pages": 15, + "bindable_pages": 13, + "strict_grids": 13, + "placeholder_regions": 0, + "no_grid_pages": 2, + "bindable_pages_by_stage": {"find_tables_lines": 13}, + "region_scoped": { + "denominator": 13, + "fully_checked": 0, + "structural_agreement": 0, + "row_binding_unverifiable": 13, + "column_binding_unverifiable": 1, + "row_label_unverifiable": 0, + "row_labels_checked_positive": 0, + "ambiguity_nonempty": 0, + "candidate_valueless_unbound": 0, + "native_valueless_unbound": 0, + "cell_contradiction_nonempty": 0, + "row_label_contradiction_nonempty": 0, + "contradiction_nonempty": 0, + "native_unbound_nonempty": 0, + "model_unbound_nonempty": 4, + }, + "whole_page": { + "denominator": 15, + "fully_checked": 0, + "structural_agreement": 0, + "row_binding_unverifiable": 15, + "column_binding_unverifiable": 15, + "row_label_unverifiable": 0, + "row_labels_checked_positive": 0, + "ambiguity_nonempty": 0, + "candidate_valueless_unbound": 0, + "native_valueless_unbound": 0, + "cell_contradiction_nonempty": 0, + "row_label_contradiction_nonempty": 0, + "contradiction_nonempty": 0, + "native_unbound_nonempty": 0, + "model_unbound_nonempty": 6, + }, + }, + regions=(), + pages=(), + ) + + text = summary_text(report) + + assert "model unbound non-empty: 6/15" in text + assert "model unbound non-empty: 4" in text