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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

BUG: Clip corr edge cases between -1.0 and 1.0 #61154

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

BUG: Clip corr edge cases between -1.0 and 1.0 #61154

wants to merge 10 commits into from
+36 −6

Conversation

j-hendricks
Copy link
Contributor

Closes #61120

Clips correlation coefficient in DataFrame.corr() between -1.0 and 1.0

Adds test_corr_within_bounds to ensure coefficient within bounds

Sorry, something went wrong.

mroeschke
mroeschke requested changes Mar 20, 2025
View reviewed changes
@jhendricks487
Transfered np.clip to algos.nancorr
Loading
Loading status checks…
7f2fdaa
@j-hendricks j-hendricks requested a review from WillAyd as a code owner March 20, 2025 16:42
mroeschke
mroeschke reviewed Mar 20, 2025
View reviewed changes
jhendricks487 and others added 3 commits March 20, 2025 13:12
@jhendricks487
Clip covxy / divsor instead of result
34eb701
@jhendricks487
Clip covxy / divsor within nogil
Loading
Loading status checks…
fc5ccae
@j-hendricks
Merge branch 'main' into clip-corr-edge-cases

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
GPG key ID: B5690EEEBB952194
Verified
Learn about vigilant mode
Loading
Loading status checks…
904c892
mroeschke
mroeschke requested changes Mar 21, 2025
View reviewed changes
Copy link
Member

@mroeschke mroeschke left a comment

Choose a reason for hiding this comment

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

Thanks, does nancorr_spearman also need this change?

@j-hendricks
Copy link
Contributor Author

I do not think nancorr_spearman requires this change because it computes the divisor by first ranking the raw data rather than using the raw data itself like nancorr. This results in a stable value for the divisor. In the case of y = [1.35951, 1.3595100000000007], the divisor equals 0.5 for nancorr_spearman but equals almost 0 for nancorr.

@j-hendricks
Merge branch 'main' into clip-corr-edge-cases

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
GPG key ID: B5690EEEBB952194
Verified
Learn about vigilant mode
Loading
Loading status checks…
c83ccaf
@mroeschke mroeschke added this to the 3.0 milestone Mar 24, 2025
mroeschke
mroeschke reviewed Mar 24, 2025
View reviewed changes
Copy link
Member

@mroeschke mroeschke left a comment

Choose a reason for hiding this comment

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

Thanks could you add a whatsnew note to v3.0.0.rst?

@j-hendricks
Copy link
Contributor Author

Added a whatnew note under "Notable Bug Fixes"

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

@mroeschke mroeschke

@WillAyd WillAyd

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
cov/corr
Projects
None yet
Milestone
3.0
Development

Successfully merging this pull request may close these issues.

BUG: .corr() values significantly higher than 1
3 participants
@j-hendricks @mroeschke @jhendricks487