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

Add argument average to MeanAveragePrecision #2018

Merged
merged 13 commits into from
Aug 28, 2023
Merged

Conversation

SkafteNicki
Copy link
Member

@SkafteNicki SkafteNicki commented Aug 23, 2023

What does this PR do?

Fixes #2004

Before submitting
  • Was this discussed/agreed 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 🙃


📚 Documentation preview 📚: https://torchmetrics--2018.org.readthedocs.build/en/2018/

@SkafteNicki SkafteNicki added the enhancement New feature or request label Aug 23, 2023
@SkafteNicki SkafteNicki added this to the v1.2.0 milestone Aug 23, 2023
@codecov
Copy link

codecov bot commented Aug 23, 2023

Codecov Report

Merging #2018 (75d6146) into master (eeb40e9) will decrease coverage by 49%.
Report is 1 commits behind head on master.
The diff coverage is 25%.

Additional details and impacted files
@@           Coverage Diff            @@
##           master   #2018     +/-   ##
========================================
- Coverage      87%     38%    -49%     
========================================
  Files         275     277      +2     
  Lines       15650   15733     +83     
========================================
- Hits        13567    5997   -7570     
- Misses       2083    9736   +7653     

@mergify mergify bot added ready and removed ready labels Aug 23, 2023
Copy link

@imatiach-msft imatiach-msft left a comment

Choose a reason for hiding this comment

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

Amazing!!!! Thank you!

Copy link

@Advitya17 Advitya17 left a comment

Choose a reason for hiding this comment

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

Thank you very much @SkafteNicki for such a quick turnaround on the feature request! Please let us know when torchmetrics may be released with this aggregate suport on MAP for object detection, and we can add the aggregate method back on our side on the RAI toolbox repo :)

src/torchmetrics/detection/mean_ap.py Show resolved Hide resolved
tests/unittests/detection/test_map.py Show resolved Hide resolved
@SkafteNicki
Copy link
Member Author

Thank you very much @SkafteNicki for such a quick turnaround on the feature request! Please let us know when torchmetrics may be released with this aggregate suport on MAP for object detection, and we can add the aggregate method back on our side on the RAI toolbox repo :)

@Advitya17 regarding release: so we just relased v1.1 of torchmetrics which this feature did not end up being part of. Therefore it may take some time before v1.2 is released. We do not really have fixed dates for released, but do it more dynamically based on when enough features have been added.

@mergify mergify bot removed the has conflicts label Aug 25, 2023
@Borda
Copy link
Member

Borda commented Aug 27, 2023

pls check the test_average_argument[False] for GPU tests...

@Borda Borda enabled auto-merge (squash) August 28, 2023 10:01
@Borda
Copy link
Member

Borda commented Aug 28, 2023

@SkafteNicki shall we include it in 1.1.x bug-fix release?

@SkafteNicki
Copy link
Member Author

@Borda fine by me. Do note it is not a bug-fix but a new feature as it has never been a feature of torchmetrics.

@Borda
Copy link
Member

Borda commented Aug 28, 2023

Do note it is not a bug-fix but a new feature as it has never been a feature of torchmetrics.

yes I know, but addressing an user issue 🦩

@Borda Borda merged commit 63c7bbe into master Aug 28, 2023
65 checks passed
@Borda Borda deleted the feature/map_average branch August 28, 2023 16:16
Borda pushed a commit that referenced this pull request Aug 28, 2023
matsumotosan pushed a commit to matsumotosan/metrics that referenced this pull request Sep 19, 2023
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.

Breaking change in MeanAveragePrecision by removing average param
5 participants