test(bench): sweep the in-region thresholds — it is not a tuning problem - #28
Merged
hallelx2 merged 1 commit intoSep 17, 2026
Conversation
HAL-1363 proposed that the precision gap after region detection (0.281 against camelot's 0.514) was a threshold tuned for a whole page and wrong for a small region: a five-row table has five words per column, so MinWordsVertical=3 should be emitting spurious boundaries. Twelve configurations say no, and say the hypothesis was backwards. Lowering MinWordsVertical is catastrophic — minwv=1 halves precision to 0.128 and drags recall down with it. The default is already the strictest value tested and sits at or near the optimum. It is not causing the problem, it is the only thing holding precision up. Padding is similarly exhausted. Beyond the default it degrades monotonically, 0.274 to 0.261 to 0.247 across pad 2 to 4, buying no recall worth having. The best config in the whole sweep beats baseline by 0.005, which is noise. So the reachable precision range is 0.128 to 0.289 and camelot's 0.514 is not in it. No default change fixes this and no further sweeping is worth the wall-clock. What remains is the gridding algorithm itself: given the same region, camelot divides it into cells differently, and that difference is worth about 0.23 precision. One method note worth keeping. A three-document smoke run put pad=2.0 first at F1 0.628; over 125 documents that config is sixth and the ranking inverts. The cheap sample is for checking the harness runs, not for reading a result off — it would have sent the next few days in precisely the wrong direction. PadLines is now configurable, following the same zero-means-default rule as every other field in TextEdgeOpts, with a negative value as the explicit none. The inconsistent version shipped briefly in the previous commit would have silently disabled padding for anyone using a partially filled struct. Both sweeps agree to every decimal place across all twelve configs.
Reviewer's GuideThis PR adds a reproducible 125-document sweep of in-region gridding thresholds and padding, showing that configuration changes cannot close the precision gap and identifying the gridding algorithm as the next investigation target. It also formalizes configurable region padding with consistent zero-means-default semantics and regression tests. Sequence diagram for reproducible configuration scoringsequenceDiagram
participant Sweep as sweep.py
participant Extract as bench-extract
participant Score as score.py
participant Corpus as ICDAR corpus
loop each document and configuration
Sweep->>Extract: run_cfg(exe, pdf, cfg)
Extract->>Corpus: extract table rows
Corpus-->>Extract: JSON rows
Extract-->>Sweep: relations
Sweep->>Score: score(gt, got)
Score-->>Sweep: precision, recall, F1
end
Sweep->>Sweep: sort results by F1
Flow diagram for the in-region threshold sweepflowchart LR
Corpus[125-document ICDAR corpus] --> Sweep[sweep.py]
Sweep --> Extract[bench-extract]
Extract --> Metrics[Per-document precision recall F1]
Metrics --> Results[Compare 12 configurations]
Results --> Decision{Precision reaches camelot's 0.514?}
Decision -- No --> Algorithm[Investigate region gridding algorithm]
Decision -- Yes --> Defaults[Consider threshold or padding change]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Review limit reachedNext included review available in 5 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (7)
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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 security issue, and 2 other issues
Security issues:
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="bench/icdar2013/sweep.py" line_range="47-50" />
<code_context>
+ cmd += ["-pad", str(cfg["pad"])]
+ cmd.append(pdf)
+ try:
+ out = subprocess.run(cmd, capture_output=True, timeout=120).stdout
+ return relations([t["rows"] for t in json.loads(out or b"[]")])
+ except Exception:
+ return Counter()
+
+
</code_context>
<issue_to_address>
**issue (testing):** Extractor failures, timeouts, nonzero exit statuses, and malformed output are all converted into an empty `Counter`, which is scored as a valid zero-result document. A broken configuration therefore silently lowers its precision, recall, and F1 and can produce false conclusions about the threshold sweep.
**Triggers:** When the extractor fails or emits invalid JSON for any document/configuration.
**Suggested fix:** Check `subprocess.run(...).returncode` and raise or record the failure instead of returning an empty result; report failed runs separately from genuine empty extraction results.
</issue_to_address>
### Comment 2
<location path="docs/README.md" line_range="23" />
<code_context>
| date | subject | headline |
| --- | --- | --- |
+| [2026-09-17](evaluations/2026-09-17-region-gridding-threshold-sweep.md) | in-region gridding threshold sweep | **negative** — 12 configs, precision never leaves 0.28. Not a tuning problem; the gridding algorithm is |
| [2026-09-17](evaluations/2026-09-17-text-edge-region-detection.md) | text-edge region detection (Nurminen) | **partial** — F1 0.245 → 0.337 over page-wide `text`, but below `lines` 0.442. Recall transferred, precision did not: gridding is now the bottleneck |
| [2026-09-17](evaluations/2026-09-17-field-comparison-and-metric-correction.md) | the field, and a metric correction | **per-doc F1 0.442**, 5th of 10, ~10x faster than anything comparable. No Go library comes close. Earlier figures were pooled, not the competition's metric |
</code_context>
<issue_to_address>
**nitpick:** The documentation headline says precision never leaves 0.28, but the same committed results include `minwv=1` at precision 0.128. The headline contradicts the table and the later stated range of 0.128–0.289.
**Suggested fix:** Change the headline to state that precision spans 0.128–0.289, or otherwise qualify the claim so it does not contradict the reported sweep.
</issue_to_address>
### Comment 3
<location path="bench/icdar2013/sweep.py" line_range="47" />
<code_context>
out = subprocess.run(cmd, capture_output=True, timeout=120).stdout
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>
Comment on lines
+47
to
+50
| out = subprocess.run(cmd, capture_output=True, timeout=120).stdout | ||
| return relations([t["rows"] for t in json.loads(out or b"[]")]) | ||
| except Exception: | ||
| return Counter() |
There was a problem hiding this comment.
issue (testing): Extractor failures, timeouts, nonzero exit statuses, and malformed output are all converted into an empty Counter, which is scored as a valid zero-result document. A broken configuration therefore silently lowers its precision, recall, and F1 and can produce false conclusions about the threshold sweep.
Triggers: When the extractor fails or emits invalid JSON for any document/configuration.
Suggested fix: Check subprocess.run(...).returncode and raise or record the failure instead of returning an empty result; report failed runs separately from genuine empty extraction results.
|
|
||
| | date | subject | headline | | ||
| | --- | --- | --- | | ||
| | [2026-09-17](evaluations/2026-09-17-region-gridding-threshold-sweep.md) | in-region gridding threshold sweep | **negative** — 12 configs, precision never leaves 0.28. Not a tuning problem; the gridding algorithm is | |
There was a problem hiding this comment.
nitpick: The documentation headline says precision never leaves 0.28, but the same committed results include minwv=1 at precision 0.128. The headline contradicts the table and the later stated range of 0.128–0.289.
Suggested fix: Change the headline to state that precision spans 0.128–0.289, or otherwise qualify the claim so it does not contradict the reported sweep.
| cmd += ["-pad", str(cfg["pad"])] | ||
| cmd.append(pdf) | ||
| try: | ||
| out = subprocess.run(cmd, capture_output=True, timeout=120).stdout |
There was a problem hiding this comment.
security (python.lang.security.audit.dangerous-subprocess-use-audit): Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
Source: opengrep
hallelx2
deleted the
halleluyaholudele/hal-1363-pdfgrab-cell-inference-inside-a-detected-region
branch
September 17, 2026 22:53
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Answers the cheap question from HAL-1363 before anyone spends days on the expensive one.
The hypothesis, and why it was backwards
HAL-1363 proposed the precision gap after region detection (0.281 vs camelot's 0.514) was a threshold tuned for a whole page and wrong for a small region — a five-row table has five words per column, so
MinWordsVertical=3should be producing spurious boundaries.Twelve configurations, 125 documents:
pad=nonepad=0.5minwh=2pad=2.0pad=4.0minwv=2minwv=1Lowering
MinWordsVerticalis catastrophic. The default is already the strictest value tested and is the only thing holding precision up. Padding beyond the default degrades monotonically. Best config beats baseline by 0.005 — noise.Reachable precision range: 0.128–0.289. Camelot's 0.514 is not in it.
What this settles
The gap is not configuration. No default change fixes it, and no further sweeping is worth the wall-clock. What remains is the gridding algorithm itself — given the same region, camelot divides it into cells differently, and that difference is worth ~0.23 precision.
_generate_columns_and_rowsincamelot/parsers/base.pyis the next and only sensible step.The oracle result still bounds the opportunity at 0.935 given a correct grid, so the ceiling is not the problem.
A method note worth keeping
A three-document smoke run put
pad=2.0first at F1 0.628. Over 125 documents that config is sixth, and the ranking inverts. The cheap sample is for checking the harness runs, never for reading a result off — it would have sent the next few days in exactly the wrong direction.Also in here
PadLinesis now configurable and follows the same zero-means-default rule as every other field inTextEdgeOpts, with a negative value as the explicit "none". The inconsistent version that shipped in #27 would have silently disabled padding for anyone using a partially-filled struct — covered by a new test.Both sweeps agree to every decimal place across all twelve configs. Local:
gofmt,go build,go vet,go test ./...green. CI red for HAL-1354.Closes HAL-1363
Summary by Sourcery
Determine whether in-region precision can be improved through threshold tuning and establish algorithmic gridding as the next area of investigation.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests: