docs: add bench/ and docs/evaluations/, with the ICDAR 2013 result - #20
Conversation
Benchmark harnesses and measurement findings had no home. The ICDAR 2013 harness was living in a temp directory and every result from this work existed only in the issue tracker, where it records that something was decided rather than what was measured. bench/ is a SEPARATE Go module. Nested modules are excluded from the parent ./... , so the library build ignores it and a consumer of pdftable never fetches its dependencies. The first version of this commit did not do that and go list ./... showed bench/icdar2013 inside the library module -- exactly the leak the split exists to prevent. Datasets are not committed. run.py fetches ICDAR 2013 into a cache directory, builds the extractor as its own module against the working tree, and scores pdftable alongside pdfplumber -- "is 0.36 good?" is unanswerable without a baseline. docs/evaluations/ holds dated reports. Each states the commit it ran against, the external oracle used, the metric precisely, what the number is NOT comparable to, and what remains untested. That last part is the point: a benchmark number without its scope reads as a general claim, and 0.36 here is end-to-end detection plus structure, not the structure-only task papers report 0.85-0.95 on. Reports are append-only, so a regression shows up as a diff between two dated files rather than disappearing into a rewrite. The library source stays in the repository root. That is the import path for a single-package Go library, and moving it would break every consumer; Go also deliberately keeps _test.go files beside the code they cover. The README now documents the layout so the root is legible rather than merely conventional.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
📝 WalkthroughWalkthroughThe change adds an isolated ICDAR 2013 benchmark module with extraction, scoring, diagnostics, and dataset setup. It also adds benchmark documentation, evaluation reports, roadmap updates, and repository layout documentation. ChangesICDAR 2013 Benchmark
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Runner as run.py
participant Dataset as ICDAR 2013 dataset
participant Extractor as extract.go
participant Scorer as score.py
Runner->>Dataset: download or reuse dataset
Runner->>Extractor: build isolated extractor
Runner->>Scorer: run benchmark
Scorer->>Extractor: extract tables from PDFs
Extractor-->>Scorer: return JSON table rows
Scorer-->>Runner: report precision, recall, and F1
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 a new benchmarks module and documented evaluation reports for ICDAR 2013 table-structure accuracy and font/table fidelity, including a Python+Go harness that scores pdftable and pdfplumber on the ICDAR 2013 dataset, and updates the README to describe the repository layout and link to these evaluations. Sequence diagram for the ICDAR 2013 benchmark harnesssequenceDiagram
actor User
participant run_py as run_py
participant extract_go as extract_go_exe
participant score_py as score_py
participant pdfplumber as pdfplumber
participant Dataset as icdar2013_dataset
User->>run_py: main()
run_py->>run_py: scratch()
run_py->>Dataset: fetch(dest)
run_py->>extract_go: build_extractor(dest)
run_py->>score_py: subprocess.run(score_py, root, exe)
score_py->>Dataset: gt_relations(xml_path)
score_py->>extract_go: run_pdftable(exe, pdf, strategy, merge)
score_py->>pdfplumber: run_pdfplumber(pdf, strategy)
score_py->>User: print precision/recall/F1 table
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: 12
🤖 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/diag.py`:
- Around line 59-63: Update the reporting block in the diagnostic script to
handle an empty `pairs` collection before calculating the zero-detection
percentage. Report a clear “no documents found” message when no pairs exist, and
preserve the existing percentage output for non-empty collections.
- Around line 43-57: Update the subprocess and JSON parsing flow in the
diagnostic loop to catch timeout, execution, and malformed-output errors,
matching the fallback behavior of run_pdftable in score.py. Treat any failed or
invalid PDF result as empty tables, then continue scoring and appending results
for the remaining documents.
In `@bench/icdar2013/README.md`:
- Line 8: Update the ICDAR2013 benchmark installation instruction to pin
pdfplumber to version 0.11.9, ensuring future runs use the same dependency
version as the reference oracle.
In `@bench/icdar2013/run.py`:
- Line 55: Configure a module-level network timeout before the dataset download
in run.py by importing socket and setting socket.setdefaulttimeout(60). Keep the
existing urllib.request.urlretrieve(URL, tgz) call unchanged while ensuring
stalled downloads fail after the specified timeout.
- Around line 57-58: Update the tarfile extraction in the download/archive
handling block to pass tarfile.data_filter to extractall, ensuring entries
remain confined to dest while preserving the existing extraction behavior.
In `@bench/icdar2013/score.py`:
- Around line 104-134: Update the exception handlers in run_pdftable and
run_pdfplumber to write extraction failures and their error details to stderr
before returning Counter(). Preserve the existing empty-counter fallback, and
use the extract.go-style diagnostic pattern so missing executables, subprocess
failures, and pdfplumber errors are visible.
- Around line 38-59: Update relations_from_grid to accept or derive source-cell
IDs alongside normalized grid text, preserving each covered position’s
originating cell identity. When selecting the nearest non-empty horizontal or
vertical neighbor, skip candidates whose source ID matches the current cell’s
ID, while retaining relations between distinct cells and counting each position
according to the existing traversal.
In `@docs/evaluations/2026-08-02-font-metrics-and-table-fidelity.md`:
- Around line 3-5: Update the evaluation metadata near the Commits entry to
explicitly identify the exact commit tree used for the reported measurements,
while retaining the existing commit range only as historical context.
- Around line 103-108: Update the “Findings were confirmed” statement to avoid
describing pdftoppm and pdftotext as independent sources; either label the three
items as verification signals or add a renderer/parser from a non-Poppler
implementation before retaining the claim of three independent sources.
In `@docs/evaluations/2026-08-02-icdar2013-table-structure.md`:
- Around line 30-37: Update the diagnostic output fenced block in the evaluation
report to declare the text language, preserving its existing contents and
formatting so it satisfies Markdown lint rule MD040.
- Around line 89-90: Add a “What remains untested” section in the evaluation
report before the closing roadmap note, documenting scope limitations including
scanned PDFs and document or layout classes not represented in the 125
born-digital PDFs.
In `@docs/README.md`:
- Around line 31-33: Update the report filename convention described in the
README to include a collision-free timestamp or sequence suffix after the date
and subject, ensuring same-day benchmark reruns create distinct append-only
files while preserving the existing dated-report 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: bdf11d17-72a3-47d0-b628-6c91f3a73fad
📒 Files selected for processing (11)
README.mdbench/README.mdbench/go.modbench/icdar2013/README.mdbench/icdar2013/diag.pybench/icdar2013/extract.gobench/icdar2013/run.pybench/icdar2013/score.pydocs/README.mddocs/evaluations/2026-08-02-font-metrics-and-table-fidelity.mddocs/evaluations/2026-08-02-icdar2013-table-structure.md
| for pdf, xml in pairs: | ||
| gt = gt_relations(xml) | ||
| out = subprocess.run( | ||
| [exe, "-strategy", "lines", pdf], capture_output=True, timeout=120 | ||
| ).stdout | ||
| tables = json.loads(out or b"[]") | ||
| detected_tables_total += len(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) | ||
| if nd == 0: | ||
| zero_detect += 1 | ||
| gt_regions_total += 1 | ||
| per_doc.append((os.path.basename(pdf), ng, nd, c, len(tables))) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
One slow or failing PDF aborts the entire diagnostic run.
Unlike run_pdftable in score.py (which wraps the equivalent subprocess call in try/except and falls back to an empty result), the subprocess call and json.loads here have no exception handling. A single PDF that exceeds the 120s timeout, or produces malformed stdout, raises an uncaught exception and stops the diagnostic before it reports results for the rest of the dataset.
🔧 Proposed fix
for pdf, xml in pairs:
gt = gt_relations(xml)
- out = subprocess.run(
- [exe, "-strategy", "lines", pdf], capture_output=True, timeout=120
- ).stdout
- tables = json.loads(out or b"[]")
+ try:
+ out = subprocess.run(
+ [exe, "-strategy", "lines", pdf], capture_output=True, timeout=120
+ ).stdout
+ tables = json.loads(out or b"[]")
+ except Exception as e:
+ print(f"warning: extraction failed for {pdf}: {e}", file=sys.stderr)
+ tables = []
detected_tables_total += len(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 pdf, xml in pairs: | |
| gt = gt_relations(xml) | |
| out = subprocess.run( | |
| [exe, "-strategy", "lines", pdf], capture_output=True, timeout=120 | |
| ).stdout | |
| tables = json.loads(out or b"[]") | |
| detected_tables_total += len(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) | |
| if nd == 0: | |
| zero_detect += 1 | |
| gt_regions_total += 1 | |
| per_doc.append((os.path.basename(pdf), ng, nd, c, len(tables))) | |
| for pdf, xml in pairs: | |
| gt = gt_relations(xml) | |
| try: | |
| out = subprocess.run( | |
| [exe, "-strategy", "lines", pdf], capture_output=True, timeout=120 | |
| ).stdout | |
| tables = json.loads(out or b"[]") | |
| except Exception as e: | |
| print(f"warning: extraction failed for {pdf}: {e}", file=sys.stderr) | |
| tables = [] | |
| detected_tables_total += len(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) | |
| if nd == 0: | |
| zero_detect += 1 | |
| gt_regions_total += 1 | |
| per_doc.append((os.path.basename(pdf), ng, nd, c, len(tables))) |
🧰 Tools
🪛 ast-grep (0.45.0)
[error] 44-46: Command coming from incoming request
Context: subprocess.run(
[exe, "-strategy", "lines", pdf], capture_output=True, timeout=120
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
🪛 Ruff (0.16.0)
[error] 45-45: subprocess call: check for execution of untrusted input
(S603)
🤖 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/diag.py` around lines 43 - 57, Update the subprocess and JSON
parsing flow in the diagnostic loop to catch timeout, execution, and
malformed-output errors, matching the fallback behavior of run_pdftable in
score.py. Treat any failed or invalid PDF result as empty tables, then continue
scoring and appending results for the remaining documents.
| print(f"documents : {len(pairs)}") | ||
| print(f"documents where we found NO table: {zero_detect}" | ||
| f" ({100*zero_detect/len(pairs):.0f}%)") | ||
| print(f"tables detected (total) : {detected_tables_total}") | ||
| print() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Guard against division by zero when no documents are found.
If root contains no matching -str.xml/.pdf pairs, pairs is empty and 100*zero_detect/len(pairs) at Line 61 raises ZeroDivisionError instead of reporting a clear "no documents found" message.
🛡️ Proposed fix
pairs.sort()
+ if not pairs:
+ print(f"no documents found under {root}")
+ return 1
+
zero_detect = 0🤖 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/diag.py` around lines 59 - 63, Update the reporting block in
the diagnostic script to handle an empty `pairs` collection before calculating
the zero-detection percentage. Report a clear “no documents found” message when
no pairs exist, and preserve the existing percentage output for non-empty
collections.
| ground truth. | ||
|
|
||
| ```sh | ||
| pip install pdfplumber |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
git ls-files | rg '(^|/)bench/icdar2013/README\.md$|pdfplumber|requirements|lock' || true
echo
echo "README excerpt:"
if [ -f bench/icdar2013/README.md ]; then
nl -ba bench/icdar2013/README.md | sed -n '1,80p'
fi
echo
echo "Occurrences:"
rg -n "pdfplumber 0\.11\.9|pip install pdfplumber|0\.11\.9" .Repository: hallelx2/pdftable
Length of output: 288
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "README lines:"
awk '{printf "%4d\t%s\n", NR, $0}' bench/icdar2013/README.md | sed -n '1,90p'
echo
echo "Script excerpt and pdfplumber references:"
awk '{printf "%4d\t%s\n", NR, $0}' scripts/capture_pdfplumber_text_golden.py | sed -n '1,160p'
rg -n "pdfplumber|0\.11\.9|pip install pdfplumber|pip freeze|requirements|lock" .Repository: hallelx2/pdftable
Length of output: 39482
Pin pdfplumber for the ICDAR2013 benchmark.
The ICDAR2013 evaluation reports the reference oracle as pdfplumber 0.11.9, but pip install pdfplumber can install another version. Pin the dependency in the README/run instructions or document a lockfile so later runs remain comparable.
Proposed fix
-pip install pdfplumber
+pip install pdfplumber==0.11.9📝 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.
| pip install pdfplumber | |
| pip install pdfplumber==0.11.9 |
🤖 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/README.md` at line 8, Update the ICDAR2013 benchmark
installation instruction to pin pdfplumber to version 0.11.9, ensuring future
runs use the same dependency version as the reference oracle.
| tgz = os.path.join(dest, "icdar2013.tar.gz") | ||
| if not os.path.exists(tgz): | ||
| print(f"downloading {URL}") | ||
| urllib.request.urlretrieve(URL, tgz) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Add a timeout to the dataset download.
urllib.request.urlretrieve(URL, tgz) has no timeout. A network stall hangs the harness indefinitely instead of failing with a clear error.
⏱️ Proposed fix
- urllib.request.urlretrieve(URL, tgz)
+ urllib.request.urlretrieve(URL, tgz, reporthook=None)Set a module-level timeout instead, since urlretrieve has no timeout parameter:
import socket
socket.setdefaulttimeout(60)🤖 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/run.py` at line 55, Configure a module-level network timeout
before the dataset download in run.py by importing socket and setting
socket.setdefaulttimeout(60). Keep the existing urllib.request.urlretrieve(URL,
tgz) call unchanged while ensuring stalled downloads fail after the specified
timeout.
| with tarfile.open(tgz) as t: | ||
| t.extractall(dest) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== target file excerpt =="
if [ -f bench/icdar2013/run.py ]; then
nl -ba bench/icdar2013/run.py | sed -n '1,120p'
else
echo "bench/icdar2013/run.py not found"
fi
echo
echo "== setup files =="
git ls-files | rg '(^|/(setup\.py|pyproject\.toml|requirements.*\.txt|.*setup.*\.cfg|README.*))$' | sed -n '1,80p'
echo
echo "== python version declarations =="
for f in $(git ls-files | rg '(^|/)(pyproject\.toml|setup\.py|setup\.cfg|requirements.*\.txt|README.*|.*requirements.*\.txt)$'); do
echo "--- $f ---"
rg -n 'python_requires|Requires-Python|Programming Language :: Python|python_version|python-full-version|Python\s*[0-9.]*' "$f" || true
done
echo
echo "== tarfile extractall occurrences =="
rg -n 'extractall\(|data_filter|tarfile\.open' .Repository: hallelx2/pdftable
Length of output: 221
🌐 Web query:
Python tarfile extractall filter data CVE-2007-4559 default filter None documentation
💡 Result:
CVE-2007-4559 is a directory traversal vulnerability in Python's tarfile module, where maliciously crafted tar archives containing ".." (dot-dot) sequences or symbolic links could allow an attacker to overwrite arbitrary files outside the intended destination directory [1][2]. Historically, the Python tarfile.extractall method did not implement any security checks, and official documentation warned users against extracting archives from untrusted sources without manual inspection [3][4]. To address this, Python introduced the filter argument in PEP 706 [3]. Key details regarding the filter, its defaults, and CVE-2007-4559 include: 1. Evolution of the Default Filter: - Historically (before Python 3.14), the default behavior when no filter was provided was effectively 'fully_trusted', which performed no security checks and left applications vulnerable to CVE-2007-4559 [3][4]. - Starting in Python 3.14, the default filter for extractall and extract was changed to 'data', which blocks dangerous features like absolute paths or links to files outside the destination directory [4]. - You can manually set a default behavior by configuring the TarFile.extraction_filter attribute [4]. If this is set to None (the default), the library will use the 'data' filter (in Python 3.14+) [4]. 2. Available Filters: - 'data': Intended for cross-platform data archives; it blocks or ignores dangerous features like UNIX-specific metadata or links pointing outside the extraction path [4][5]. - 'tar': Honors most UNIX-like filesystem features but blocks features deemed surprising or malicious [4][5]. - 'fully_trusted': Explicitly trusts all metadata in the archive. This should only be used if the archive is fully trusted or if the user performs their own complex validation [3][4]. 3. Newer Vulnerabilities: While the 'data' filter was designed to mitigate CVE-2007-4559, more recent vulnerabilities (e.g., CVE-2025-4517, CVE-2025-4330) have been discovered that affect the 'data' and 'tar' filters in Python 3.12 and later [6][7]. These issues allow for arbitrary filesystem writes or symlink attacks even when using these filters [6][7]. Developers should ensure their Python environment is updated to include patches for these more recent vulnerabilities, as the standard 'data' filter alone may not be sufficient on unpatched systems [6][7]. For maximum security when dealing with untrusted archives, it remains recommended to avoid extracting archives from untrusted sources whenever possible [4][2].
Citations:
- 1: https://ubuntu.com/security/CVE-2007-4559
- 2: tarfile: Traversal attack vulnerability python/cpython#65308
- 3: https://peps.python.org/pep-0706/
- 4: https://docs.python.org/3/library/tarfile.html
- 5: https://docs.python.org/release/3.11.5/library/tarfile.html
- 6: https://openwall.com/lists/oss-security/2025/06/24/1
- 7: https://mail.python.org/archives/list/security-announce@python.org/thread/MAXIJJCUUMCL7ATZNDVEGGHUMQMUUKLG/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== target file excerpt =="
if [ -f bench/icdar2013/run.py ]; then
awk '{printf "%6d %s\n", NR, $0}' bench/icdar2013/run.py | sed -n '1,120p'
else
echo "bench/icdar2013/run.py not found"
fi
echo
echo "== repository files of interest =="
git ls-files | grep -E '(^|/)(pyproject\.toml|setup\.py|setup\.cfg|requirements.*\.txt|README.*|\.python-version|Dockerfile|.*requirements.*\.txt)$' | sed -n '1,120p'
echo
echo "== python version declarations =="
for f in $(git ls-files | grep -E '(^|/)(pyproject\.toml|setup\.py|setup\.cfg|requirements.*\.txt|README.*|\.python-version|Dockerfile|.*requirements.*\.txt)$'); do
echo "--- $f ---"
grep -n -E 'python_requires|Requires-Python|Programming Language :: Python|python_version|python-full-version|Python\s*[0-9.]*' "$f" || true
done
echo
echo "== tarfile extractall occurrences =="
grep -rn 'extractall\(|data_filter|tarfile\.open' . || true
echo
echo "== deterministic tarfile behavior probe =="
python3 - <<'PY'
import sys
print(f"python={sys.version_info.major}.{sys.version_info.minor}.{sys.version_info.micro}")
import tarfile
if hasattr(tarfile.TarFile, 'extractall'):
import inspect
sig = inspect.signature(tarfile.TarFile.extractall)
print("extractall signature:", sig)
print("has extraction_filter:", hasattr(tarfile.TarFile, 'extraction_filter'))
if hasattr(tarfile, 'data_filter'):
print("has tarfile.data_filter")
PYRepository: hallelx2/pdftable
Length of output: 5686
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== tarfile occurrences =="
grep -RIn 'extractall\(|data_filter|tarfile\.open' . || true
echo
echo "== inspect repository Python references without rg/fd dependencies as needed =="
for f in README.md bench/README.md bench/icdar2013/README.md docs/README.md; do
[ -f "$f" ] || continue
echo "--- $f ---"
grep -In -E 'Python|python|tarfile|extractall|requirements|version|data_filter' "$f" || true
doneRepository: hallelx2/pdftable
Length of output: 1246
Pass a filter to tarfile.extractall().
bench/icdar2013/run.py:58 extracts a downloaded tarball without filter=. An archive entry with a ../ path can extract outside dest; use tarfile.data_filter here to keep extraction confined.
🧰 Tools
🪛 ast-grep (0.45.0)
[error] 57-57: Calling extractall() on a zipfile.ZipFile or tarfile archive without validating member paths lets a crafted entry (e.g. "../../etc/passwd") write outside the destination directory (Zip Slip). Validate each member resolves inside the target directory, or pass a safe filter (tarfile: filter="data" / tarfile.data_filter).
Context: t.extractall(dest)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(archive-extractall-path-traversal-python)
🪛 Ruff (0.16.0)
[error] 58-58: Uses of tarfile.extractall()
(S202)
🤖 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/run.py` around lines 57 - 58, Update the tarfile extraction
in the download/archive handling block to pass tarfile.data_filter to
extractall, ensuring entries remain confined to dest while preserving the
existing extraction behavior.
| **Date:** 2026-08-02 | ||
| **Commits:** `325e628` … `0ca65ca` (PRs #9, #13–#19) | ||
| **References:** rendered pages via `pdftoppm`, poppler `pdftotext -layout`, pdfplumber 0.11.9 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Record the exact commit used for the measurements.
Commits: 325e628 … 0ca65ca describes a range, but it does not identify which tree produced the reported values. Record the measured commit explicitly and keep the range only as historical context.
🤖 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 `@docs/evaluations/2026-08-02-font-metrics-and-table-fidelity.md` around lines
3 - 5, Update the evaluation metadata near the Commits entry to explicitly
identify the exact commit tree used for the reported measurements, while
retaining the existing commit range only as historical context.
| Findings were confirmed against three independent sources before being | ||
| accepted: | ||
|
|
||
| 1. the page rendered to PNG (`pdftoppm -r 150`) and read directly, | ||
| 2. poppler `pdftotext -layout`, | ||
| 3. pdfplumber 0.11.9. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files matching target name:"
fd -a '2026-08-02-font-metrics-and-table-fidelity\.md' . || true
target="$(fd '2026-08-02-font-metrics-and-table-fidelity\.md' . | head -n 1 || true)"
if [ -n "${target:-}" ]; then
echo
echo "Relevant lines:"
sed -n '90,115p' "$target" | nl -ba -v90
fi
echo
echo "Search for independent sources wording in repo:"
rg -n "independent sources|independent source|verification signals|pdftoppm|pdftotext|pdfplumber" . || trueRepository: hallelx2/pdftable
Length of output: 323
🏁 Script executed:
#!/bin/bash
set -u
target="/home/jailuser/git/docs/evaluations/2026-08-02-font-metrics-and-table-fidelity.md"
echo "Relevant lines:"
awk '{ printf "%5d %s\n", NR, $0 }' "$target" | sed -n '90,115p'
echo
echo "Search for independent sources wording in repo:"
rg -n "independent sources|independent source|verification signals|pdftoppm|pdftotext|pdfplumber" /home/jailuser/git || trueRepository: hallelx2/pdftable
Length of output: 40179
Do not count two Poppler outputs as independent sources.
pdftoppm and pdftotext are outputs from the Poppler toolchain. Describe them as verification signals, or add a PDF renderer/parser from another implementation before claiming three independent sources.
🤖 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 `@docs/evaluations/2026-08-02-font-metrics-and-table-fidelity.md` around lines
103 - 108, Update the “Findings were confirmed” statement to avoid describing
pdftoppm and pdftotext as independent sources; either label the three items as
verification signals or add a renderer/parser from a non-Poppler implementation
before retaining the claim of three independent sources.
| ``` | ||
| documents : 125 | ||
| documents where NO table was found : 28 (22%) | ||
| tables detected (total) : 306 | ||
|
|
||
| restricted to documents where a table WAS detected: | ||
| precision 0.865 recall 0.409 F1 0.556 | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Specify a language for the diagnostic output block.
Add text to the fence so the report satisfies Markdown lint rule MD040.
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 30-30: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@docs/evaluations/2026-08-02-icdar2013-table-structure.md` around lines 30 -
37, Update the diagnostic output fenced block in the evaluation report to
declare the text language, preserving its existing contents and formatting so it
satisfies Markdown lint rule MD040.
Source: Linters/SAST tools
| Tracked as HAL-568. Re-run this harness after each and record a new dated | ||
| report here. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add the required untested-scope section.
docs/README.md states that every evaluation report must document what remains untested. Add a ## What remains untested section before the closing roadmap note. Include scope limits such as scanned PDFs and document or layout classes absent from the 125 born-digital PDFs.
🤖 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 `@docs/evaluations/2026-08-02-icdar2013-table-structure.md` around lines 89 -
90, Add a “What remains untested” section in the evaluation report before the
closing roadmap note, documenting scope limitations including scanned PDFs and
document or layout classes not represented in the 125 born-digital PDFs.
| Reports are append-only. Re-running a benchmark adds a new dated file | ||
| rather than editing an old one, so a regression is visible as a diff | ||
| between two reports instead of vanishing into a rewrite. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Use collision-free filenames for append-only reports.
The current filenames use YYYY-MM-DD-<subject>.md. Two runs of the same benchmark on the same date would target the same path, which conflicts with the append-only requirement.
Define a timestamp or sequence suffix for same-day reruns.
🤖 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 `@docs/README.md` around lines 31 - 33, Update the report filename convention
described in the README to include a collision-free timestamp or sequence suffix
after the date and subject, ensuring same-day benchmark reruns create distinct
append-only files while preserving the existing dated-report behavior.
1 participant
Benchmark harnesses and measurement findings had no home. The ICDAR 2013 harness was living in a temp directory, and every result from this work existed only in Linear — which records that something was decided, not what was measured.
What this adds
bench/is a separate Go module, deliberatelyNested modules are excluded from the parent's
./..., so the library build ignores it and a consumer ofpdftablenever fetches its dependencies.The first version of this commit got that wrong —
go list ./...showedgithub.com/hallelx2/pdftable/bench/icdar2013inside the library module, which is precisely the leak the split exists to prevent. Fixed and verified:Datasets are not committed.
run.pyfetches ICDAR 2013 (~12 MB) into a cache dir, builds the extractor as its own module against the working tree, and scores pdftable alongside pdfplumber — "is 0.36 good?" is unanswerable without a baseline.docs/evaluations/— dated, append-onlyEach report states the commit it ran against, the external oracle used, the metric precisely, what the number is not comparable to, and what remains untested.
That last part is the point. A benchmark number without its scope reads as a general claim. The 0.362 here is end-to-end detection plus structure — not the structure-only task that papers report 0.85–0.95 on, where the system is handed the table region.
Append-only means a regression appears as a diff between two dated files rather than vanishing into a rewrite.
What did NOT move, and why
The library source stays in the repository root. For a single-package Go library the root is the import path — moving
page.gointosrc/orpkg/breaks every consumer, andpkg/is a recognised Go anti-pattern. Go also deliberately keeps_test.gobeside the code it covers.The README now documents the layout, so the root is legible rather than merely conventional.
Verification
go build ./...,go vet ./...,go test ./... -count=1 -race— green.bench/confirmed absent fromgo list ./....Relates to HAL-568
Summary by Sourcery
Add a standalone benchmarking module and documented evaluation results for font metrics and table extraction accuracy.
New Features:
Enhancements:
Summary by CodeRabbit
Documentation
New Features