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 spearmanr #86

Merged
merged 2 commits into from Nov 25, 2019
Merged

Added spearmanr #86

merged 2 commits into from Nov 25, 2019

Conversation

sleighsoft
Copy link
Contributor

@sleighsoft sleighsoft commented Nov 20, 2019

Addresses issue lmcinnes/umap#319 and #85

I know tests are missing, but I am not 100% sure how to add them the right way. Guidance welcome.

@lmcinnes
Copy link
Owner

Thanks, this looks promising. For tests I think the custom distances require just testing things independently. You can probably just write a couple of tests, one that verifies that the ranking function works as intended, and one that just tests that the spearman distance outputs the results you would expect for some reasonable test vectors (a random example, and a few corner cases such as all 0 vectors, etc.)

@gokceneraslan
Copy link

Cool! Can you also check if all numpy calls are supported by numba's numpy support: https://numba.pydata.org/numba-doc/dev/reference/numpysupported.html

@lmcinnes
Copy link
Owner

Ideally this will happen once some tests exercise that code -- and then we can fix up any issues.

@sleighsoft
Copy link
Contributor Author

sleighsoft commented Nov 20, 2019

I have this working in a jupyter notebook. So it is definitely numba compatible.
Will add tests in the upcoming days.

@sleighsoft
Copy link
Contributor Author

Tests added.

@lmcinnes
Copy link
Owner

It looks like it is just black formatting holding up the tests right now.

@sleighsoft
Copy link
Contributor Author

sleighsoft commented Nov 25, 2019

Weird, I ran black on all of them. + I did not modify rp_trees and _threaded

@lmcinnes
Copy link
Owner

I'm not really sure what the issue is; I'll just merge this and then deal with the fall-out from there. Thanks for submitting 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.

None yet

3 participants