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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix pearson aggregation #998

Merged
merged 12 commits into from Apr 29, 2022
Merged

Fix pearson aggregation #998

merged 12 commits into from Apr 29, 2022

Conversation

ben-davidson-6
Copy link
Contributor

What does this PR do?

Fixes aggregation across devices for PearsonCorrCoeff

Before submitting

  • Was this discussed/approved 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 馃檭

@ben-davidson-6
Copy link
Contributor Author

The problem previously was that in the aggregation we were assuming that the per device var_x, var_y and corxy were actually variances and correlations, but they were actually just running sums. The sums can be seen in the following wikepedia article https://en.wikipedia.org/wiki/Algorithms_for_calculating_variance:

  • welfords online algorithm for variance (denoted M_{2,n} in wikepedia)
  • online calculation for covariance (Denoted C_n in wikepedia)

@codecov
Copy link

codecov bot commented Apr 28, 2022 •
edited

Codecov Report

Merging #998 (ccf86e1) into master (9011ec9) will decrease coverage by 0%.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #998   +/-   ##
=====================================
- Coverage      95%    95%   -0%     
=====================================
  Files         179    179           
  Lines        7644   7652    +8     
=====================================
- Hits         7255   7250    -5     
- Misses        389    402   +13

@SkafteNicki SkafteNicki added the bug / fix Something isn't working label Apr 29, 2022
@SkafteNicki SkafteNicki added this to the v0.9 milestone Apr 29, 2022
SkafteNicki
SkafteNicki approved these changes Apr 29, 2022
View reviewed changes
Copy link
Member

@SkafteNicki SkafteNicki left a comment

Choose a reason for hiding this comment

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

LGTM

tests/regression/test_pearson.py Outdated Show resolved Hide resolved
torchmetrics/regression/pearson.py Show resolved Hide resolved
@mergify mergify bot added the ready label Apr 29, 2022
Borda
Borda approved these changes Apr 29, 2022
View reviewed changes
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.

lgtm

@Borda Borda enabled auto-merge (squash) April 29, 2022 12:57
@Borda Borda merged commit 32facee into Lightning-AI:master Apr 29, 2022
@Borda Borda modified the milestones: v0.9, v0.8 May 3, 2022
Borda pushed a commit that referenced this pull request May 5, 2022
@ben-davidson-6 @Borda
Fix pearson aggregation (#998)
d49292c 
* break tests
* fix test
* changelog
* Apply suggestions from code review

Co-authored-by: Nicki Skafte Detlefsen <skaftenicki@gmail.com>

(cherry picked from commit 32facee)
@ben-davidson-6
Copy link
Contributor Author

Can I pretty please get a tshirt for my contribution :)

https://discord.com/channels/1077906959069626439/1077914894428549171/1210976775127957504

@Borda
Copy link
Member

Borda commented Feb 24, 2024

Can I pretty please get a tshirt for my contribution :)

sure, pls fill the form :)

@ben-davidson-6
Copy link
Contributor Author

where is the form sorry, I can't find it.

@Cstrausman89
Copy link

where is the form sorry, I can't find it.

Sorry about that! I just DM'd it to you on Discord. Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Borda Borda Borda approved these changes

@SkafteNicki SkafteNicki SkafteNicki approved these changes

@justusschock justusschock Awaiting requested review from justusschock justusschock is a code owner

@tchaton tchaton Awaiting requested review from tchaton

@ethanwharris ethanwharris Awaiting requested review from ethanwharris

@SeanNaren SeanNaren Awaiting requested review from SeanNaren

Assignees
No one assigned
Labels
bug / fix Something isn't working ready
Projects
None yet
Milestone
v0.8
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@ben-davidson-6 @Borda @Cstrausman89 @SkafteNicki