bench: measure the hybrid ceiling — 0.362 to 0.935 with correct boundaries - #23
Conversation
…aries Answers the question that decides whether a layout model is worth deploying: given perfect rows and columns, how good is extraction? pdftable, current best (lines) precision 0.865 recall 0.229 F1 0.362 pdftable + ORACLE boundaries precision 0.948 recall 0.922 F1 0.935 Given the right grid, pdftable extracts almost perfectly. So essentially the entire end-to-end gap is table STRUCTURE, not text extraction -- the cell filling, the coordinates and the text fidelity are not the limiting factor. A layout model that outputs row/column structure converts almost the whole gap, and pdftable keeps the two things a generative model cannot give: exact cell text and exact coordinates for citations. It also produces a concrete integration rule. MergeSplitTokens drops the oracle result from 0.935 to 0.726, because that setting exists to repair columns a geometric guess cut through a value; with a correct grid there is nothing to repair and every merge destroys a correct cell. Explicit boundaries from a model therefore imply MergeSplitTokens=false. The first version of this experiment reported 0.119 with PERFECT input -- near-random, and plainly measuring itself. Two causes, the same mistake twice: every cell bounding-box edge was treated as a grid line, which shredded each table into fragments (fixed by deriving the grid from the ground truth start-col/end-col indices, giving exactly ncols+1 lines); and ground truth was counted for pages the oracle never attempted, which reported a deliberate exclusion as an extraction failure. 0.119 -> 0.782 -> 0.935. A third suspicion was unfounded: the ground-truth Y origin was checked against pdfplumber word positions and is bottom-left, the same space pdftable reports.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
📝 WalkthroughWalkthroughAdded an ICDAR 2013 oracle-boundaries benchmark. The harness derives grid boundaries from XML, runs ChangesICDAR 2013 oracle-boundary evaluation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Benchmark as oracle.py
participant Dataset as ICDAR 2013 XML/PDF
participant Extractor as pdftable
participant Scorer as scoring helpers
Benchmark->>Dataset: discover PDF/XML pairs
Benchmark->>Dataset: derive oracle row and column boundaries
Benchmark->>Extractor: run extraction with oracle boundaries
Extractor-->>Benchmark: extracted cell relations or failure
Benchmark->>Scorer: compare extracted relations with ground truth
Scorer-->>Benchmark: precision, recall, and F1
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Reviewer's GuideAdds an ICDAR 2013 benchmark harness that feeds pdftable ground-truth table boundaries to measure the hybrid ceiling with a layout model, documents the experiment and its integration rules, and wires it into the bench/docs index. Sequence diagram for the new oracle benchmark harness interaction per documentsequenceDiagram
participant main as main
participant extractor as extractor_exe
participant scoring as score_module
main->>main: oracle_edges(xml_path)
main->>main: gt_relations_single_region(xml_path)
loop for each variant in variants
main->>extractor: subprocess.run([exe, "-oracle", path] + extra + [pdf])
extractor-->>main: tables JSON
main->>scoring: relations_from_grid(t["rows"]) per table
scoring-->>main: rels Counter
main->>scoring: score(gt, rels)
scoring-->>main: c, nd, ng
main->>main: accumulate totals[name]
end
main->>scoring: prf(*totals[name]) for each variant
scoring-->>main: precision, recall, F1
main->>main: print summary table and documents scored
Flow diagram for the oracle harness benchmarking pipelineflowchart LR
A["Start main()"] --> B["Collect (pdf, xml) pairs under dataset root"]
B --> C["Apply limit if provided"]
C --> D["For each (pdf, xml) pair"]
D --> E["Compute edges = oracle_edges(xml)"]
E --> F{"edges exists?"}
F -->|No| D
F -->|Yes| G["gt = gt_relations_single_region(xml)"]
G --> H["Write edges JSON to temp file"]
H --> I["For each variant in variants"]
I --> J["Run extractor_exe with -oracle and variant flags"]
J --> K["Parse tables JSON"]
K --> L["Build rels via relations_from_grid"]
L --> M["Update totals using score(gt, rels)"]
M --> I
I --> N["Delete temp file"]
N --> D
D --> O["Compute precision, recall, F1 via prf"]
O --> P["Print metrics and scored document count"]
P --> Q["End"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
bench/icdar2013/oracle.py (2)
64-64: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueOptional: consider
defusedxmlfor parsing dataset XML.Both
ET.parsecalls (Line 64, Line 121) parse XML files from the benchmark dataset directory. Static analysis flags standardxml.etreeas vulnerable to entity-expansion attacks on untrusted input (S314). Since this is dev-only benchmark tooling parsing a known, locally-downloaded dataset rather than externally-supplied network input, the practical exposure is low, butdefusedxml.ElementTree.parseis a drop-in replacement if you want to close the gap.Also applies to: 121-121
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bench/icdar2013/oracle.py` at line 64, Replace both XML parsing calls in the benchmark flow with defusedxml.ElementTree.parse, updating the import accordingly while preserving the existing getroot and downstream parsing behavior.Source: Linters/SAST tools
55-55: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: address the ruff RUF007/RUF005 hints.
Line 55 uses
zip(idx, idx[1:]);itertools.pairwise(idx)is the idiomatic equivalent for Python's target version and reads more clearly. Line 186 concatenates lists with+;[exe, "-oracle", path, *extra, pdf]is the more idiomatic form ruff suggests. Neither changes behavior.Also applies to: 186-186
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bench/icdar2013/oracle.py` at line 55, Update the loop using zip(idx, idx[1:]) to use itertools.pairwise(idx), adding the required import, and replace the list concatenation at the command construction near the second occurrence with list unpacking ([exe, "-oracle", path, *extra, pdf]); preserve existing behavior.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bench/icdar2013/oracle.py`:
- Around line 49-60: Update _boundaries to handle indices present in only one of
lo or hi without raising KeyError, including asymmetric column and row spans.
For each gap, use the available extent when an edge is missing while preserving
the midpoint calculation when both extents exist, and retain the existing
empty-input and sorted-boundary behavior.
- Around line 183-198: Update the subprocess execution loop over variants to
check the CompletedProcess returncode and surface any subprocess or JSON parsing
failures instead of silently assigning tables = []. Log or print the variant and
relevant error details, while preserving successful output parsing and ensuring
failures are visible in the final benchmark summary rather than included as
empty results.
---
Nitpick comments:
In `@bench/icdar2013/oracle.py`:
- Line 64: Replace both XML parsing calls in the benchmark flow with
defusedxml.ElementTree.parse, updating the import accordingly while preserving
the existing getroot and downstream parsing behavior.
- Line 55: Update the loop using zip(idx, idx[1:]) to use
itertools.pairwise(idx), adding the required import, and replace the list
concatenation at the command construction near the second occurrence with list
unpacking ([exe, "-oracle", path, *extra, pdf]); preserve existing behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3bb9117e-71a2-4545-b8ce-36772c568030
📒 Files selected for processing (4)
bench/README.mdbench/icdar2013/oracle.pydocs/README.mddocs/evaluations/2026-08-03-hybrid-ceiling-oracle-boundaries.md
| def _boundaries(lo: dict[int, float], hi: dict[int, float]) -> list[float]: | ||
| """Grid lines from per-index extents: outer edges plus gap midpoints.""" | ||
| idx = sorted(set(lo) | set(hi)) | ||
| if len(idx) < 1: | ||
| return [] | ||
| out = [lo[idx[0]]] | ||
| for a, b in zip(idx, idx[1:]): | ||
| # Midpoint of the gutter between two adjacent bands. Using either | ||
| # edge alone would clip whichever side is wider on some rows. | ||
| out.append((hi[a] + lo[b]) / 2) | ||
| out.append(hi[idx[-1]]) | ||
| return sorted(out) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Reproduce the KeyError in _boundaries with a spanning-cell fixture.
python3 - <<'EOF'
def _boundaries(lo, hi):
idx = sorted(set(lo) | set(hi))
if len(idx) < 1:
return []
out = [lo[idx[0]]]
for a, b in zip(idx, idx[1:]):
out.append((hi[a] + lo[b]) / 2)
out.append(hi[idx[-1]])
return sorted(out)
col_lo = {0: 0.0, 1: 10.0}
col_hi = {0: 5.0, 3: 40.0}
print(_boundaries(col_lo, col_hi))
EOFRepository: hallelx2/pdftable
Length of output: 282
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
git ls-files | rg '(^|/)oracle\.py$|bench/icdar2013' || true
echo
echo "Relevant file outline:"
ast-grep outline bench/icdar2013/oracle.py --view expanded || true
echo
echo "Relevant lines 1-120:"
sed -n '1,120p' bench/icdar2013/oracle.py | nl -ba
echo
echo "Relevant lines 140-180:"
sed -n '140,180p' bench/icdar2013/oracle.py | nl -ba -v 140Repository: hallelx2/pdftable
Length of output: 670
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' 'bench/icdar2013/oracle.py:1-120'
awk 'NR>=1 && NR<=120 { printf "%6d %s\n", NR, $0 }' bench/icdar2013/oracle.py
printf '\n%s\n' 'bench/icdar2013/oracle.py:120-190'
awk 'NR>=120 && NR<=190 { printf "%6d %s\n", NR, $0 }' bench/icdar2013/oracle.py
printf '\n%s\n' 'OracleEdges caller context / imports'
rg -n "def _boundaries|def oracle_edges|oracle_edges\\(|_boundaries\\(|from itertools|import itertools|try:|except Exception|returncode|tables = \\[\\]" bench/icdar2013/oracle.pyRepository: hallelx2/pdftable
Length of output: 9871
Fix the KeyError risk in _boundaries for asymmetric column/row spans.
_boundaries builds idx from lo | hi, then reads both hi[a] and lo[b] for every gap. When a spanning cell starts at column c but no cell ends in column c, hi[c] is missing while c is still in idx, and the loop raises KeyError. The same row-span scenario can fail inside _boundaries(row_lo, row_hi).
Handle missing band extents consistently, for example by using the available edge from lo/hi when one side has no explicit start/end evidence.
🧰 Tools
🪛 Ruff (0.16.0)
[warning] 55-55: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
[warning] 55-55: Prefer itertools.pairwise() over zip() when iterating over successive pairs
Replace zip() with itertools.pairwise()
(RUF007)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bench/icdar2013/oracle.py` around lines 49 - 60, Update _boundaries to handle
indices present in only one of lo or hi without raising KeyError, including
asymmetric column and row spans. For each gap, use the available extent when an
edge is missing while preserving the midpoint calculation when both extents
exist, and retain the existing empty-input and sorted-boundary behavior.
| for name, extra in variants.items(): | ||
| try: | ||
| out = subprocess.run( | ||
| [exe, "-oracle", path] + extra + [pdf], | ||
| capture_output=True, timeout=180).stdout | ||
| tables = json.loads(out or b"[]") | ||
| except Exception: | ||
| tables = [] | ||
| rels: Counter = Counter() | ||
| for t in tables: | ||
| rels += relations_from_grid( | ||
| [[norm(c) for c in r] for r in t["rows"]]) | ||
| c, nd, ng = score(gt, rels) | ||
| totals[name][0] += c | ||
| totals[name][1] += nd | ||
| totals[name][2] += ng |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Surface subprocess failures instead of silently zeroing them out.
Line 189 catches every exception from subprocess.run/json.loads and falls back to tables = [], with no logging. The subprocess result's returncode is also never checked, so a non-zero exit (crash, bad -oracle argument, missing exe) that still writes something to stdout can silently degrade a variant's recall instead of surfacing a tool error. The file's own docstring documents two prior instances of "the harness measuring itself" (edge-clustering and page-scoring bugs); this blanket fallback reintroduces the same class of silent-failure risk for future runs or dataset changes.
Track and print failures so a systemic issue (wrong exe path, crashed binary, flag typo) is visible in the summary rather than blended into the F1 numbers.
🩹 Proposed fix: log failures and check the return code
try:
- out = subprocess.run(
- [exe, "-oracle", path] + extra + [pdf],
- capture_output=True, timeout=180).stdout
- tables = json.loads(out or b"[]")
- except Exception:
+ result = subprocess.run(
+ [exe, "-oracle", path, *extra, pdf],
+ capture_output=True, timeout=180)
+ if result.returncode != 0:
+ print(f" [warn] {name} exited {result.returncode} on "
+ f"{pdf}: {result.stderr.decode(errors='replace')[:200]}",
+ file=sys.stderr)
+ tables = json.loads(result.stdout or b"[]")
+ except Exception as exc:
+ print(f" [warn] {name} failed on {pdf}: {exc}", file=sys.stderr)
tables = []📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for name, extra in variants.items(): | |
| try: | |
| out = subprocess.run( | |
| [exe, "-oracle", path] + extra + [pdf], | |
| capture_output=True, timeout=180).stdout | |
| tables = json.loads(out or b"[]") | |
| except Exception: | |
| tables = [] | |
| rels: Counter = Counter() | |
| for t in tables: | |
| rels += relations_from_grid( | |
| [[norm(c) for c in r] for r in t["rows"]]) | |
| c, nd, ng = score(gt, rels) | |
| totals[name][0] += c | |
| totals[name][1] += nd | |
| totals[name][2] += ng | |
| for name, extra in variants.items(): | |
| try: | |
| result = subprocess.run( | |
| [exe, "-oracle", path, *extra, pdf], | |
| capture_output=True, timeout=180) | |
| if result.returncode != 0: | |
| print(f" [warn] {name} exited {result.returncode} on " | |
| f"{pdf}: {result.stderr.decode(errors='replace')[:200]}", | |
| file=sys.stderr) | |
| tables = json.loads(result.stdout or b"[]") | |
| except Exception as exc: | |
| print(f" [warn] {name} failed on {pdf}: {exc}", file=sys.stderr) | |
| tables = [] | |
| rels: Counter = Counter() | |
| for t in tables: | |
| rels += relations_from_grid( | |
| [[norm(c) for c in r] for r in t["rows"]]) | |
| c, nd, ng = score(gt, rels) | |
| totals[name][0] += c | |
| totals[name][1] += nd | |
| totals[name][2] += ng |
🧰 Tools
🪛 ast-grep (0.45.0)
[error] 184-186: Use of unsanitized data to create processes
Context: subprocess.run(
[exe, "-oracle", path] + extra + [pdf],
capture_output=True, timeout=180)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(os-system-unsanitized-data)
[error] 184-186: Command coming from incoming request
Context: subprocess.run(
[exe, "-oracle", path] + extra + [pdf],
capture_output=True, timeout=180)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
🪛 Ruff (0.16.0)
[error] 185-185: subprocess call: check for execution of untrusted input
(S603)
[warning] 186-186: Consider [exe, "-oracle", path, *extra, pdf] instead of concatenation
Replace with [exe, "-oracle", path, *extra, pdf]
(RUF005)
[warning] 189-189: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bench/icdar2013/oracle.py` around lines 183 - 198, Update the subprocess
execution loop over variants to check the CompletedProcess returncode and
surface any subprocess or JSON parsing failures instead of silently assigning
tables = []. Log or print the variant and relevant error details, while
preserving successful output parsing and ensuring failures are visible in the
final benchmark summary rather than included as empty results.
1 participant
Answers the question that decides whether a layout model is worth deploying: given perfect rows and columns, how good is extraction?
lines)MergeSplitTokens107 documents (single-region pages).
The verdict
0.362 → 0.935. Given the right grid, pdftable extracts almost perfectly.
Essentially the whole end-to-end gap is table structure, not text extraction. Every cell-filling, coordinate and font-metric fix of the last two days is not the limiting factor — finding rows and columns is.
So a layout model that outputs row/column structure converts nearly the entire gap, and pdftable keeps the two things a generative model cannot give: exact cell text and exact coordinates for citations.
A concrete integration rule falls out
MergeSplitTokenstakes the oracle result from 0.935 down to 0.726. That setting exists to repair columns a geometric guess cut through a value; with a correct grid there is nothing to repair, so every merge destroys a correct cell.Explicit boundaries from a model ⇒
MergeSplitTokens = false. It stays useful on the pure-geometry path.Integration point already exists
No new dependency, no HTTP client in the library. The caller owns the model call.
Two harness bugs — worth reading
The first version reported 0.119 F1 with perfect input. Near-random, and obviously measuring itself. Two causes, the same mistake twice:
start-col/end-colindices — exactlyncols+1lines.0.119 → 0.782 → 0.935.A third suspicion was unfounded: I checked the ground-truth Y origin against pdfplumber word positions and it is bottom-left, same as pdftable (GT
y1=619.0vs wordy0=616.9).Same lesson as the font work: a measurement that disagrees violently with expectation is far more likely to be a broken measurement than a broken system.
Scope
Single-region pages only (107 of 125). Multi-table pages are excluded because merging their regions produces a grid spanning the gap between them — the harness measuring itself again. A real layout model emits one region per table, so this is a fair proxy, but it is not a measurement of multi-table pages.
Relates to HAL-568
Summary by Sourcery
Add a benchmarking harness and evaluation to measure pdftable’s performance when given oracle row/column boundaries, establishing the hybrid layout-model ceiling and documenting integration guidance and harness fixes.
New Features:
Enhancements:
Documentation:
Summary by CodeRabbit
New Features
Documentation