[rotation] Fix handling of bounding boxes in predictor when expand=True - Githubissues

mindee / doctr

docTR (Document Text Recognition) - a seamless, high-performing & accessible library for OCR-related tasks powered by Deep Learning.

https://mindee.github.io/doctr/

Apache License 2.0

3.85k stars 443 forks source link

[rotation] Fix handling of bounding boxes in predictor when expand=True #443

Closed fg-mindee closed 2 years ago

fg-mindee commented 3 years ago

In our current OCR predictor, there is a potential issue with rotation at the detection stage:

by default, expand=False in rotations: this means that a segment at the very top right end of the image, if rotated 45°, would go out of bounds and won't become a box candidate. Here we miss potential detections
if we simply switch to expand=True: some extra care needs to be taken to rotate and reproject bounding boxes relatively to the initial image (and with relative coords). As of now, making this switch would most likely cause a lot of problems.

This will require some heavy discussions to handle the matter properly in the long term.

fg-mindee commented 3 years ago

cf. #536

We still need to switch predictor to active expansion

charlesmindee commented 2 years ago

is that still an issue with #731 ?

fg-mindee commented 2 years ago

I was reviewing this issue actually, and I'm not 100% sure. I assume it does, but we need to test if expand=True works once #731 is merged to be safe :+1:

fg-mindee commented 2 years ago

@charlesmindee according to the last commits we just made on the main branch, this isn't an issue anymore I believe. Can you confirm it so we can close the last 0.5.0 issue related to rotation? :smiley:

charlesmindee commented 2 years ago

I am not sure to get your point here: expand=True is used only in the rotation transformation because we have to rotate the page, and it is working well in this transformation at training time, but at inference time in the predictor we don't rotate pages since we have now a rotation-aware detection model. To be sure, I checked everywhere in the predictors and it is impossible to pass the kwarg expand. @fg-mindee

fg-mindee commented 2 years ago

Could you specify;

do you mean that we won't need to pass expand=True at inference time in the predictor because the predictor behaviour & flags are already taking care of this?
or do you mean that we might need in the future to pass it, and it's impossible that this works as is?

In the first case, I believe we can close this issue :+1:

fg-mindee commented 2 years ago

ping @charlesmindee (I think it's the first option, but I want to be sure before closing the issue)

fg-mindee commented 2 years ago

Just to confirm @charlesmindee, we can close this right?

fg-mindee commented 2 years ago

ping @charlesmindee :pray:

felixdittrich92 commented 2 years ago

@charlesmindee ?

charlesmindee commented 2 years ago

Yes sorry we can close that!