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 dtype casting in spearman and pearson #2379

Merged
merged 9 commits into from
Feb 14, 2024
Merged

Conversation

SkafteNicki
Copy link
Member

@SkafteNicki SkafteNicki commented Feb 13, 2024

What does this PR do?

Fixes #2314
We have guarded in general against frameworks such as deepspeed changing the metric state dtypes. However, for certain metrics this may still happen if the metric overwrites the state directly instead of doing self.metric_state += batch_update which nearly all metrics does. This PR simply fixes this for Pearson and Spearman metric.

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

@SkafteNicki SkafteNicki added bug / fix Something isn't working enhancement New feature or request labels Feb 13, 2024
@SkafteNicki SkafteNicki added this to the v1.3.x milestone Feb 13, 2024
@SkafteNicki SkafteNicki marked this pull request as ready for review February 14, 2024 06:43
Copy link

codecov bot commented Feb 14, 2024

Codecov Report

Merging #2379 (12371ef) into master (afae59e) will increase coverage by 0%.
The diff coverage is 100%.

Additional details and impacted files
@@          Coverage Diff           @@
##           master   #2379   +/-   ##
======================================
  Coverage      69%     69%           
======================================
  Files         305     305           
  Lines       17180   17186    +6     
======================================
+ Hits        11840   11846    +6     
  Misses       5340    5340           

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@mergify mergify bot added the ready label Feb 14, 2024
@Borda Borda merged commit f4ef8a8 into master Feb 14, 2024
58 checks passed
@Borda Borda deleted the bugfix/dtype_casting branch February 14, 2024 18:35
Borda pushed a commit that referenced this pull request Mar 16, 2024
* fix dtype in pearson and spearman
* small refactor
* update classification
* update to new pytest format
* Apply suggestions from code review

---------

Co-authored-by: Jirka Borovec <6035284+Borda@users.noreply.github.com>

(cherry picked from commit f4ef8a8)
Borda pushed a commit that referenced this pull request Mar 18, 2024
* fix dtype in pearson and spearman
* small refactor
* update classification
* update to new pytest format
* Apply suggestions from code review

---------

Co-authored-by: Jirka Borovec <6035284+Borda@users.noreply.github.com>

(cherry picked from commit f4ef8a8)
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 enhancement New feature or request ready topic: Regress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DeepSpeed still, still changes metric states from fp32 to fp16
2 participants