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

Don't raise exception for empty intersection (fix issue #139) #141

Closed
wants to merge 1 commit into from

Conversation

stweil
Copy link
Contributor

@stweil stweil commented Aug 15, 2020

Signed-off-by: Stefan Weil [email protected]

@codecov
Copy link

codecov bot commented Aug 15, 2020

Codecov Report

Merging #141 into master will decrease coverage by 0.03%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #141      +/-   ##
==========================================
- Coverage   37.77%   37.73%   -0.04%     
==========================================
  Files           9        9              
  Lines         998      999       +1     
  Branches      212      212              
==========================================
  Hits          377      377              
- Misses        555      556       +1     
  Partials       66       66              
Impacted Files Coverage Δ
ocrd_tesserocr/segment_region.py 53.28% <0.00%> (-0.36%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a370312...bff7dcc. Read the comment docs.

@kba
Copy link
Member

kba commented Aug 16, 2020

But if the child element is outside the parent element, that is an error that should be adressed. I agree that raising an exception shifts the responsibility for debugging to the user, that is not ideal. But this should be debugged nonetheless.

@stweil
Copy link
Contributor Author

stweil commented Aug 16, 2020

But this should be debugged nonetheless.

Sure, that's why I added LOG.error. But you are right, this is not a fix but a workaround, so issue #139 should be kept open even with this commit.

@stweil
Copy link
Contributor Author

stweil commented Aug 16, 2020

But if the child element is outside the parent element, that is an error that should be adressed.

Maybe both parent and child are empty, then the child is not outside, but the same error is triggered.

@kba kba requested a review from bertsky August 16, 2020 11:16
@bertsky
Copy link
Collaborator

bertsky commented Aug 16, 2020

Introducing this exception was a conscious decision for the current state of coordinate consistency in OCR-D.

We know that most processors currently write illegal coordinates sometimes, which ultimately fails at validation time with lots of hard to debug or trace or visualise error messages. But shifting the burden for correct coordinates from validation to implementation is a large effort, and we need core to support this as much as possible. That's where this idea will step in.

But looking at the example in #139 I can now see that this will happen much more frequently than I thought in ocrd_tesserocr. So yes, we need to offer some workaround as well. I'll have to think about which is preferable: fixing this instance of #139 directly or downgrading to an error message...

@stweil
Copy link
Contributor Author

stweil commented Sep 4, 2020

Issue #139 is fixed now with a better solution, so closing this pull request.

@stweil stweil closed this Sep 4, 2020
@stweil stweil deleted the issue139 branch September 4, 2020 19:37
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.

3 participants