ENH: Add axis argument to Dataframe.corr #35984

kc611 · 2020-08-30T07:12:04Z

closes Add axis argument to DataFrame.corr #35002
tests added / passed

pep8speaks · 2020-08-30T07:12:09Z

Hello @kc611! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-08-31 15:28:08 UTC

dsaxton

Thanks @kc611 for the PR

pandas/tests/frame/methods/test_cov_corr.py

pandas/core/frame.py

dsaxton · 2020-08-30T14:05:41Z

Also can you add a whatsnew note for v1.2.0?

kc611 · 2020-08-30T14:29:24Z

@dsaxton Thanks for the review. I made the changes accordingly.

I was also thinking about adding a callable method in the test but since this is a minor improvement (and most probably will work for every case for which Dataframe.corr worked normally,since we're only transposing the original matrix), I dropped the idea. Any changes to tests are welcome.

And also under what section of whatsnew note for v1.2.0 should I add the note?

pandas/tests/frame/methods/test_cov_corr.py

dsaxton · 2020-08-30T14:44:01Z

And also under what section of whatsnew note for v1.2.0 should I add the note?

You can add it under enhancements (a bullet within other enhancements is probably fine)

doc/source/whatsnew/v1.2.0.rst

Co-authored-by: Daniel Saxton <[email protected]>

pandas/core/frame.py

…sue_35002

dsaxton

LGTM @kc611 nice work

kc611 · 2020-08-30T15:50:58Z

LGTM @kc611 nice work

Thanks

MarcoGorelli

Thanks @kc611 , just leaving a small request

pandas/core/frame.py

kc611 · 2020-08-31T11:57:57Z

@MarcoGorelli Thanks for review, I added the type annotations as requested.

But it seems that the tests are failing and I cannot figure out the exact reason. Can you assist me.

dsaxton · 2020-08-31T13:22:32Z

pandas/core/frame.py

+    def corr(
+        self,
+        method: Union[str, Callable[[np.ndarray, np.ndarray], np.float64]] = "pearson",
+        min_periods: Optional[int] = 1, axis: Union[str, int] = 0


Running black on this file should fix the linting, the other CI issues seem unrelated

https://pandas.pydata.org/docs/dev/development/contributing.html#python-pep8-black

Thanks @dsaxton. The linting issue is fixed but CI issue still persists

MarcoGorelli

Generally looks good to me, though I'm not the best person to comment on whether doing it this way (taking the transpose) is most efficient. @dsaxton @WillAyd @jreback in case you have comments

MarcoGorelli · 2020-09-01T16:32:20Z

pandas/tests/frame/methods/test_cov_corr.py

+    def test_corr_axes(self, meth):
+        # https://github.com/pandas-dev/pandas/issues/35002
+        df = pd.DataFrame(np.random.normal(size=(10, 4)))
+        expected = df.T.corr(meth, axis=0)


I think it's usually encouraged to explicitly write out the expected DataFrame so that expected doesn't go down the same code path as result

Yeah I could do that but, wouldn't it just be a test for Dataframe.corr function itself. Since the original operations to be done on matrix itself are left unchanged.

Personally I don't think explicitly writing Dataframe in this case is needed, unless (as you suggested) instead of taking a transpose we implement a workaround involving changing the main function itself.

OK sure, perhaps wait for others' comments then

The test comes close to being circular but I think it's probably okay here. In this case it's hard to explicitly construct the expected DataFrame for all methods "from scratch" without either trivial input data or messy juggling of different scipy functions.

jreback · 2020-09-05T15:38:49Z

pandas/core/frame.py

-    def corr(self, method="pearson", min_periods=1) -> "DataFrame":
+    def corr(
+        self,
+        method: Union[str, Callable[[np.ndarray, np.ndarray], np.float64]] = "pearson",


can you move this signature to an alias and put it in pandas._typing, cal it MethodWithCallable

jreback · 2020-09-05T15:39:05Z

pandas/core/frame.py

@@ -8162,12 +8174,22 @@ def corr(self, method="pearson", min_periods=1) -> "DataFrame":
              dogs  cats
        dogs   1.0   0.3
        cats   0.3   1.0
+        >>> df.corr(method=histogram_intersection, axis=1)


blank line before

add a comment as well

jreback · 2020-09-05T15:39:48Z

pandas/core/frame.py

        idx = cols.copy()
        mat = numeric_df.to_numpy(dtype=float, na_value=np.nan, copy=False)

+        if axis == 1:


don't we have to transpose the results?

I don't think we do since the result is symmetric

jreback · 2020-09-05T15:40:28Z

pandas/tests/frame/methods/test_cov_corr.py

+    @pytest.mark.parametrize("meth", ["pearson", "spearman", "kendall"])
+    def test_corr_axes(self, meth):
+        # https://github.com/pandas-dev/pandas/issues/35002
+        df = pd.DataFrame(np.random.normal(size=(10, 4)))


put axis labels that are differnt for rows / columns and this should fail (need to handle that)

dsaxton · 2020-09-20T12:09:42Z

@kc611 Can you merge master and address @jreback comments above?

github-actions · 2020-10-21T00:16:35Z

This pull request is stale because it has been open for thirty days with no activity. Please update or respond to this comment if you're still interested in working on this.

dsaxton · 2020-10-21T00:49:07Z

@kc611 Can you address comments and merge master?

Added axis argument to Dataframe.corr

f6ef933

kc611 mentioned this pull request Aug 30, 2020

Add axis argument to DataFrame.corr #35002

Open

kc611 added 4 commits August 30, 2020 13:04

Minor Linting Issues

3648aeb

Minor Whitespace Issue

c588bc4

Changed Failing Test

0dbde9d

Updated Documantation

feecd8d

kc611 closed this Aug 30, 2020

kc611 reopened this Aug 30, 2020

kc611 added 2 commits August 30, 2020 14:28

Debug Modified Dataframe.corr function

91c1e3e

Removed PEP 8 Issues-Trailing Whitespaces

fb89cbe

dsaxton reviewed Aug 30, 2020

View reviewed changes

pandas/tests/frame/methods/test_cov_corr.py Outdated Show resolved Hide resolved

pandas/tests/frame/methods/test_cov_corr.py Outdated Show resolved Hide resolved

pandas/core/frame.py Outdated Show resolved Hide resolved

pandas/core/frame.py Outdated Show resolved Hide resolved

dsaxton added Enhancement Numeric Operations Arithmetic, Comparison, and Logical operations labels Aug 30, 2020

dsaxton changed the title ~~Added axis argument to Dataframe.corr~~ ENH: Add axis argument to Dataframe.corr Aug 30, 2020

Made suitable changes according to review

d2a87f8

dsaxton reviewed Aug 30, 2020

View reviewed changes

pandas/tests/frame/methods/test_cov_corr.py Outdated Show resolved Hide resolved

kc611 added 2 commits August 30, 2020 20:43

Added whatsnew note

725f36a

Improved tests

f1c884b

dsaxton reviewed Aug 30, 2020

View reviewed changes

doc/source/whatsnew/v1.2.0.rst Outdated Show resolved Hide resolved

Update doc/source/whatsnew/v1.2.0.rst

0552fa4

Co-authored-by: Daniel Saxton <[email protected]>

dsaxton reviewed Aug 30, 2020

View reviewed changes

pandas/core/frame.py Show resolved Hide resolved

kc611 added 2 commits August 30, 2020 21:13

Added version added directive

3c4f88c

Merge branch 'issue_35002' of https://github.com/kc611/pandas into is…

0f1e817

…sue_35002

dsaxton approved these changes Aug 30, 2020

View reviewed changes

dsaxton added this to the 1.2 milestone Aug 30, 2020

MarcoGorelli suggested changes Aug 31, 2020

View reviewed changes

pandas/core/frame.py Outdated Show resolved Hide resolved

kc611 added 2 commits August 31, 2020 17:21

Added type annotations

5871508

Resolved Pep8 whitespace issues

b98401b

dsaxton reviewed Aug 31, 2020

View reviewed changes

kc611 added 2 commits August 31, 2020 20:10

Solved linter Issue

8fc6cda

Solved Linter Issues

f2e6e84

MarcoGorelli suggested changes Sep 1, 2020

View reviewed changes

jreback requested changes Sep 5, 2020

View reviewed changes

github-actions bot added the Stale label Oct 21, 2020

kc611 closed this Oct 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: Add axis argument to Dataframe.corr #35984

ENH: Add axis argument to Dataframe.corr #35984

kc611 commented Aug 30, 2020 •

edited

Loading

pep8speaks commented Aug 30, 2020 •

edited

Loading

dsaxton left a comment

dsaxton commented Aug 30, 2020

kc611 commented Aug 30, 2020

dsaxton commented Aug 30, 2020

dsaxton left a comment

kc611 commented Aug 30, 2020

MarcoGorelli left a comment

kc611 commented Aug 31, 2020 •

edited

Loading

dsaxton Aug 31, 2020

kc611 Aug 31, 2020 •

edited

Loading

MarcoGorelli left a comment

MarcoGorelli Sep 1, 2020

kc611 Sep 1, 2020

MarcoGorelli Sep 1, 2020

dsaxton Sep 1, 2020

jreback Sep 5, 2020

jreback Sep 5, 2020

jreback Sep 5, 2020

jreback Sep 5, 2020

dsaxton Sep 20, 2020

jreback Sep 5, 2020

dsaxton commented Sep 20, 2020

github-actions bot commented Oct 21, 2020

dsaxton commented Oct 21, 2020

ENH: Add axis argument to Dataframe.corr #35984

ENH: Add axis argument to Dataframe.corr #35984

Conversation

kc611 commented Aug 30, 2020 • edited Loading

pep8speaks commented Aug 30, 2020 • edited Loading

Comment last updated at 2020-08-31 15:28:08 UTC

dsaxton left a comment

Choose a reason for hiding this comment

dsaxton commented Aug 30, 2020

kc611 commented Aug 30, 2020

dsaxton commented Aug 30, 2020

dsaxton left a comment

Choose a reason for hiding this comment

kc611 commented Aug 30, 2020

MarcoGorelli left a comment

Choose a reason for hiding this comment

kc611 commented Aug 31, 2020 • edited Loading

Choose a reason for hiding this comment

kc611 Aug 31, 2020 • edited Loading

Choose a reason for hiding this comment

MarcoGorelli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsaxton commented Sep 20, 2020

github-actions bot commented Oct 21, 2020

dsaxton commented Oct 21, 2020

kc611 commented Aug 30, 2020 •

edited

Loading

pep8speaks commented Aug 30, 2020 •

edited

Loading

kc611 commented Aug 31, 2020 •

edited

Loading

kc611 Aug 31, 2020 •

edited

Loading