-
Notifications
You must be signed in to change notification settings - Fork 405
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
Update _safe_divide
to allow Accuracy
to run on the GPU
#2640
Conversation
Caused a CPU/GPU sync point. torch.where can accept the float directly
for more information, see https://pre-commit.ci
Update comment locations from pre-commit (I got it set up after the original change and missed it locally) Also remove redundant comment
_safe_divide
to allow Accuracy
to run on the GPU
Oh! Thank you @Borda for doing cleanups for me. I apologize I didn't get pre-commit installed until after the initial commit. |
no worries :) |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2640 +/- ##
======================================
Coverage 69% 69%
======================================
Files 316 316
Lines 17881 17881
======================================
Hits 12331 12331
Misses 5550 5550 |
@ndrwrbgs mind checking the failing tests? |
Of course! I’m grateful for the good test coverage and can see what’s happening here. Downgrading to draft so it’s off the docket until it is ready, I overestimated the ease. |
as a draft you will not run all tests so as it is at its advanced stage, let's keep it as full PR just add |
_safe_divide
to allow Accuracy
to run on the GPU_safe_divide
to allow Accuracy
to run on the GPU [wip]
_safe_divide
to allow Accuracy
to run on the GPU [wip]_safe_divide
to allow Accuracy
to run on the GPU [wip]
@ndrwrbgs the error only happens for older versions of PyTorch. The problem is that torch.where(condition, input, other, *, out=None) expects |
Thank you! Promise I’ll get it fixed, I opened this just before visiting
family and have gotten busy 😊.
|
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Thank you for your patience! I noticed errors like below in this Azure job:
Here's an example both from my build and another before to confirm it's not introduced with this PR: 🤔 I'm not sure whether it has any observable impact or not. cc @Borda |
_safe_divide
to allow Accuracy
to run on the GPU [wip]_safe_divide
to allow Accuracy
to run on the GPU
cc @SkafteNicki Another failure on the Windows 2.4.0 torch.Unique: https://github.com/Lightning-AI/torchmetrics/actions/runs/10150851419/job/28068885801?pr=2640 @Borda the other failures appear to be unrelated to the change, but I'm not seeing any rerun button available on my end. Is that something you're able to to with write permissions? |
--------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: jirka <jirka.borovec@seznam.cz> Co-authored-by: Jirka Borovec <6035284+Borda@users.noreply.github.com> (cherry picked from commit 56d3495)
--------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: jirka <jirka.borovec@seznam.cz> Co-authored-by: Jirka Borovec <6035284+Borda@users.noreply.github.com> (cherry picked from commit 56d3495)
Caused a CPU/GPU sync point. torch.where can accept the float directly
What does this PR do?
Fixes #2638
Before submitting
Ensure CPU/GPU sync point avoided for Accuracy metric
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--2640.org.readthedocs.build/en/2640/