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

added micro average option for torch metrics #874

Merged
merged 23 commits into from
May 25, 2022

Conversation

razmikmelikbekyan
Copy link
Contributor

@razmikmelikbekyan razmikmelikbekyan commented Mar 6, 2022

What does this PR do?

Added Micro average option for IoU.

Fixes #826

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 🙃

@codecov
Copy link

codecov bot commented Mar 6, 2022

Codecov Report

Merging #874 (1008e88) into master (567f0ea) will increase coverage by 15%.
The diff coverage is 96%.

❗ Current head 1008e88 differs from pull request most recent head 5435cec. Consider uploading reports for the commit 5435cec to get more accurate results

@@           Coverage Diff           @@
##           master   #874     +/-   ##
=======================================
+ Coverage      71%    86%    +15%     
=======================================
  Files         181    181             
  Lines        7993   8004     +11     
=======================================
+ Hits         5681   6895   +1214     
+ Misses       2312   1109   -1203     

@razmikmelikbekyan
Copy link
Contributor Author

@Borda Please have a look. After initial review, I will remove all the commented-out lines, but there might be some incompatibility related issues since I was going to remove the reduction parameter fully.

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.

implementation-wise it looks good to me. Just not sure how to proceed with different arguments for different metrics. IMO it should all be called reduction, but not sure how to handle different arguments. Will we have separate helper functions or just one function that does it all and some others just for input validation?

tests/classification/test_jaccard.py Show resolved Hide resolved
@Borda Borda added the enhancement New feature or request label Mar 7, 2022
@Borda Borda changed the title ENH: added micro average option for torch metrics added micro average option for torch metrics Mar 20, 2022
@mergify mergify bot removed the has conflicts label Mar 20, 2022
@Borda Borda added this to the v0.8 milestone Mar 22, 2022
@Borda Borda requested review from awaelchli and removed request for ananyahjha93 March 22, 2022 16:42
@Borda
Copy link
Member

Borda commented Mar 24, 2022

@razmikmelikbekyan how is it going here? 🐰

@razmikmelikbekyan
Copy link
Contributor Author

razmikmelikbekyan commented Mar 24, 2022

@razmikmelikbekyan how is it going here? 🐰

@Borda Honestly said I am a bit confused about the next steps. I am not sure that I will have enough time right now to refactor all the other metrics and so on and I do not understand what exactly needs to be done in this particular PR.

Should we merge it or we want to change name back to reduction and so one (in my opinion name average_type is more relevant and adapted by the community for this kind of metrics than reduction)

@Borda
Copy link
Member

Borda commented Mar 25, 2022

Honestly said I am a bit confused about the next steps.

@stancld mind help with this PR? :)

@mergify mergify bot removed the has conflicts label May 5, 2022
@justusschock
Copy link
Member

Hey @razmikmelikbekyan ,

Very sorry for the long return time. The decisions made offline with @Borda and @SkafteNicki were that we want to get this to merge now and that we will deal with the naming inconsistencies while doing the major refactor (#1001 ).
That being said, do you think, there is a chance that you could look into the failing tests?

@razmikmelikbekyan
Copy link
Contributor Author

Hey @razmikmelikbekyan ,

Very sorry for the long return time. The decisions made offline with @Borda and @SkafteNicki were that we want to get this to merge now and that we will deal with the naming inconsistencies while doing the major refactor (#1001 ). That being said, do you think, there is a chance that you could look into the failing tests?

hey @justusschock, yep, I will have a look soon.

@Borda Borda requested a review from justusschock May 25, 2022 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Micro Average IoU
6 participants