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 IoU score for classes not present in target or pred #3098

Merged
merged 17 commits into from
Sep 17, 2020

Conversation

abrahambotros
Copy link
Contributor

@abrahambotros abrahambotros commented Aug 21, 2020

What does this PR do?

Fixes #3097
Fixes #2736

  • Allow configurable not_present_score for IoU for classes not present in target or pred. Defaults to 1.0.
  • Also allow passing num_classes parameter through from iou metric class down to its underlying functional iou call.

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 your PR does only one thing, instead of bundling different changes together? Otherwise, we ask you to create a separate PR for every change.
  • Did you make sure to update the documentation with your changes?
  • Did you write any new necessary tests?
  • Did you verify new and existing tests pass locally with your changes?
  • If you made a notable change (that affects users), did you update the CHANGELOG?

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 🙃

@codecov
Copy link

codecov bot commented Aug 22, 2020

Codecov Report

Merging #3098 into master will increase coverage by 5%.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master   #3098    +/-   ##
=======================================
+ Coverage      86%     91%    +5%     
=======================================
  Files         109     107     -2     
  Lines        8031    8034     +3     
=======================================
+ Hits         6904    7300   +396     
+ Misses       1127     734   -393     

@Borda Borda added the bug Something isn't working label Aug 24, 2020
@Borda Borda changed the title [DRAFT] Fix IoU score for classes not present in target or pred [wip] Fix IoU score for classes not present in target or pred Aug 24, 2020
@Borda Borda marked this pull request as draft August 24, 2020 07:30
@Borda Borda added this to the 0.9.x milestone Aug 24, 2020
@SkafteNicki
Copy link
Member

@abrahambotros how is it going here? I the PR still WIP?

@abrahambotros
Copy link
Contributor Author

@SkafteNicki from my side, this is ready for review. However, it's still marked as WIP/draft since I haven't gotten feedback / confirmation on the related issue (#3097).

Not sure of the etiquette for this repo, but in case it's just in limbo here, I'll remove the [wip] and draft tags now even without the issue feedback. Any mods can feel free to let me know otherwise.

@abrahambotros abrahambotros marked this pull request as ready for review September 1, 2020 23:28
@abrahambotros abrahambotros changed the title [wip] Fix IoU score for classes not present in target or pred Fix IoU score for classes not present in target or pred Sep 1, 2020
@mergify
Copy link
Contributor

mergify bot commented Sep 1, 2020

This pull request is now in conflict... :(

@abrahambotros
Copy link
Contributor Author

@SkafteNicki I also rebased on master and added this change to the changelog, requiring the force push above. Still ready for review.

CHANGELOG.md Outdated Show resolved Hide resolved
pytorch_lightning/metrics/functional/classification.py Outdated Show resolved Hide resolved
@mergify mergify bot requested a review from a team September 2, 2020 07:31
@SkafteNicki
Copy link
Member

@abrahambotros PR is looking good. One question:
After looking at issues, I can see that issue #2736 is also related to IoU metric. I was wondering if you would be able to fix this also (now that we are doing changed to the metric). Basically it involves renaming the remove_bg to ignore_index and let it be int (or list of int) instead of bool. Then you would need to skip theses classes in the for-loop when doing the per class calculation?
Please say no if you don't want to, and I will move forward with approving this PR.

@pep8speaks
Copy link

pep8speaks commented Sep 3, 2020

Hello @abrahambotros! Thanks for updating this PR.

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

Comment last updated at 2020-09-15 20:33:29 UTC

@abrahambotros abrahambotros force-pushed the iou-not-present-fixes branch 2 times, most recently from 67439be to 0d5516c Compare September 3, 2020 19:34
@abrahambotros
Copy link
Contributor Author

@SkafteNicki that sounded like a reasonable addition here; added that change in 0d5516c

@mergify
Copy link
Contributor

mergify bot commented Sep 3, 2020

This pull request is now in conflict... :(

@abrahambotros
Copy link
Contributor Author

Ah, looks like there's a new merge conflict in the changelog; I should include the ignore_index change in the changelog as well anyway, which I forgot to do. Will push a change soon.

@abrahambotros abrahambotros force-pushed the iou-not-present-fixes branch 2 times, most recently from 295d4b2 to a2576e1 Compare September 4, 2020 01:18
@abrahambotros
Copy link
Contributor Author

abrahambotros commented Sep 4, 2020

@SkafteNicki changes done and ready for final review.

A few of the CI tests apparently were cancelled, and looks like PRs that were merged today also had some skipped; nothing seems related to this PR, but let me know if I need to change anything else.

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

Copy link
Contributor

@rohitgr7 rohitgr7 left a comment

Choose a reason for hiding this comment

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

There is a failing test for IOU. @abrahambotros can you check?

@mergify mergify bot requested a review from a team September 15, 2020 18:59
Use macro instead of micro averaging in sklearn's jaccard score, to
match multi-class IoU, which conventionally takes per-class scores
before averaging.
@abrahambotros
Copy link
Contributor Author

@rohitgr7 should be fixed now via 42ef161 . That sklearn Jaccard test was broken when rebasing earlier today on #3322, since I mistakenly copied the new micro averaging usage instead of sticking with macro for Jaccard.

@rohitgr7
Copy link
Contributor

@abrahambotros I am not familiar with IoU that much, but does it require new reduction methods just like in other classification metrics? Same for dice_score?

@abrahambotros
Copy link
Contributor Author

@rohitgr7 good question to discuss. My understanding is that IoU (and I believe dice score as well) is always computed per-class (analogous to macro for Jaccard in sklearn), and therefore would not require any changes here.

I think micro (calculating metrics globally; the new default in the other metrics as of #3322) would not make sense here. I might be wrong, but micro in IoU world might actually devolve to just accuracy, since the intersection is all TPs, and the union is all cases?

weighted could be a valid addition in theory, but I think in practice would not be used? People often use IoU to avoid issues where a (background) class dominates the pixels of an image, and you explicitly don't want to weigh that class highly against others. In general, you want each class to have good representation in your final score by measuring each class against its own total area (its union), regardless of how many examples of that class were actually in your example/dataset. Maybe an exception would be if someone wanted to pass in an explicit weighting to use for each class, but they could probably just use reduction=none and do this weighting themselves in this case, or we could support doing it internally if enough people ask.

I could be wrong on some of these points though. Maybe @SkafteNicki or others have some input/corrections?

@SkafteNicki
Copy link
Member

@abrahambotros I agree with your analysis.
We can consider IoU to be a special case of the jaccard score (similar to how accuracy is also a special case). Thus, I would not introduce an class_reduce argument in this metric as IoU have a very strict definition (calculate for each class and take the average aka macro).

@abrahambotros
Copy link
Contributor Author

Thanks @SkafteNicki !

@SkafteNicki
Copy link
Member

@rohitgr7 if you are happy with changes made, will you go ahead and approve so we can get this merged?

Copy link
Contributor

@rohitgr7 rohitgr7 left a comment

Choose a reason for hiding this comment

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

Nice work :)

@Borda
Copy link
Member

Borda commented Sep 16, 2020

@justusschock mind check last version...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ready PRs ready to be merged
Projects
None yet
7 participants