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

fixed duplicate calculation of spearmanr function in metrics wrapper. #4627

Merged

Conversation

benlipkin
Copy link
Contributor

During _compute, the scipy.stats spearmanr function was called twice, redundantly, once for calculating the score and once for calculating the p-value, under the conditional branch where return_pvalue=True. I adjusted the _compute function to execute the spearmanr function once, store the results tuple in a temporary variable, and then pass the indexed contents to the expected keys of the returned dictionary.

Copy link
Member

@lhoestq lhoestq left a comment

Choose a reason for hiding this comment

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

Thank you !

I think the same improvement can be applied in the evaluate library, which is the new library we're using to add new metrics :)

https://github.com/huggingface/evaluate/blob/main/metrics/spearmanr/spearmanr.py

@benlipkin
Copy link
Contributor Author

Great, can open a PR in evaluate as well to optimize this.

Relatedly, I wanted to add a new metric, Kendall Tau (https://docs.scipy.org/doc/scipy/reference/generated/scipy.stats.kendalltau.html). If I were to open a PR with the wrapper, description, citation, docstrings, readme, etc. would it make more sense to do that in the datasets or evaluate repo (or both)?

Thanks!

@benlipkin
Copy link
Contributor Author

PR opened inevaluate library with same minor adjustment: huggingface/evaluate#176

@lhoestq
Copy link
Member

lhoestq commented Jul 7, 2022

If I were to open a PR with the wrapper, description, citation, docstrings, readme, etc. would it make more sense to do that in the datasets or evaluate repo (or both)?

I think you could just add it to evaluate, we're not adding new metrics in this repo anymore

@lhoestq lhoestq merged commit 731e7b0 into huggingface:main Jul 7, 2022
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