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 ignore_index in the computation of IoU #328

Merged
merged 12 commits into from
Jul 29, 2021
Merged

fix ignore_index in the computation of IoU #328

merged 12 commits into from
Jul 29, 2021

Conversation

CSautier
Copy link
Contributor

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?

What does this PR do?

Fixes #312

PR review

The way it does is to remove the line and columns of the ignore_index in the confmat. It should work whenever ignore_index is in the correct range but could be tested further.

There are certainly cleaner ways to do this.

@codecov
Copy link

codecov bot commented Jun 29, 2021

Codecov Report

Merging #328 (32389fb) into master (85ebc3b) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #328   +/-   ##
=======================================
  Coverage   96.08%   96.08%           
=======================================
  Files         124      124           
  Lines        3980     3982    +2     
=======================================
+ Hits         3824     3826    +2     
  Misses        156      156           
Flag Coverage Δ
Linux 75.69% <50.00%> (-0.02%) ⬇️
Windows 75.69% <50.00%> (-0.02%) ⬇️
cpu 96.08% <100.00%> (+<0.01%) ⬆️
macOS 96.08% <100.00%> (+<0.01%) ⬆️
pytest 96.08% <100.00%> (+<0.01%) ⬆️
python3.6 95.30% <100.00%> (+<0.01%) ⬆️
python3.8 96.08% <100.00%> (+<0.01%) ⬆️
python3.9 95.98% <100.00%> (+<0.01%) ⬆️
torch1.3.1 95.30% <100.00%> (+<0.01%) ⬆️
torch1.4.0 95.37% <100.00%> (+<0.01%) ⬆️
torch1.9.0 95.98% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
torchmetrics/functional/classification/iou.py 100.00% <100.00%> (ø)

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 85ebc3b...32389fb. Read the comment docs.

@Borda Borda added the bug / fix Something isn't working label Jun 29, 2021
Copy link
Member

@Borda Borda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice, can we add a test to verify it works as intended now?

@mergify mergify bot removed the has conflicts label Jul 7, 2021
@Borda
Copy link
Member

Borda commented Jul 7, 2021

@CSautier mind check the failing tests?

@Borda
Copy link
Member

Borda commented Jul 26, 2021

@CSautier how is it going? 🐰

@CSautier
Copy link
Contributor Author

CSautier commented Jul 26, 2021

Sorry, it slipped out of my mind.
I verified the tests, and I believe test_iou(True, 'none', 0, torch.Tensor([0.5, 0.5])) does not make sense. I think the result should indeed be (2/3, 1/2) instead of (1/2, 1/2), which would mean that the current version passing this test is wrong. Similarly test_iou_ignore_index([0, 1, 1, 2, 2], [0, 1, 2, 2, 2], 2, 3, 'none', [1, 1 / 2]) should indeed return (1,1) as my version does and not (1, 1/2) as the test expects.

However there is one correct tests that my computation does not pass
test_iou_ignore_index([0, 1, 1, 2, 2], [0, 1, 2, 2, 2], 1, 3, 'none', [1, 2 / 3])
and it shows a problem in how I remove the ignore_index if it is predicted.

I'm working on it now, and will change my PR if I find a solution.
If you could also verify that I'm correct and test_iou(True, 'none', 0, torch.Tensor([0.5, 0.5])) should indeed return (2/3, 1/2) and test_iou_ignore_index([0, 1, 1, 2, 2], [0, 1, 2, 2, 2], 2, 3, 'none', [1, 1 / 2]) should in fact return (1, 1) that would be great.

@pep8speaks
Copy link

pep8speaks commented Jul 26, 2021

Hello @CSautier! Thanks for updating this PR.

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-07-29 16:03:10 UTC

@CSautier
Copy link
Contributor Author

I've updated the PR, and included the two modifications I mentioned earlier in the tests.

Copy link
Member

@SkafteNicki SkafteNicki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM :]

@SkafteNicki SkafteNicki enabled auto-merge (squash) July 29, 2021 15:21
@SkafteNicki SkafteNicki merged commit c9d36b2 into Lightning-AI:master Jul 29, 2021
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.

Incorrect results with ignore_index in IoU
4 participants