Feat/refactor layoutelement textregion to vectorized data structure #3881
Conversation
This file contains 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
- initial refactor on tesseract ocr agent return as array instead of as a list
- more refactoring is required after the rest of data structure change is complete - specifically the algorithm in inference lib will need to be refactored to use vector math with vector data structures
- test original form should not have passed but because the function modified input it was passing - refactor the function to use new data structure removes the implicit modification of input
- inference library requires text regsions from pdfminer to be of either EmbeddedTextRegion or ImageTextRegion for class identification purposes - best way foward is to refactor the library to use layoutelements for both inferred and extracted (i.e., pdfminer) layouts for consistency then we can remove this temporary patch function in this commit
…lement-textregion-to-vectorized-data-structure
- for now convert output to list so we can reuse existing assertions - add todo note to refactor the checks to use array directly
…- Ingest test fixtures update (#3882) This pull request includes updated ingest test fixtures. Please review and merge if appropriate. Co-authored-by: badGarnet <[email protected]>
badGarnet
commented
Jan 22, 2025
Comment on lines
+71
to
+72
| "element_id": "a0c3c6b7e1e8c95016b989ef43c5ea2e", | ||
| "text": "2 For each dataset, we train several models of different sizes for different needs (the trade-off between accuracy vs. computational cost). For \u201cbase model\u201d and \u201clarge model\u201d, we refer to using the ResNet 50 or ResNet 101 backbones [13], respectively. One can train models of different architectures, like Faster R-CNN [28] (P) and Mask R-CNN [12] (M). For example, an F in the Large Model column indicates it has m Faster R-CNN model trained using the ResNet 101 backbone. The platform is maintained and a number of additions will be made to the model zoo in coming months.", |
There was a problem hiding this comment.
this is an improvement as a result of this PR (unintended but improvements are welcome)
…ta-structure' of github.com:Unstructured-IO/unstructured into feat/refactor-layoutelement-textregion-to-vectorized-data-structure
badGarnet
requested review from
christinestraub,
pawel-kmiecik,
ryannikolaidis,
MaksOpp and
cragwolfe
There was a problem hiding this comment.
Generally looks good!
I remember we had a discussion about using Data Frames (pandas or better - polars) but then the usage was not justified just for vectorized IoU.
Now I see that it could be easier to track the indeces of different numpy vector (series) and manipulate them if we used DFs. But - it's just a thing to think of, shouldn't block this refactor.
| assert result[1].bbox == Rectangle(20, 20, 30, 30) | ||
| def test_process_file_with_pdfminer(): | ||
| layout, links = process_file_with_pdfminer( | ||
| Path(__file__).parents[3] / "example-docs" / "pdf" / "layout-parser-paper-fast.pdf" |
There was a problem hiding this comment.
nit: Don't we have some fixture somewhere that delivers the example-docs path (could be scope=session)? If not I guess it's worth adding. Looks like being used in many tests.
| @@ -45,18 +46,79 @@ def process_file_with_pdfminer( | |||
| return extracted_layout, layouts_links | |||
|
|
|||
|
|
|||
| def _validate_bbox(bbox: list[int | float]) -> bool: | |||
| return all(x is not None for x in bbox) and ((bbox[2] - bbox[0]) * (bbox[3] - bbox[1]) > 0) | |||
There was a problem hiding this comment.
Is this check correct? What about:
bbox = (x1 = 3, y1 = 3, x2 = 2, y2 = 2) => (2-3) * (2-3) = -1 * -1 = 1 > 0
(but I haven't drunk my coffee yet so maybe I don't see something :D )
There was a problem hiding this comment.
Yeah, maybe:
bbox[0] < bbox[2] and # x1 < x2
bbox[1] < bbox[3] # y1 < y2?
…lement-textregion-to-vectorized-data-structure
badGarnet
deleted the
feat/refactor-layoutelement-textregion-to-vectorized-data-structure
branch
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.
This PR refactors the data structure for
list[LayoutElement]andlist[TextRegion]used in partition pdf/image files.numpyarray to store datapartitionfunction itself, i.e.,partitionstill returnslist[Element]list[LayoutElement]->LayoutElements;list[TextRegion]->TextRegionselement_array.as_list()call. This is the last step before turninglist[LayoutElement]intolist[Element]as returnlist[Element]fromLayoutElementsdata structure instead.The goal of this PR is to replace the data structure as much as possible without changing underlying logic. There are a few places where the slicing or filtering logic was simple enough to be converted into vector data structure operations. Those are refactored to be vector based. As a result there is some small improvements observed in ingest test. This is likely because the vector operations cleaned up some previous inconsistency in data types and operations.