fix(tables): flatten page-number tables of contents instead of gridding#142
fix(tables): flatten page-number tables of contents instead of gridding#142abimaelmartell wants to merge 5 commits into
Conversation
A title-based contents page ("About the Publisher vii", "Experiment #1
… 3") with no dot leaders and no section numbers was detected as a
2-column data table and rendered as a markdown grid, scrambling the
linear reading order (a top cause of NID loss on affected docs) and
scoring 0 on table structure.
Add is_page_number_toc: a narrow (2-3 col) list whose last column is
mostly page numbers (short integers or roman numerals) that are mostly
non-decreasing, with a text-title first column and NO header row (a
TOC's first row is already an entry). Such tables now route through the
existing flat-list TOC renderer.
The no-header + narrow-width + monotonic guards keep real data tables
intact — e.g. a 4-column regional table, or a 2-column "Mineral | CEC"
table with a header row and ascending values.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/tables/detect_heuristic.rs">
<violation number="1" location="src/tables/detect_heuristic.rs:930">
P3: Roman front-matter pages are detected as TOCs but not formatted as page numbers, so they lose the intended title/page separation. Extend `format::is_page_number_cell` with the same Roman-token recognition.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| if t.chars().all(|c| c.is_ascii_digit()) && t.len() <= 4 { | ||
| return t.parse().ok(); | ||
| } | ||
| let lower = t.to_ascii_lowercase(); |
There was a problem hiding this comment.
P3: Roman front-matter pages are detected as TOCs but not formatted as page numbers, so they lose the intended title/page separation. Extend format::is_page_number_cell with the same Roman-token recognition.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/tables/detect_heuristic.rs, line 930:
<comment>Roman front-matter pages are detected as TOCs but not formatted as page numbers, so they lose the intended title/page separation. Extend `format::is_page_number_cell` with the same Roman-token recognition.</comment>
<file context>
@@ -914,7 +914,109 @@ fn looks_like_number(s: &str) -> bool {
+ if t.chars().all(|c| c.is_ascii_digit()) && t.len() <= 4 {
+ return t.parse().ok();
+ }
+ let lower = t.to_ascii_lowercase();
+ if !lower.is_empty() && lower.len() <= 6 && lower.chars().all(|c| "ivxlc".contains(c)) {
+ let mut total = 0i32;
</file context>
Reflects the phantom-TOC fix in this PR: NID 0.88 -> 0.89 on the 200-doc benchmark. Other cells are unchanged at 2-decimal precision. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…n page cells
- page_number_value now requires a *canonical* roman numeral (re-encode
and compare), so words like "civil"/"mix"/"ill" are no longer parsed
as page numbers.
- The no-header guard checks the actual first row's last cell instead of
the first non-empty one, so a blank header cell ("Category | ") still
rejects the TOC heuristic.
- format::is_page_number_cell recognizes canonical roman numerals, so
roman front-matter pages (vii, ix) get proper title/page separation in
the flat TOC list.
- Fix the non-monotonic test to use 5 rows so it exercises the
monotonicity guard rather than the row-count early return; add
roman-lookalike and blank-header rejection tests.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
All four addressed:
Benchmark unchanged (overall 0.834, NID 0.888, TEDS 0.656, MHS 0.742) — the fixes tighten detection without altering corpus outcomes. fmt/clippy/616 tests green. |
|
@abimaelmartell I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 4 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
- Extract canonical_roman_value + to_roman_lower into tables/mod.rs and use them from both the TOC detector and the formatter, removing the duplicated mapping/loop and keeping them in sync. The shared helper accepts ≤8 chars, so longer front-matter numerals (xxxviii) flatten consistently on both sides. - Add a page-span guard to is_page_number_toc: real page numbers skip through the document (range >> entry count), so a dense consecutive ordinal/rank/ID column (1,2,3,…) is rejected — monotonicity alone did not separate those data tables from contents. Costs ~0.001 aggregate on the benchmark (NID 0.888->0.887) for the added precision; still a clear win over baseline (NID 0.883, TEDS unchanged). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
All three addressed:
Cost of the stronger signal is ~0.001 aggregate (overall 0.834→0.833, NID 0.888→0.887) — a precision gain worth it, and still a clear win over baseline main (NID 0.883, TEDS unchanged at 0.656). Doc 108 still flattens; 617 tests + clippy green. |
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
The strict page-span rule rejected legitimate one-page-per-entry TOCs (range ~= entry count). Relax it: accept any page sequence with a gap (nearly all real contents). Only a *perfectly dense* consecutive run — which rank/ID/ordinal columns produce, but a chapter-per-page TOC can too — falls back to a title signal: flatten when the first-column entries average multi-word headings, keep as a table when they are the short single-word labels typical of leaderboards/ID lists. Recovers the ~0.001 the range-only rule cost (NID back to 0.888) while still rejecting dense ordinal data tables. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Valid — fixed. The strict page-span rule did reject legitimate one-page-per-entry TOCs (range ≈ entry count), which is the ~0.001 it had cost. Reworked
This recovers the consecutive-page TOC case (NID back to 0.888, overall 0.834) while still rejecting dense ordinal data tables ( |
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/tables/detect_heuristic.rs">
<violation number="1" location="src/tables/detect_heuristic.rs:1017">
P1: Headerless rank/ID tables with a skipped or repeated value now flatten even when labels are ordinary multi-word names. `!dense_consecutive` treats those common data-table sequences as contents without applying the new title signal; retain a TOC-specific guard on this path instead of accepting every imperfect sequence.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
| sorted.dedup(); | ||
| sorted.len() == page_vals.len() | ||
| }; | ||
| if !dense_consecutive { |
There was a problem hiding this comment.
P1: Headerless rank/ID tables with a skipped or repeated value now flatten even when labels are ordinary multi-word names. !dense_consecutive treats those common data-table sequences as contents without applying the new title signal; retain a TOC-specific guard on this path instead of accepting every imperfect sequence.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/tables/detect_heuristic.rs, line 1017:
<comment>Headerless rank/ID tables with a skipped or repeated value now flatten even when labels are ordinary multi-word names. `!dense_consecutive` treats those common data-table sequences as contents without applying the new title signal; retain a TOC-specific guard on this path instead of accepting every imperfect sequence.</comment>
<file context>
@@ -996,14 +996,44 @@ pub(super) fn is_page_number_toc(cells: &[Vec<String>]) -> bool {
+ sorted.dedup();
+ sorted.len() == page_vals.len()
+ };
+ if !dense_consecutive {
+ // Narrow range but with a gap or repeat — still contents-like.
+ return true;
</file context>
Summary
A title-based table-of-contents page —
About the Publisher vii,Experiment #1: Hydrostatic Pressure 3— with no dot leaders and no section numbers was being detected as a 2-column data table and rendered as a markdown grid. That scrambles the page's linear reading order (a leading cause of reading-order/NID loss on those docs) and scores zero on table structure.The codebase already renders detected TOCs as a flat per-row list (
TableKind::Toc→format_toc_as_list), butis_table_of_contentsonly recognized dot-leader and section-numbered TOCs. This adds the missing page-number-column case.is_page_number_tocmatches a narrow (2–3 column) list where:The narrow-width, no-header, and monotonic guards keep real tables intact — verified on a 4-column regional data table and a 2-column
Mineral | CECtable (header row + ascending values) that both correctly stay gridded.Impact (opendataloader-bench, 200 docs)
NID (reading order) and MHS improve with no TEDS regression — the change removes phantom-table scrambling without touching real tables.
Testing
Mineral | CECtable, and a 4-column regional grid all correctly rejected.cargo fmt/cargo clippy -- -D warnings/cargo testpass.🤖 Generated with Claude Code
Summary by cubic
Fixes misdetected page-number-only tables of contents by flattening them into lists instead of grids, restoring reading order. Benchmarks/docs updated: NID 0.89 (rounded); TEDS unchanged.
is_page_number_tocand wired it intois_table_of_contents.canonical_roman_value/to_roman_lowerintables::modand use them in both detector andformat::is_page_number_cell; formatter now recognizes canonical roman numerals for clean title/page splits.Mineral | CECwith header).Written for commit 5a2bad7. Summary will update on new commits.