Skip to content

CLN: Clean nanops.get_corr_func #33244

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

Merged
merged 6 commits into from
Apr 7, 2020
Merged

CLN: Clean nanops.get_corr_func #33244

merged 6 commits into from
Apr 7, 2020

Conversation

dsaxton
Copy link
Member

@dsaxton dsaxton commented Apr 2, 2020

Small cleaning (instead of creating a dictionary of functions and then returning only one, just return the one)

return np.corrcoef(a, b)[0, 1]

def _kendall(a, b):
# kendallttau returns a tuple of the tau statistic and pvalue
Copy link
Member

Choose a reason for hiding this comment

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

is this comment worth retaining?

Copy link
Member Author

Choose a reason for hiding this comment

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

Might as well, should we also have a comment for spearmanr and np.corrcoef?

Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

do we have tests for invaid method names?

def func(a, b):
return kendalltau(a, b)[0]

return func
Copy link
Contributor

Choose a reason for hiding this comment

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

i would make all of these lambda, e.g.(for all 3 cases)

return lambda a, b: kendaltau(a, b)[0]

Copy link
Member

Choose a reason for hiding this comment

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

hah, i asked for these to be changed from lambda to non-lambdas for nicer tracebacks

Copy link
Contributor

Choose a reason for hiding this comment

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

oh really it does? hmm, ok, then that's fine

@jreback jreback added Clean Numeric Operations Arithmetic, Comparison, and Logical operations labels Apr 5, 2020
@jreback jreback added this to the 1.1 milestone Apr 5, 2020
@dsaxton
Copy link
Member Author

dsaxton commented Apr 5, 2020

do we have tests for invaid method names?

There's testing at least for DataFrame.corr (test_corr_invalid_method in /tests/frame/methods/test_cov_corr.py) but not this function directly AFAIK

@jreback jreback merged commit fc98ab5 into pandas-dev:master Apr 7, 2020
@jreback
Copy link
Contributor

jreback commented Apr 7, 2020

thanks

@dsaxton dsaxton deleted the get-corr branch April 7, 2020 01:25
jbrockmendel pushed a commit to jbrockmendel/pandas that referenced this pull request Apr 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Clean Numeric Operations Arithmetic, Comparison, and Logical operations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants