-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Add get_image for all DocItem, specialize for FloatingItem #67
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sh-gupta @cau-git
- Need to be backwards compatible within docling-core 2.x.y, e.g. new field with default value, and
- then imply a new minor version bump of the DoclingDocument as well as potentially DocMeta which also depends on the DocItem.
@sh-gupta We quickly discussed, we need to do the following points (@cau-git will explain),
|
Signed-off-by: Shubham Gupta <[email protected]>
This reverts commit e48cd47. Signed-off-by: Shubham Gupta <[email protected]>
Signed-off-by: Shubham Gupta <[email protected]>
66160ba
to
924b974
Compare
Made the required changes:
|
This looks good now, thanks! To move on, you best create a companion PR on main |
@sh-gupta Could we add some unit-tests for this functionality? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
I'm wondering if we want to raise an exception instead of returning None.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very nice!
Signed-off-by: Shubham Gupta <[email protected]>
f4e5acc
@PeterStaar-IBM : I've added tests for @dolfim-ibm : I initially thought raising an error but then decided to stick with @vagenas : I've made the changes as per your discussion. Can you please take a look? |
We want the
image
attribute to be available forTextItem
to allow exposing equation images. I have movedimage
fromFloatingItem
toDocItem
so that it becomes part ofTextItem
as well.Related issue: DS4SD/docling#299