fix(tokens): normalize inverted bbox in get_location instead of asserting - #676
fix(tokens): normalize inverted bbox in get_location instead of asserting#676maxtaran2010 wants to merge 2 commits into
Conversation
|
✅ DCO Check Passed Thanks @maxtaran2010, all your commits are properly signed off. 🎉 |
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/
|
There was a problem hiding this comment.
Pull request overview
This PR updates DocumentToken.get_location() to handle slightly inverted bounding boxes by normalizing coordinate order instead of asserting, preventing crashes during DocTags export for near-degenerate layout elements (Issue #620).
Changes:
- Normalize potentially inverted
bboxcoordinates usingsorted()and remove the crash-causing assertions. - Add unit tests covering normal, partially inverted (issue repro), and fully inverted bounding boxes.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| docling_core/types/doc/tokens.py | Normalizes inverted bbox coordinates before computing location tokens, avoiding asserts/crashes. |
| test/test_tokens.py | Adds regression/unit tests to ensure inverted bboxes serialize consistently and don’t crash. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -303,13 +303,19 @@ def get_location( | |||
| self_closing: bool = False, | |||
| ): | |||
| """Get the location string give bbox and page-dim.""" | |||
Addresses review feedback on docling-project#676. Signed-off-by: max <87073104+maxtaran2010@users.noreply.github.com>
…ting Near-degenerate elements (e.g. a thin rule whose layout-model regression produced a slightly inverted bbox) could arrive with bbox[0] > bbox[2] or bbox[1] > bbox[3]. The redundant asserts in DocumentToken.get_location crashed export_to_doctags(add_location=True) on such boxes, even though the method already normalizes ordering via min()/max() downstream. This bites long documents in particular, where the odds of one degenerate element grow with page count. Sort the coordinates before normalization to mirror the existing min()/max() behaviour, and drop the asserts. Adds regression tests for normal, partially inverted (from the issue repro), and fully inverted bboxes. Fixes docling-project#620 Signed-off-by: max <87073104+maxtaran2010@users.noreply.github.com>
Addresses review feedback on docling-project#676. Signed-off-by: max <87073104+maxtaran2010@users.noreply.github.com>
7f5d4c1 to
6cade8c
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
ceberam
left a comment
There was a problem hiding this comment.
Thanks @maxtaran2010 for this contribution. The fix is correct and the change itself is clean and minimal.
A couple of comments:
- However, after the sort,
left <= rightandtop <= bottomis guaranteed, so dividing by positive page dimensions preserves that:x0 <= x1andy0 <= y1. The four calls on lines 320–323 can now be written withoutmin()ormax(). Could you please simplify those lines? - The inline comment on
get_locationis very informative for users to understand the eventual normalization. Could you please add the comment to the function docstring instead of the inline comments?
Fixes #620
Problem
`DocumentToken.get_location()` in `docling_core/types/doc/tokens.py` asserted `bbox[0] <= bbox[2]` and `bbox[1] <= bbox[3]`. Near-degenerate elements (e.g. a thin decorative rule whose layout-model regression produces a slightly inverted bbox) can violate this by a fraction of a point, which crashes `export_to_doctags(add_location=True)`:
The asserts were redundant: the method already emits correctly-ordered location tokens via `min()`/`max()` downstream. The probability of hitting one such element grows with page count, so long documents are the most affected.
Fix
Sort the coordinates before normalization (mirroring the existing `min()`/`max()` behaviour) and drop the asserts. Output is unchanged for well-formed boxes and now correct instead of crashing for inverted ones.
Tests
Added `test/test_tokens.py` covering:
All pass; existing `test_serialization_doctag.py` and `test_doctags_load.py` (17 tests) still pass, and `ruff` is clean.