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 for PCC error with float16 tensor #1819

Merged
merged 6 commits into from
Jun 5, 2023

Conversation

shenoynikhil
Copy link
Contributor

@shenoynikhil shenoynikhil commented Jun 3, 2023

What does this PR do?

Fixes #1813.

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--1819.org.readthedocs.build/en/1819/

@shenoynikhil shenoynikhil changed the title updated code for amp training Fix for PCC error with float16 tensor Jun 3, 2023
Copy link
Member

@Borda Borda left a comment

Choose a reason for hiding this comment

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

Can we pls add test covering this case?

@SkafteNicki
Copy link
Member

Can we pls add test covering this case?

We do explicitly test that it does not work for float16:

@pytest.mark.xfail(reason="PearsonCorrCoef metric does not support cpu + half precision")
def test_pearson_corrcoef_half_cpu(self, preds, target):
"""Test dtype support of the metric on CPU."""
num_outputs = EXTRA_DIM if preds.ndim == 3 else 1
self.run_precision_test_cpu(preds, target, partial(PearsonCorrCoef, num_outputs=num_outputs), pearson_corrcoef)

So just removing the xfail should be enough.

@SkafteNicki SkafteNicki added the bug / fix Something isn't working label Jun 3, 2023
@SkafteNicki SkafteNicki added this to the v1.0.0 milestone Jun 3, 2023
@mergify mergify bot requested a review from a team June 3, 2023 12:42
@codecov
Copy link

codecov bot commented Jun 3, 2023

Codecov Report

Merging #1819 (a20fb29) into master (c03a716) will decrease coverage by 47%.
The diff coverage is 100%.

Additional details and impacted files
@@           Coverage Diff            @@
##           master   #1819     +/-   ##
========================================
- Coverage      87%     41%    -47%     
========================================
  Files         254     254             
  Lines       14303   14306      +3     
========================================
- Hits        12510    5795   -6715     
- Misses       1793    8511   +6718     

@mergify mergify bot added the ready label Jun 5, 2023
@SkafteNicki SkafteNicki merged commit 6855853 into Lightning-AI:master Jun 5, 2023
@shenoynikhil shenoynikhil deleted the fix-pcc branch June 18, 2023 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug / fix Something isn't working ready
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PCC gives error with AMP
4 participants