Skip to content

ENH: DataFrame.corr(method='spearman') is cythonized. #3823

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 2 commits into from
Jun 17, 2013
Merged

ENH: DataFrame.corr(method='spearman') is cythonized. #3823

merged 2 commits into from
Jun 17, 2013

Conversation

jniznan
Copy link
Contributor

@jniznan jniznan commented Jun 9, 2013

It should be more than 10 times faster than the old version.

@jreback
Copy link
Contributor

jreback commented Jun 9, 2013

ok....can you put a release notes mention? (enhancement section),

and if you are gung ho, pls add...a vbench entry (vb_suite/stat_ops.py)....it could have a start date of say 2013/1/1 (or earlier if you can figure out about when spearman was added....was sometime last year, but I don't remember)

@cpcloud
Copy link
Member

cpcloud commented Jun 10, 2013

@jreback is there any consensus about fused types/memoryviews? if there's a consensus to use should we "require" that new cython use it?

@jreback
Copy link
Contributor

jreback commented Jun 10, 2013

should use them
but in this case doesn't matter as float64 is all that is needed

@cpcloud
Copy link
Member

cpcloud commented Jun 10, 2013

fused types sure but what about memoryviews here?

@jreback
Copy link
Contributor

jreback commented Jun 10, 2013

if u can get them to work
should do it


N, K = (<object> mat).shape

if minp is None:
Copy link
Member

Choose a reason for hiding this comment

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

why not just make this default to 1 and then u can give it a type of Py_ssize_t...

@jreback
Copy link
Contributor

jreback commented Jun 12, 2013

@jniznan does @cpcloud change make sense?

@jniznan
Copy link
Contributor Author

jniznan commented Jun 12, 2013

Yes, I will push the changes in the next day or two.

@jreback
Copy link
Contributor

jreback commented Jun 15, 2013

@jniznan can you see if you can finish this up ? thanks

@jniznan
Copy link
Contributor Author

jniznan commented Jun 16, 2013

@jreback sorry about the delay,
it seems that the spearman functionality was added on 2011/12/3 (GH #428) , so i will put 2011/12/4 in the benchmark.

@jniznan
Copy link
Contributor Author

jniznan commented Jun 16, 2013

@jreback i put in the release notes mention and the vbench entry, i have also implemented the change @cpcloud proposed

@cpcloud
Copy link
Member

cpcloud commented Jun 16, 2013

@jniznan can u rebase?

@jniznan
Copy link
Contributor Author

jniznan commented Jun 17, 2013

@cpcloud it should be rebased now.

@jreback jreback merged commit 0a42ae3 into pandas-dev:master Jun 17, 2013
@jreback
Copy link
Contributor

jreback commented Jun 17, 2013

Merged...thanks.....nice improvements

-------------------------------------------------------------------------------
Test name                                    | head[ms] | base[ms] |  ratio   |
-------------------------------------------------------------------------------
stats_corr_spearman                          |  83.9807 | 3735.8600 |   0.0225 |

Target [a70b48a] : PERF: stats_corr a smaller vbsuite
Base   [c4a218c] : Merge pull request #3929 from y-p/PR_PTF_no_more

@jniznan jniznan deleted the correlations branch June 18, 2013 05:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants