fix(underline): rescue snug-owned underlines from the table-ruling filters#143
Conversation
…lters Documents that underline many full-width lines (dense CJK business docs, legal redlines, 10-K section links) produce span-similar rules at 3+ y-levels — exactly what the repeated-ruling filter treats as table rulings, so every semantic underline on such pages was discarded. Three changes fix detection without re-marking real tables: 1. Snug-owner rescue: a rule survives the repeated-ruling filter when the union of touching text runs on its baseline row owns it (rule contained within the union's span +0.75em, runs cover >=60% of the rule, no column-sized gaps between runs). Table row separators fail ownership: they overshoot their cells' text or match gapped items. Same-row segmented rules (column-header separators) always stay discarded, and a rule enclosed by a drawn cell-sized box (rect-grid tables) is never rescued. 2. Vertical window widened 0.35em -> 0.72em below the baseline: CJK layouts draw underlines under the full em box, measured at ~0.67em. 3. Prose-table guard in the positions-path suppressor: a detected 'table' whose cells hold flowing prose (>=30% of cells over 100 chars) is a detection artifact of boxed callouts + stacked rules, not a real table — suppressing there erased every underline on the page. Also fixes cluster_x_positions fabricating phantom table columns from style-split continuation runs (touching items, gap <2pt, now feed one column start) — the fix that keeps rect-grid table shapes stable while underlined links inside cells are correctly marked. Snapshot updates are underline gains on regulation/form fixtures and one empty spacer-column change in a subscripted header. Corpus (508-doc public bench sweep): text output byte-identical on all docs; underlined items +224/-0; strikeout now fires on redline docs. Item-level GT coverage: is_underline 86->151/405, is_strikeout 0->10/44. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L3U6BKYCS73DVA83odAfYB
There was a problem hiding this comment.
2 issues found across 6 files
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/extractor/underline.rs">
<violation number="1" location="src/extractor/underline.rs:177">
P2: Tall drawn table cells can now be marked as underlined: boxes over 90pt bypass the only flank check for `re`-drawn rectangles, then the snug-owner path rescues their repeated row rules. Remove the height ceiling (or classify tall cell boxes separately) so any enclosing drawn cell box remains non-rescuable as described.</violation>
</file>
<file name="src/tables/detect_rects.rs">
<violation number="1" location="src/tables/detect_rects.rs:2384">
P2: Overlapping text from adjacent cells now loses the later column start: a gap of `-50pt` satisfies `< 2.0` just like touching runs. Bound the negative tolerance so only genuinely touching style runs are suppressed; otherwise narrow tables with long cell values can collapse columns.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
… marks Found by a 202-doc real-world corpus diff (pdf-evals) that exercises the full markdown pipeline, which the bench-corpus item sweep does not: 1. Math fraction bars and lattice grid lines are underline geometry — short horizontal rules under digits. Guard: a narrow rule (<=60pt) with bar-sized text hanging just below it (denominator) never marks. The below-text width bound matters: tightly-leaded REAL underlines have a full-width next line below, which must not trip the guard. 2. merge_subscript_items refused to merge when the parent was underlined but the tiny digit was not (the drawn rule easily misses the digit's own overlap window) — losing the merge broke subscript tokens inside table cells (b+2 no longer became b₂). Strikeout boundaries still block the merge in both directions; only parent-underlined/digit-bare merges, absorbing with the parent's flags. Corpus after refinement: underlined items +220/-2 (the 2 are fraction bars the old code wrongly marked), GT rule-text coverage 149/405 underline + 10/44 strikeout, text output identical on all 508 bench docs and word-count-identical on the 202 pdf-evals docs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L3U6BKYCS73DVA83odAfYB
|
Pushed two refinements found by running the full pdf-evals 202-doc corpus diff (baseline-vs-branch binaries — this exercises the complete markdown pipeline, which the item-level bench sweep does not):
Final corpus evidence: bench 508 docs — text byte-identical, underlined items +220/−2 (both removals are fraction bars the old code wrongly marked); GT rule coverage 149/405 underline, 10/44 strikeout. pdf-evals 202 docs — word-count-identical everywhere; diffs are Known cosmetic limitation: short grid lines under short diagram labels (math lattice figures) still mark — geometrically indistinguishable from a short underline; ~3 instances corpus-wide, markup-only. |
There was a problem hiding this comment.
2 issues found across 2 files (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/extractor/underline.rs">
<violation number="1" location="src/extractor/underline.rs:435">
P2: Short strikeouts can be dropped when nearby lower text makes the rule look like a fraction. Apply `fraction_rules` only to underline matching so `rule_strikes_item` still evaluates.</violation>
</file>
<file name="src/extractor/mod.rs">
<violation number="1" location="src/extractor/mod.rs:913">
P2: An ununderlined superscript/footnote immediately after underlined text is now emitted inside the parent's `<u>` span. `is_underline == false` also represents a genuinely unruled digit, so retain the underline boundary unless geometry positively establishes that the rule covers it.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| let marks_ok = parent.is_strikeout == item.is_strikeout | ||
| && (parent.is_underline == item.is_underline | ||
| || (parent.is_underline && !item.is_underline)); | ||
| if parent.font_size >= sub_threshold && ends_with_letter && marks_ok { |
There was a problem hiding this comment.
P2: An ununderlined superscript/footnote immediately after underlined text is now emitted inside the parent's <u> span. is_underline == false also represents a genuinely unruled digit, so retain the underline boundary unless geometry positively establishes that the rule covers it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/extractor/mod.rs, line 913:
<comment>An ununderlined superscript/footnote immediately after underlined text is now emitted inside the parent's `<u>` span. `is_underline == false` also represents a genuinely unruled digit, so retain the underline boundary unless geometry positively establishes that the rule covers it.</comment>
<file context>
@@ -903,9 +903,17 @@ pub(crate) fn merge_subscript_items(items: Vec<TextItem>) -> Vec<TextItem> {
+ // the drawn rule easily misses the tiny digit's overlap
+ // window, and refusing costs the whole subscript token
+ // ("b"+"2" staying split). Visually the rule spans both.
+ let marks_ok = parent.is_strikeout == item.is_strikeout
+ && (parent.is_underline == item.is_underline
+ || (parent.is_underline && !item.is_underline));
</file context>
| let marks_ok = parent.is_strikeout == item.is_strikeout | |
| && (parent.is_underline == item.is_underline | |
| || (parent.is_underline && !item.is_underline)); | |
| if parent.font_size >= sub_threshold && ends_with_letter && marks_ok { | |
| let same_marks = parent.is_underline == item.is_underline | |
| && parent.is_strikeout == item.is_strikeout; | |
| if parent.font_size >= sub_threshold && ends_with_letter && same_marks { |
There was a problem hiding this comment.
Deliberate trade, keeping as-is: rules aren't available at merge time, so geometry can't positively establish coverage here — and the failure modes are asymmetric. Refusing the merge corrupts the token (b+2 no longer forms the subscript, measured in table cells on the corpus); merging extends a span over one digit (cosmetic). Strikeout boundaries still block both directions, which is where extension would be semantically wrong (redline docs).
There was a problem hiding this comment.
The strikeout point is fair, but for the underline/subscript case the parent comment was too broad here: refusing the merge breaks the b+2 token, while merging only overextends a <u> span cosmetically. The strikeout boundary still looks like the real semantic guard.
|
@abimaelmartell I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
2 issues found across 6 files
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/extractor/underline.rs">
<violation number="1" location="src/extractor/underline.rs:172">
P2: Repeated genuine underlines inside a filled callout or highlighted panel are discarded as table rulings. `rect_flank` treats any medium-height fill spanning the rule as a cell box; require grid/border evidence before using generic fill rectangles as a table veto.</violation>
<violation number="2" location="src/extractor/underline.rs:418">
P2: Real underlines above a short, normally leaded next line are suppressed as fraction bars. The guard measures the gap to the next item's top, so normal line leading falls inside the 0.5em denominator window; distinguish actual fraction geometry without treating ordinary following lines as denominators.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
…raction guard, bounded gaps - Cell-box veto now requires GRID EVIDENCE (a vertically abutting neighbor rect with x-overlap) instead of a height window: multiline table cells taller than the old 90pt ceiling veto again, and isolated filled callout panels (which legitimately contain underlines) no longer veto at all. - The fraction guard gates only UNDERLINE marking; rule_strikes_item still evaluates, so short strikeouts near lower text survive. - Fraction hug distance tightened to 0.3em so a short last-line at normal leading is not mistaken for a denominator. - Continuation-run suppression bounds the negative gap (-4pt): text overhanging from an adjacent cell keeps its own column start. Corpus after review fixes: underlined items +222/-2, GT coverage 150/405 underline + 10/44 strikeout, bench text output identical on all 508 docs, pdf-evals word loss bounded at equation-reflow noise. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L3U6BKYCS73DVA83odAfYB
|
All six review comments addressed — five fixed in the latest commit, one defended inline (subscript-merge trade: token corruption vs one-digit cosmetic underline extension). Re-verified after the fixes: 756 tests pass; bench corpus 508 docs text-identical, underlined items +222/−2; GT rule coverage 150/405 underline + 10/44 strikeout; pdf-evals 202-doc corpus word-loss bounded at equation-reflow noise. ALL GREEN. |
|
@abimaelmartell I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
2 issues found across 6 files
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_rects.rs">
<violation number="1" location="src/tables/detect_rects.rs:2387">
P2: Narrow cells whose text ends within 2pt of the next cell lose the next column entirely, even though those are separate table cells. A tighter touch-only threshold or cell-boundary evidence would avoid merging legitimate adjacent columns.</violation>
</file>
<file name="src/extractor/underline.rs">
<violation number="1" location="src/extractor/underline.rs:144">
P2: Borderless single-column ruled tables now mark their cell text as underlined. The snug-owner rescue bypasses the repeated-ruling filter, while the remaining tabular check only recognizes three separated items; retain the repeated-rule suppression when no independent non-tabular signal distinguishes this case.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| // adjacent cell overlaps by far more than italic kerning ever | ||
| // does, and must still start its own column. | ||
| let gap = item.x - (prev.x + prev.width); | ||
| (prev.y - item.y).abs() <= 2.0 && gap < 2.0 && gap > -4.0 && item.x >= prev.x |
There was a problem hiding this comment.
P2: Narrow cells whose text ends within 2pt of the next cell lose the next column entirely, even though those are separate table cells. A tighter touch-only threshold or cell-boundary evidence would avoid merging legitimate adjacent columns.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/tables/detect_rects.rs, line 2387:
<comment>Narrow cells whose text ends within 2pt of the next cell lose the next column entirely, even though those are separate table cells. A tighter touch-only threshold or cell-boundary evidence would avoid merging legitimate adjacent columns.</comment>
<file context>
@@ -2367,7 +2367,29 @@ fn detect_merged_cluster_table(
+ // adjacent cell overlaps by far more than italic kerning ever
+ // does, and must still start its own column.
+ let gap = item.x - (prev.x + prev.width);
+ (prev.y - item.y).abs() <= 2.0 && gap < 2.0 && gap > -4.0 && item.x >= prev.x
+ };
+ if !is_continuation {
</file context>
There was a problem hiding this comment.
Keeping the 2pt threshold, with corpus evidence: a start dropped here is only lost as a COLUMN VOTE — any row of the same table where the cells keep normal padding (cell padding is typically >=4pt/side) restores the column. For the column to vanish, text would have to touch within 2pt across the boundary in essentially every row — at which point there is no visual column boundary to preserve. Across the 710-doc verification (508 bench + 202 pdf-evals), table shapes changed on exactly 3 docs, all inspected: one restored column (the regression this bound fixed), one empty spacer column in a subscript-soup header, and one cover page that stopped rendering as a phantom table.
There was a problem hiding this comment.
Thanks for the feedback! I've saved this as a new learning to improve future reviews.
| // always rulings — each segment snugly owns its column label, | ||
| // so snugness must not override that check. | ||
| !is_segmented_row_ruling_rule(rule, &rules) | ||
| && ((has_snug_text_owner(rule, items) |
There was a problem hiding this comment.
P2: Borderless single-column ruled tables now mark their cell text as underlined. The snug-owner rescue bypasses the repeated-ruling filter, while the remaining tabular check only recognizes three separated items; retain the repeated-rule suppression when no independent non-tabular signal distinguishes this case.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/extractor/underline.rs, line 144:
<comment>Borderless single-column ruled tables now mark their cell text as underlined. The snug-owner rescue bypasses the repeated-ruling filter, while the remaining tabular check only recognizes three separated items; retain the repeated-rule suppression when no independent non-tabular signal distinguishes this case.</comment>
<file context>
@@ -115,20 +115,156 @@ fn rules_from_graphics(rects: &[PdfRect], lines: &[UnderlineLine], page: u32) ->
+ // always rulings — each segment snugly owns its column label,
+ // so snugness must not override that check.
+ !is_segmented_row_ruling_rule(rule, &rules)
+ && ((has_snug_text_owner(rule, items)
+ && !has_flanking_verticals(rule, rects, lines, page))
+ || !is_repeated_ruling_rule(rule, &rules))
</file context>
There was a problem hiding this comment.
This ambiguity is real but it is THE core trade of this PR, resolved empirically: a borderless single-column ruled table and a page of underlined full-width lines are geometrically identical — no local signal separates them (that is why the old filter deleted all of one class to protect the other). The ground truth we have points the other way: text_dense__underline (a doc of exactly this shape) expects underlines, +64 ParseBench is_underline rules flip to covered, and the 0.72em baseline window already excludes most row separators (which sit at row midpoints, ~0.7-1.2em below the text). Corpus check across 710 docs: the snapshot diffs this produced were genuine underlines (regulation defined-terms, form headings, in-cell hyperlinks), with zero content changes. If a real single-column-ruled-table regression shows up we can revisit with rect-edge row evidence, but I don't want to re-lose the 64 rules to a theoretical case.
There was a problem hiding this comment.
The parent comment is too broad here: the corpus evidence says these repeated rules are genuine underlines, not table rulings. The repeated-rule suppression should stay narrowed to cases with real non-tabular evidence, and this PR’s shape doesn’t have that.
Thanks for the feedback! I've saved this as a new learning to improve future reviews.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L3U6BKYCS73DVA83odAfYB
Problem
Geometric underline detection (shipped with the ENG-5015 round) almost never fired on real documents: fire-pdf's
<u>emission produced zero output on docs liketext_dense__underline(48 items, allisUnderline: false). Three independent suppressors were eating them:Fix
cluster_x_positionsno longer fabricates phantom table columns from style-split continuation runs (gap <2pt) — this keeps rect-grid table shapes stable while underlined links inside cells are marked (the multiline-indent fixture's links are genuinely underlined).Verification
<u>Expiration date</u>), one empty spacer-column change in a subscripted table header🤖 Generated with Claude Code
https://claude.ai/code/session_01L3U6BKYCS73DVA83odAfYB
Summary by cubic
Fixes underline detection and adds a fraction-bar guard to avoid false positives on dense CJK and redline docs without re-marking tables (supports ENG-5034). Also restores subscript merges where underlines previously blocked b₂ formation.
cluster_x_positions: skip near-zero-gap continuation runs (<2pt) and bound negative overlap (>-4pt) to prevent phantom columns.Written for commit 656f094. Summary will update on new commits.