Skip to content
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

fix(layout_utils): correct conditional logic in adapt_bbox function #416

Closed

Conversation

Raphilanthrope
Copy link

@Raphilanthrope Raphilanthrope commented Nov 22, 2024

The second 'if' statement in the adapt_bbox function was changed to 'elif'
to properly handle the conditional flow for different DocItemLabel types.

This change ensures that the function correctly processes TABLE, PICTURE,
and other DocItemLabel types as intended.

Resolves: #362

Checklist:

  • Commit Message Formatting: Commit titles and messages follow guidelines in the
    conventional commits.
  • Documentation has been updated, if necessary.
  • Examples have been added, if necessary.
  • Tests have been added, if necessary.

See issue raised in DS4SD#362 (comment)

Signed-off-by: Raphaël M.J.I. Larsen <39514032+Raphilanthrope@users.noreply.github.com>
Copy link

mergify bot commented Nov 22, 2024

Merge Protections

Your pull request matches the following merge protections and will not be merged until they are valid.

🟢 Enforce conventional commit

Wonderful, this rule succeeded.

Make sure that we follow https://www.conventionalcommits.org/en/v1.0.0/

  • title ~= ^(fix|feat|docs|style|refactor|perf|test|build|ci|chore|revert)(?:\(.+\))?:

@dolfim-ibm dolfim-ibm requested a review from cau-git November 25, 2024 08:39
@cau-git
Copy link
Contributor

cau-git commented Nov 25, 2024

@Raphilanthrope I can not find any logical difference between the original code and your proposal.
Do you have a practical case where this change makes a difference? If yes, please post a PDF and instructions to reproduce the issue.

@Raphilanthrope
Copy link
Author

@Raphilanthrope I can not find any logical difference between the original code and your proposal. Do you have a practical case where this change makes a difference? If yes, please post a PDF and instructions to reproduce the issue.

I don't have a practical example where this change induces a difference.

In fact, it might be the case that it does not induce any difference if the label of the object given to adapt_bbox is restricted to [TABLE, PICTURE], otherwise if the label is authorized to be different than these two labels, when it is, both the first 'if' and the 'else' clauses are treated whereas only the first clause is treated when the second 'if' is replaces by 'elif'.

In summary, the question is: Is the label of the object given to adapt_bbox restricted to [TABLE, PICTURE] ?
If not, the PR would ensure the wanted behavior suggested by the comment after the 'else' keyword (that indicates that the object is expected to be a table).

@PeterStaar-IBM
Copy link
Contributor

@cau-git please update

@cau-git
Copy link
Contributor

cau-git commented Dec 2, 2024

@Raphilanthrope I am closing this because it will be obsolete with an update I am making to the layout postprocessing.

@cau-git cau-git closed this Dec 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Should the second "if" keyword in adapt_bbox from layout_utils.py rather be an "elif" keyword ?
3 participants