Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork10.9k
ENH: adding covariance param to corrcoef - see ticket #19852#21434
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 ourterms of service andprivacy 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?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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.
Thank you! I am not sure where the decision was standing on adding this, but it seems reasonable to me.
I added a few comments for things that need to be fixed. But the main task implementation-wise will be to add tests for the new feature and also for all the relevant error paths!
numpy/lib/function_base.py Outdated
| @@ -2681,14 +2681,14 @@ def cov(m, y=None, rowvar=True, bias=False, ddof=None, fweights=None, | |||
| return c.squeeze() | |||
| def _corrcoef_dispatcher(x, y=None, rowvar=None, bias=None, ddof=None, *, | |||
| def _corrcoef_dispatcher(x=None, cov = None, y=None, rowvar=None, bias=None, ddof=None, *, | |||
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.
The new parameter must be added at the end (or after the*, since order is irrelevant after that). In principle we could add it last, but in practice we want users to writecov=... anyway.
numpy/lib/function_base.py Outdated
| @@ -2728,6 +2728,9 @@ def corrcoef(x, y=None, rowvar=True, bias=np._NoValue, ddof=np._NoValue, *, | |||
| .. versionadded:: 1.20 | |||
| cov : _NoValue, optional | |||
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.
This should be the actual type, i.e. array_like probably
numpy/lib/function_base.py Outdated
| @@ -2818,7 +2821,12 @@ def corrcoef(x, y=None, rowvar=True, bias=np._NoValue, ddof=np._NoValue, *, | |||
| # 2015-03-15, 1.10 | |||
| warnings.warn('bias and ddof have no effect and are deprecated', | |||
| DeprecationWarning, stacklevel=3) | |||
| c = cov(x, y, rowvar, dtype=dtype) | |||
| if x is None and cov is None: | |||
| raise ValueError("'x' or 'cov' must be passed as a parameter") | |||
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.
This sould be aTypeError, it should be similar to what you currently get fornp.corrcoef() after all.
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.
I don't think you check forboth being passed. We need to check that exactly one of them is passed only.
numpy/lib/function_base.py Outdated
| @@ -2839,6 +2847,9 @@ def corrcoef(x, y=None, rowvar=True, bias=np._NoValue, ddof=np._NoValue, *, | |||
| return c | |||
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.
Please remove these additional blank lines again.
Uh oh!
There was an error while loading.Please reload this page.
Param cov was added to corrcoef func in order to accept a pre-calculated covariance matrix