feat(reading-order): re-enable same-row left-to-right linking with additive r2l fix#163
Open
aImErYbArRaUlT wants to merge 3 commits into
Open
Conversation
Signed-off-by: aImErYbArRaUlT <aimery@barratec.com>
Signed-off-by: aImErYbArRaUlT <aimery@barratec.com>
Signed-off-by: aImErYbArRaUlT <aimery@barratec.com>
Contributor
|
✅ DCO Check Passed Thanks @aImErYbArRaUlT, all your commits are properly signed off. 🎉 |
Contributor
Merge Protections🔴 1 of 2 protections blocking · waiting on 👀 reviews
🔴 Require two reviewer for test updatesWaiting for
This rule is failing.When test data is updated, we require two reviewers
Show 1 satisfied protection🟢 Enforce conventional commitMake sure that we follow https://www.conventionalcommits.org/en/v1.0.0/
|
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.
Addresses #162.
_init_l2r_map(the same-row left-to-right linking) has been guarded byFalsesince #44, with the note that it "leads to errors ... might be necessary in the future." This re-enables it and fixes the underlying issue the errors came from.What was wrong
With same-row linking off, an element on the same row as a right-hand neighbour (a right-aligned date next to a job title, or the right column of a two-column figure) never gets an edge into the reading-order graph. It ends up as an orphan node, and an orphan sorts to the end of the traversal. So same-row right elements get emitted last, detached from where they belong.
Why it isn't just a one-line re-enable
Flipping the guard back on exposes a second issue in the r2l branch of
_init_ud_maps: when an element is a same-row right partner, the code overwrites its up/down links with only the same-row link andcontinues, skipping the spatial search that finds its real vertical parents. So a caption sitting above the right column loses its link to the figure and orphans to the bottom. That's the regression the original "leads to errors" note was about.The fix (two commits)
follows_maintext_order(cid-adjacency) guard.continueso the spatial search still runs and the element keeps its real vertical parents.This preserves the model's count invariant (it returns exactly as many elements as it receives, none stranded). The symptom-level alternatives I tried either stranded elements or dropped edges, which violates that.
Evidence (dpbench, Spearman vs. ground truth)
Measured across three states to isolate each commit's effect:
Net: mean 0.9820 -> 0.9845, 6 docs improved, 0 regressions, 0 elements stranded, stable across 5 seeds, full suite passes.
Tests
Added a regression test for the caption-orphan case (
test_caption_not_orphaned_in_two_column_figure). It is a minimal hand-built fixture that reproduces the orphaning. It fails on the pre-fix behaviour (caption emitted last) and passes with the fix. Verified it actually bites: confirmed failing on the re-enable-only branch and passing on the full fix.Scope
This targets the same-row / right-aligned class only. It does not solve single-column heading inversion or general multi-column reading order. I tested SSG-42 directly and it's unchanged, since that page is single-column and the same-row branch never fires.
Checklist