-
Notifications
You must be signed in to change notification settings - Fork 412
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
MAP metric, fix metric for CUDA execution #673
Conversation
Codecov Report
@@ Coverage Diff @@
## master #673 +/- ##
=====================================
Coverage 95% 95%
=====================================
Files 166 166
Lines 6377 6379 +2
=====================================
+ Hits 6070 6074 +4
+ Misses 307 305 -2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why wasn't this caught in our test? We do have CUDA tests
@justusschock Good question. I don't have a special setup. Just DDP mode with a single GPU. |
@tupek could you try to include the necessary changes to the tests in this PR? |
Discussed this already in Slack. @twsl you found the issue with the tests did you? |
When executing MAP.compute() on CUDA device, my error was different from #671. I got the following with torchmetrics==0.6.1 and torch==1.10.0:
|
@Bunoviske pretty sure this one was fixed in a previous PR. If you install the lib from main branch the error above should show up. |
you can simply install from future bugfix release as
|
@tkupek could you please add test for this case so we can make quick bug-fix release 🐰 |
Sorry, I don't know why the CUDA test is not working. |
I think the issue is that in https://github.com/PyTorchLightning/metrics/blob/d071eb2f1245112c599b7fe0d165b8ac55663083/tests/helpers/testers.py#L168 the passed predictions don't satisfy the condition as a list of lists is passed. Therefor the tensors are never move to device. |
I think the tests should be fixed now. Had to change the tester function to move not only tensors but also collection of tensors to the right device. |
* move and initialize tensors on the correct device (fix cuda) * remove condition for moving tensors, its done in .to() * fix gpu test * docs Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com> Co-authored-by: SkafteNicki <skaftenicki@gmail.com> Co-authored-by: Jirka <jirka.borovec@seznam.cz> (cherry picked from commit 07b5dc5)
What does this PR do?
Fixes an issue where the new MAP implementation cannot be executed on CUDA devices.
The tensors have to be initialized/moved to the correct CUDA device.
Fixes #671
Before submitting
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?
🎉