fix: page # should be completely removed with remove_headers - #798
fix: page # should be completely removed with remove_headers#798ultrasaurus wants to merge 8 commits into
Conversation
yfedoseev
left a comment
There was a problem hiding this comment.
Thanks for the focused repro, @ultrasaurus — and nice work building the synthetic PDF helper; that's exactly the right way to pin this down. Your instinct is correct: "page #" should not be governed by the first-occurrence logic, and it lines up with the spec.
Why remove_headers_removes_word_plus_page_number fails (page 0 leaks): in mark_running_artifact_spans the guard
if let Some(&first_seen_on) = signatures.get(&sig) {
if page_index == first_seen_on { continue; } // exempts page 0's "page 1"
...
}is a cover-page-title protection — it keeps the first appearance of a running line in case that first line is actually the document title rather than chrome. The problem is it fires unconditionally, so for a varying-literal signature like "page #" it exempts page 0's "page 1" while stripping "page 2".."page 5". Pass 2 then can't catch it either, because it dedups by exact text and "page 1".."page 5" are five distinct one-off keys below the occurrence threshold. So page 0 leaks.
The fix is exactly your decoupling, and it's spec-aligned. ensure_running_artifact_signatures already computes literal_variants per signature but then discards it — the signatures map only carries first_seen_on. Carry an is_varying flag alongside it (or the variants count) and gate the exemption on !is_varying:
- constant-literal signature (
variants < 2): first-occurrence exemption stays — it may genuinely be a title that only looks like chrome because later pages repeat it. - varying-literal signature (
variants >= 2): the digits change per page, so it's a folio/running-head by definition — its first occurrence ("page 1","Chapter 3 42") is chrome exactly like the rest and must not be exempted.
ISO 32000-1 §14.8.2.2 backs this up: it groups "running heads and folios (page numbers)" as a single pagination-artifact class "typically used on every page" — first page included — so the varying page-number component is intrinsic to the artifact and shouldn't defeat detection of the constant part. Go ahead with the focused fix.
Two notes before it's a merge candidate (same as on #795, so no surprises):
- This branch also carries the #794 behavior change (band-matching
Other/PageNumber, parity detection), so it's not test-only — it'll need the full ~400-PDF corpus sweep before merge, same as #795. Might be cleanest to split the pure test-coverage commit from the behavior change so the tests can land independently. - Please drop
data/1965-nelson.pdf— your in-code synthetic builders are the right pattern; the example can take a path argument instead of a committed third-party PDF.
And note this connects to the line-grouping idea from your #795 comment: once signatures are computed at the line level, "word + page number" headers fall out naturally (the whole line "Chapter 3 #" is the signature, first page included), which addresses both this and the "the" false positive with one mechanism.
4865f14 to
c6a236e
Compare
|
apologies -- I was testing on both branches. PR now rebased on main, which shows the same failing test: |
|
Thanks for the careful test coverage here, and for asking before diving into a fix — this is a good catch. I agree with your instinct: "page #"-style (word + varying number) headers shouldn't be lumped in with the first-occurrence exemption. Here's the reasoning, plus what I found checking how other tools in this space handle the same question. Why it's wrong today: How other tools handle this: I checked PyMuPDF, Docling, unstructured.io, Adobe's own header/footer tool, and the closest academic precedent (Xiaofan Lin's HP Labs paper on header/footer extraction by page-association — which uses essentially the same "normalize digits to a placeholder, detect cross-page recurrence" approach this codebase already uses). None of them exempt the first occurrence of a digit-varying pattern — once something is classified as running/varying page furniture, every matching occurrence is stripped, including page 1's. The exemption is really only a "real repeated title" special case, not a general first-page carve-out. Go ahead and put together a focused fix along those lines — happy to review it. And since you're already deep in this area: I noticed while looking at #800 that the digit-detection here ( |
c6a236e to
735ceb2
Compare
|
thank you for the detailed advice I added another failing test, an example and a fix that causes both tests to pass |
- remove_headers_removes_simple_page_number - passes - remove_headers_removes_word_plus_page_number - fails The second test hits the mechanism WITH the first-occurrence which should not apply to page numbers with text, like "Page X"
When a header/footer span has text with just a digit changing then it's a page number element, which should be completely removed (not needing the first occurence exception)
735ceb2 to
80726b3
Compare
|
Thanks for pushing this through, @ultrasaurus — the core decoupling is exactly right and spec-aligned: carrying Before merging I ran the corpus sweep I flagged earlier (this branch changes real detection behavior, so it needs it). Methodology, so it's reproducible: The good news: the fix does what it's meant to. Confirmed-correct removals include genuine running footers that previously leaked their first-page occurrence — e.g. a The concern — it also deletes real content on page 0. The sweep surfaced two clear false positives, both the "number embedded in substantive text" case:
(Two more are borderline: Root cause: the first-occurrence exemption was doing double duty. Besides shielding a cover-page title, it was incidentally protecting legitimate first-page numbered content that shares the varying-digit signature shape — form line labels ( Suggested direction: keep the decoupling, but narrow the un-exemption so it only fires when the varying signature is a pure folio — bare Coverage note: the sweep completed ~390/419 docs (I OOM'd my own run on a large book by running two dumps at once); I'll finish the remaining ~29 sequentially and post the full counts, but the two cases above are already conclusive on the direction. Separately — I'm opening a follow-up issue for the RTL/CJK universality gap we discussed ( |
|
thanks for running the big corpus.. will have to give that some thought, and maybe we need some more tests! Meanwhile, I'll pull out the example and submit in a separate PR, since it would be helpful for me in my interactive testing (and others who are learning) |
Page numbers may appear with a brand (e.g. 'ACM - 80') and there may be important form information (e.g. 'Form 1a') that should be kept. The strong signal is the number monotonically increases. Gaps are allowed, such as with a full page image.
|
Recent change uses the number itself as a stronger signal. Monotonically increasing numbers are detected as page numbers (not simply varying), and so are no longer exempted from removal on their first occurrence. The code currently sets two as the floor; for longer documents the existing 50%-of-pages recurrence threshold already requires more. This addresses my motivating case (1965 Nelson paper, with "ACM • 84" as the folio). I'll wait to submit as a PR till #795 has been reviewed, since they are both in the same area of the code. I welcome feedback in the meantime @yfedoseev |
|
Ran a correctness review plus our native corpus-signature sweep ( Corpus result: 5/419 files changed (all small char-count drops, word-max-length and page rotation identical everywhere — no word-fusion, no crashes). Consistent with the intended scope. One of those five is worth a second look: Code-level gap: the fix lifts the first-occurrence exemption for signatures that are both varying and sequential, but for the motivating counter-case ( Process: no |
|
Draft, so just a status note from a pass over it today.
Also flagging overlap so it doesn't bite later: this, #795 and #967 all touch header/footer/artifact removal, and #795 and #967 additionally collide on a test filename. The sequential-page-number requirement you added here looks like it interacts with the parity work in #795 in particular — landing them in a deliberate order will save you re-doing the heuristics twice. No changes requested from me at this stage; happy to run the full sweep on it once it's rebased and out of draft. |
|
@ultrasaurus — picking this back up, and starting with a correction I owe you. Back in July I flagged Which makes the new That is the only code change I would ask for. The wider tangle — a second header/footer detector carrying its own ASCII-only normaliser, and several inconsistent notions of "digit" across the crate — is ours, and is tracked as #1120. Two limits worth a line in the doc comment, since both silently keep the exemption: roman-numeral front matter produces no digit runs at all, and section-restarting pagination (1,2,3,1,2,3) fails the strict-increase test. Both are defensible; neither is obvious from the code. The sequential-folio discriminator itself is the right shape, for what it is worth — "the digit tracks page order" is what separates a folio from a renumbered form label, and it keeps the cover-page case working. On the rebase: the branch is still |
Description
"page #" pattern should not be lumped in with first occurrence logic -- all occurrences should be removed.
Type of Change
Changes Made
added two tests
- remove_headers_removes_simple_page_number - passes
- remove_headers_removes_word_plus_page_number - passes
- remove_headers_with_text_and_pagenum_keeps_top_line_of_text - passes
I expected the second bug to pass (was just adding some test coverage).
Testing
cargo test --all-featurescargo clippy -- -D warningscargo fmtPython Bindings (if applicable)
ruff formatruff checkDocumentation
Checklist
feat:,fix:,docs:)Screenshots (if applicable)
Additional Notes
I didn't change the name of the branch now that it has a fix, let me know if you want me to