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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

MeanAveragePrecision: Skip box conversion if no boxes are present #1097

Merged
merged 6 commits into from Jun 20, 2022

Conversation

kouyk
Copy link
Contributor

@kouyk kouyk commented Jun 18, 2022

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.

What does this PR do?

Fixes #1096

Before submitting

  • Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure to update the docs?
  • Did you write any new necessary tests?

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 馃檭

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
Copy link

codecov bot commented Jun 18, 2022

Codecov Report

Merging #1097 (1c1f12f) into master (203ab6b) will decrease coverage by 32%.
The diff coverage is 100%.

@@           Coverage Diff            @@
##           master   #1097     +/-   ##
========================================
- Coverage      71%     39%    -32%     
========================================
  Files         180     180             
  Lines        8078    8079      +1     
========================================
- Hits         5735    3121   -2614     
- Misses       2343    4958   +2615     

@Borda Borda added the bug / fix Something isn't working label Jun 19, 2022
Borda
Borda previously approved these changes Jun 19, 2022
@Borda Borda added this to the v0.9 milestone Jun 19, 2022
@mergify mergify bot added the ready label Jun 19, 2022
Copy link
Member

@justusschock justusschock left a 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?

@Borda
Copy link
Member

Borda commented Jun 20, 2022

@SkafteNicki @twsl @tkupek pls check ^^ 馃惏

@Borda Borda enabled auto-merge (squash) June 20, 2022 09:12
@kouyk
Copy link
Contributor Author

kouyk commented Jun 20, 2022

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?

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 update only stores the boxes and does no computation of the metric, hence other than ensuring that the conversion process is correct, there shouldn't be anything to verify at this step.

Even for the current tests, it is somewhat redundant to test for both empty ground truth and prediction, since the problem lies in _get_safe_item_values that is applicable to both.

@Borda Borda requested review from Borda and justusschock June 20, 2022 09:24
@Borda Borda dismissed their stale review June 20, 2022 09:24

revisiting

@mergify mergify bot removed the ready label Jun 20, 2022
@justusschock
Copy link
Member

what I was asking for is, if there are no boxes in any update step, what should be the output of compute?

@kouyk
Copy link
Contributor Author

kouyk commented Jun 20, 2022

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.

Copy link
Contributor

@tkupek tkupek left a 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

@mergify mergify bot added the ready label Jun 20, 2022
@Borda Borda merged commit d29919c into Lightning-AI:master Jun 20, 2022
@kouyk kouyk deleted the map-empty-boxes-fix branch June 20, 2022 12:06
Borda pushed a commit that referenced this pull request Jun 20, 2022
)

* 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug / fix Something isn't working ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MeanAveragePrecision does not handle empty predictions or ground truth boxes not in xyxy format
5 participants