Skip to content
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

Added Spearman Correlation for similarities module. #168

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Added Spearman Correlation for similarities module. #168

wants to merge 5 commits into from

Conversation

gautamramk
Copy link

Hi,
I have added a function to implement spearman correlation in the similarities module. I am very new to Open source contributions, please do bear with any errors.
I have followed the steps as in the Contribution Guidelines. If I am missing anything, do let me know. Also I am unaware on how to test my code. Would require some guidance.

@NicolasHug
Copy link
Owner

Thanks for the PR!

It looks pretty good even though I haven't checked it in details yet.

For the tests, take a look at how to use pytest and take inspiration from the test_similarities module.

It would also be helpful to have some simple benchmark comparing Spearman with other metrics in terms of RMSE and computation time

@gautamramk
Copy link
Author

I'll run the tests, thanks a lot.

@NicolasHug
Copy link
Owner

This looks good and I'd like to merge it but could you please write a few tests in the test_similarities module?

You'll also need to update the AlgoBase.compute_similarities method.

@gautamramk
Copy link
Author

I am a bit short of time due to my exams. Shall I write the tests after my exams? I shall modify the AlgoBase.compute_similarities now itself though.

@NicolasHug
Copy link
Owner

NicolasHug commented Apr 13, 2018

No worries there's absolutely no rush!
I just wanted to make sure you knew I need some tests before I can merge this ;)

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.

2 participants