Review view: layout decides, never script (frozen base fc2dc41) - #504
Open
rejojer wants to merge 5 commits into
Open
Review view: layout decides, never script (frozen base fc2dc41)#504rejojer wants to merge 5 commits into
rejojer wants to merge 5 commits into
Conversation
extract_toc refused any PDF under 300 text weight, or 200 on its densest page, or with mostly-landscape pages, and returned an empty outline. Both rules threw away documents the detector handles: three to five pages of heading plus a line yield every heading, and an ordinary slide deck yields one node per slide. Only the no-alphabetic-script condition stays, and its result now says so with toc_source="unreadable" instead of borrowing "detected"; toc_source is set on every result (detected, bookmarks, pages, unreadable). When detection yields nothing for a document that has text, the pages are the tree: page_index_flash emits one node per text-bearing page, titled by its first block and labelled toc_source="pages". A flat tree larger than FLAT_TREE_MAX_NODES is returned without the optimize and summary passes, since the managed pipelines refuse it: flash_rejection_reason() gives the local client and the CLI one policy, pointing at standard mode for an oversized flat tree, and for an unreadable PDF naming the missing text (standard mode stays the hint there too, since a PDF of nothing but numbers lands in the same bucket and the model can still read it). The nine example PDFs extract byte-identically before and after: the removed rules never fired on ordinary documents.
extract_toc refused a PDF whose dominant script fell in Scholar's "other" family, which is not a script but a bucket: Arabic, Hebrew, Persian, Urdu, Devanagari, Bengali, Tamil, Thai, Khmer, Georgian, Armenian, Amharic, and any document with no letters at all, each reported as "no alphabetic text". Three more rules keyed on script inside the estimator: an unnumbered heading in a script other than the body's was dropped, so a Chinese report lost its English section titles; a kana-majority Japanese document had every detected heading discarded; a mostly-landscape document picked its title from page one without the body-paragraph check the portrait path applies, so a slide deck's title became slide one's body text. These are Scholar's precision-over-recall scope limits for an index of Latin and CJK papers; on PageIndex's default local mode they were silent refusals and silent losses. They are deleted here, in this repo's copy of the port, together with the Cyrillic-only density threshold; the private scholar/ tree stays a faithful port and the new tests guard the fork. With the rules gone the same layout yields the same three headings in Japanese, Hindi, Arabic, Hebrew, Thai and mixed Chinese/English, and English is unchanged. Language now only decides which cues are available: case, keyword tables, numbering styles. The page fallback emitted a node only for pages carrying text, each spanning one page, so a partly-OCR'd 20-page PDF indexed as six islands with fourteen pages in no node's range and unreachable by retrieval, accepted without an error. Titles came from the page's first raw line, on a real document the running header or the page number. Every page is now a node titled "Page N", FLAT_TREE_MAX_NODES bounds document pages, and toc_source="unreadable" means exactly that no page carries text. The refusal says what was measured, no text layer, run OCR, instead of "no alphabetic text ... try standard mode", which asserted a cause never checked and pointed at a mode fed the same bytes. The non-Latin fixtures are PyMuPDF-generated with open-licensed font subsets embedded (FiraGO, Droid Sans Fallback); tests/data/flash/make_fixtures.py regenerates them byte-identically.
A ToUnicode value can be several code points: a Devanagari conjunct, a Thai cluster, an Arabic ligature. _rtl_sign passed the whole value to unicodedata.bidirectional, which takes exactly one character, so every real Hindi and Thai PDF raised TypeError in the char merge, before any rule ran. The first code point now decides the direction, as _reverse_if_rtl already does for the same values; the empty string is LTR.
…nodes as emitted The title scorer halved any candidate whose dominant script differed from the document's, the last place flash changed a decision on script. Without the factor the nine example PDFs and the four fixture PDFs extract byte-identically. get_leaf_nodes reads `nodes` with .get like its siblings, so a flat page tree, whose nodes carry no `nodes` key, walks instead of raising KeyError. The README output block and the page_index_flash docstring now say what a node actually carries: node_id always, `nodes` only with children, `summary` only when summaries ran, and a flat tree past FLAT_TREE_MAX_NODES pages coming back unsummarized and unoptimized. The local client's page-fallback test feeds that real node shape.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
…n standard mode Retrieval reaches a page only through a node range. When the first heading or the first bookmark sits on a later page, everything before it was in no node: a memo's first section once its heading became the document title, a title slide's body, a report's cover, contents and letter to shareholders, the cover pages of a bookmark tree. Standard mode has always inserted a Preface node for exactly this (utils.add_preface_if_needed); flash now does the same, next to the page fallback and before the optimize and summary passes, and renumbers the node ids. Of the nine example PDFs, the two Federal Reserve reports and Four Lectures gain the node (pages 1-4, 1-2 and 1); the other six and every extract_toc result are unchanged. The four fixtures gain a Preface over their title page.
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
1 participant
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.
Review-only view of #502 (Flash: layout decides, never script; the page fallback covers every page) plus every follow-up on
flash-page-fallback, diffed againstreview/base-fc2dc41, main frozen at the commit #502 branches from.Never merge. The base must never advance; merging would only pollute it. Later review rounds run on this PR; fixes found here push to
flash-page-fallback(this diff updates) and reachmainthrough their own small PRs.Same pattern as #488 (v0.2.15), #461 (v0.2.14) and #457 / #458 (v0.2.13).