-
-
Notifications
You must be signed in to change notification settings - Fork 159
Fixes #453: _correl_pvalue does not divide by zero if r==1 #474
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
base: main
Are you sure you want to change the base?
Fixes #453: _correl_pvalue does not divide by zero if r==1 #474
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #474 +/- ##
=======================================
Coverage 98.54% 98.54%
=======================================
Files 19 19
Lines 3360 3362 +2
Branches 492 493 +1
=======================================
+ Hits 3311 3313 +2
Misses 26 26
Partials 23 23 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @AlexanderJCS — apologies for the long delay. One request to update the tests to make sure this line is covered by the tests. Thanks
@@ -54,6 +54,9 @@ def _correl_pvalue(r, n, k=0, alternative="two-sided"): | |||
"less", | |||
], "Alternative must be one of 'two-sided' (default), 'greater' or 'less'." | |||
|
|||
if np.isclose(r**2, 1): # Avoid divide by zero error | |||
return 0.0 # Since p value approaches 0 as r approaches 1, just return 0 | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please add a test in https://github.com/raphaelvallat/pingouin/blob/main/tests/test_correlation.py test_corr
function:
stats = corr(x, x, method="percbend") # calls _correl_pvalue
assert np.isclose(stats.at["percbend", "r"], 1)
assert np.isclose(stats.at["percbend", "p_val"], 0)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Raphael, I added these lines in the test_corr(self)
function under the perfect correlation test for the pearson method.
@raphaelvallat The edge case is covered by the correlation test. Please let me know if there's anything else you would like me to change. |
Before this change,
_correl_pvalue
would create a division by zero warning when r==1.Simply adding the condition near the top of the function:
Fixes this. This assumes that n >= 2 since p would be undefined if n < 2, but I did not account for this case since it seems that the function already assumes that n >= 2.
Please let me know if you want me to make any other changes.