fix(layout): drop FORM/KEY_VALUE_REGION clusters that swallow tables#3789
fix(layout): drop FORM/KEY_VALUE_REGION clusters that swallow tables#3789pablopupo wants to merge 1 commit into
Conversation
FORM and TABLE share the wrapper bucket in LayoutPostprocessor, so Union-Find merges a large low-confidence FORM with the smaller, higher-confidence TABLE clusters it overlaps, and the group's winner is picked mostly by area. The FORM always won, and every TABLE it wrapped collapsed into a flat FORM_AREA group, losing its TableItem structure. _handle_cross_type_overlaps now drops a FORM or KEY_VALUE_REGION cluster that substantially contains (>0.8) a TABLE or DOCUMENT_INDEX cluster before the wrapper bucket is de-overlapped, mirroring the PICTURE-vs-TABLE fix from docling-project#3523. This also subsumes a dead branch in the same function that looped over self.regular_clusters looking for a TABLE label, which can never match since TABLE is a SPECIAL_TYPE excluded from regular_clusters by construction. Adds a fixture from the issue's repro plus unit tests, and regenerates groundtruth for right_to_left_03.pdf, an existing fixture that turned out to hit the same bug (a key-value table was flattened into loose text). Resolves docling-project#3454 Signed-off-by: Pablo Pupo <pablofpupo23@gmail.com>
|
✅ DCO Check Passed Thanks @pablopupo, 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/
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
@pablopupo Thanks, that is an important finding. In fact, FORM and KEY_VALUE_REGION clusters are not supposed to compete in the standard overlap removal detection with tables or pictures. TABLE clusters and PICTURE clusters should be nested as children in forms as long as they are not nearly identical in shape. Your current code proposal drops FORM or KEY_VALUE_REGION in favour of tables, which seems unnecessarily destructive. Could you please make an attempt to preserve them and create the proper child nesting behaviour with this regard? |
FORM and TABLE clusters share the wrapper bucket in
LayoutPostprocessor, so when the layout model proposes a large low-confidence FORM region wrapping several higher-confidence TABLE regions, Union-Find in_remove_overlapping_clustersmerges them into one group, and the group's winner is picked mostly by area (_should_prefer_clusterskips its confidence check once the area ratio passes 2.0). The FORM wins every time, and the tables it wrapped collapse into a flatFORM_AREAgroup, never reaching TableFormer. On the scanned form in #3454, that's three TABLE clusters at confidence 0.877/0.872/0.795 losing to a FORM at 0.651 purely because it had the larger bbox._handle_cross_type_overlapsnow drops a FORM or KEY_VALUE_REGION cluster that substantially contains (intersection_over_self > 0.8) a TABLE or DOCUMENT_INDEX cluster, before the wrapper bucket gets de-overlapped, mirroring the PICTURE-vs-TABLE fix in #3523. I tried a simpler per-group priority rule first, but Union-Find only keeps one winner per group, so it still lost 2 of the 3 tables on this page; removing the FORM outright is what lets every table it wrapped survive. This also cleans up a dead branch in the same function that looped overself.regular_clusterschecking for a TABLE label, which can never match since TABLE is aSPECIAL_TYPEand is excluded fromregular_clustersby construction.While testing against the existing PDF fixtures I found
right_to_left_03.pdfwas hitting the same bug (a key-value table there was flattened into 16 loose text/list items instead of an 8-row table), so its groundtruth is regenerated here too.One open question, the same one I raised on #3753. Is 0.8 the right containment threshold here, or just a reasonable-looking default carried over from the existing
_process_special_clusterschild-containment check? I kept it consistent with what's already in the file, but happy to tune it if there's a more principled number.Resolves #3454
Checklist: