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

Add Kendall's Tau-c, Spearman's Rank, Pearson's Correlation metrics #2741

Merged
merged 4 commits into from
Aug 5, 2022

Conversation

nicolaspi
Copy link
Contributor

@nicolaspi nicolaspi commented Aug 5, 2022

Description

Add Kendall's Tau-c, Spearman's Rank, Pearson's Correlation metrics.

Type of change

Checklist:

  • I've properly formatted my code according to the guidelines
    • By running pre-commit hooks
  • This PR addresses an already submitted issue for TensorFlow Addons
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • This PR contains modifications to C++ custom-ops

How Has This Been Tested?

The tests follows the contribution guidelines for metrics (https://github.com/tensorflow/addons/blob/master/tensorflow_addons/metrics/README.md#contribution-guidelines).
The metrics are tested against the scipy implementation counterpart.

@boring-cyborg boring-cyborg bot added the metrics label Aug 5, 2022
@bot-of-gabrieldemarmiesse

@sorensenjs

You are owner of some files modified in this pull request.
Would you kindly review the changes whenever you have the time to?
Thank you very much.

Copy link
Contributor

@sorensenjs sorensenjs left a comment

Choose a reason for hiding this comment

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

This looks like a good change, but it's very hard to see the diffs - is it possible to either branch the file so that it shows diffs, or separate the renaming of the file from the other edits.

I'm not a frequent github user - if there's an easy way for me to see the diffs that would be helpful.

@nicolaspi
Copy link
Contributor Author

nicolaspi commented Aug 5, 2022

There is no change per se, the original code remains the same modulo some refactoring. KendallsTau has been renamed KendallsTauB (to differenciate from KendallsTauC).

I have factored update_state from KendallsTau into a base class CorrelationBase with the abstract method result. For each metric, there is a concrete class that implements result with the corresponding metric's logic.

@nicolaspi
Copy link
Contributor Author

@sorensenjs I changed the commit history to highlight the differences, this is the best I could come up with: b4977f4

Copy link
Contributor

@sorensenjs sorensenjs left a comment

Choose a reason for hiding this comment

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

Thanks for the clarification. This is a nice generalization.

@bhack bhack merged commit 5759e3d into tensorflow:master Aug 5, 2022
@bhack
Copy link
Contributor

bhack commented Aug 5, 2022

@nicolaspi Can you PR the codeowners file to match this refactoring?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants