-
Notifications
You must be signed in to change notification settings - Fork 415
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
MeanAveragePrecision: Skip box conversion if no boxes are present #1097
Conversation
The `box_convert` function from torchvision expects the input to be a Tensor[N, 4], where N > 0. Should N == 0 and in_fmt != out_fmt, `unbind` will error out on the boxes tensor during the conversion process. The workaround is therefore to skip the box conversion if boxes is an empty tensor.
Codecov Report
@@ Coverage Diff @@
## master #1097 +/- ##
========================================
- Coverage 71% 39% -32%
========================================
Files 180 180
Lines 8078 8079 +1
========================================
- Hits 5735 3121 -2614
- Misses 2343 4958 +2615 |
for more information, see https://pre-commit.ci
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.
Programming-Wise it looks fine to me.
What is the expected outcome if there are no boxes at all? Is there a test for that?
Also, Could you add a changelog entry?
@SkafteNicki @twsl @tkupek pls check ^^ 🐰 |
Should there be no boxes for both predictions and ground truth, the outcome is the same as any other case, i.e. no crashes. My understanding is that Even for the current tests, it is somewhat redundant to test for both empty ground truth and prediction, since the problem lies in |
what I was asking for is, if there are no boxes in any update step, what should be the output of compute? |
It depends on the scenario, it could be considered false positive (empty ground truth) or false negative (empty predictions) and affect the calculation that way. If there are totally no boxes in any given update step (or more specifically any pair of ground truth and prediction boxes), the result of compute should be exactly the same as without them based on my understanding of the mAP metric definition. That aside, I think #884 already addressed the case where there are no ground truth or prediction boxes, the bug in question is only present when using box formats other than xyxy and requires a conversion, it doesn't change the existing behaviour since empty boxes can already be provided in xyxy format. |
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.
Fully agree with @kouyk, this does not change the behavior, only adds a failsafe for empty boxes.
Nice tests, thanks for taking care!
LGTM
) * Skip box conversion if no boxes are present The `box_convert` function from torchvision expects the input to be a Tensor[N, 4], where N > 0. Should N == 0 and in_fmt != out_fmt, `unbind` will error out on the boxes tensor during the conversion process. The workaround is, therefore to skip the box conversion if boxes is an empty tensor. Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Jirka <jirka.borovec@seznam.cz> (cherry picked from commit d29919c)
The
box_convert
function from torchvision expects the input to be aTensor[N, 4], where N > 0. Should N == 0 and in_fmt != out_fmt,
unbind
will error out on the boxes tensor during the conversion process.
The workaround is therefore to skip the box conversion if boxes is an
empty tensor.
What does this PR do?
Fixes #1096
Before submitting
PR review
Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.
Did you have fun?
Make sure you had fun coding 🙃