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

f1 metric - micro/macro/weighted #284

Merged
merged 10 commits into from
Jun 18, 2019
Merged

Conversation

SSaishruthi
Copy link
Contributor

This PR contains a script for adding f1-macro metric.

Colab notebook: https://colab.research.google.com/drive/1LJ1yb8cUgisNP3ETfMLRYPQEyJivzDv-

@SSaishruthi SSaishruthi requested a review from a team as a code owner June 12, 2019 18:19
@SSaishruthi
Copy link
Contributor Author

Please let me know what will be my next steps. To do as far as I know will be to add a test for the same.

Copy link
Member

@Squadrick Squadrick left a comment

Choose a reason for hiding this comment

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

@SSaishruthi Next steps: __init__.py, README.md and tests.

@SSaishruthi
Copy link
Contributor Author

Thanks @Squadrick
Will update soon

@SSaishruthi
Copy link
Contributor Author

@Squadrick
Updated with F1 micro, macro and weighted
Test colab notebook: https://colab.research.google.com/drive/1qSq0SsYkPqjdKUgM1RM4kKM67X75ocFj

@SSaishruthi SSaishruthi changed the title f1-macro metric f1 metric - micro/macro/weighted Jun 13, 2019
Copy link
Member

@Squadrick Squadrick left a comment

Choose a reason for hiding this comment

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

@SSaishruthi The code looks good so far, just a few small changes. Also looking you need to add the test cases, update __init__.py, README.md and BUILD.

@SSaishruthi
Copy link
Contributor Author

SSaishruthi commented Jun 14, 2019

@Squadrick
Updated scripts. Please let me know if there is any change to be made.
Colab: https://colab.research.google.com/drive/1qSq0SsYkPqjdKUgM1RM4kKM67X75ocFj

Copy link
Member

@Squadrick Squadrick left a comment

Choose a reason for hiding this comment

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

Update the module to be called F1Score to accept micro as well. Like @WindQAQ mentioned here, it's a much cleaner API and reduces the amount of net code.

@SSaishruthi
Copy link
Contributor Author

Hi @Squadrick @WindQAQ

I have updated code to calculate F1 scores under one class.
Now, we have four options:

  1. Micro
  2. Macro
  3. Weighted
  4. None (This will produce F1 scores for each class)

Colab: https://colab.research.google.com/drive/1qSq0SsYkPqjdKUgM1RM4kKM67X75ocFj#scrollTo=hZSxoW4eYqYW

I ran code format to check scripts. I encountered an installation error in clang and removed that step because I noticed that is only for C++.

@Squadrick
Copy link
Member

@SSaishruthi Can you pull from master and update this PR?

@SSaishruthi
Copy link
Contributor Author

SSaishruthi commented Jun 17, 2019

@Squadrick

I have updated the branch and resolved conflicts.
Can we trigger the test again?

Copy link
Member

@Squadrick Squadrick left a comment

Choose a reason for hiding this comment

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

Left a few comments about refactoring the test cases, after which we should be good to merge.

def initialize_vars(self):
f1_obj = F1Score(num_classes=3, average='micro')
f1_obj1 = F1Score(num_classes=3, average='macro')
f1_obj2 = F1Score(num_classes=3, average='weighted')
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Rename f1_obj -> f1_micro, f1_obj1 -> f1_macro, f1_obj2 -> f1_weighted.

Copy link
Member

Choose a reason for hiding this comment

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

Do the renaming through out the file.

Copy link
Member

Choose a reason for hiding this comment

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

We can merge once as soon as this is done.

@SSaishruthi
Copy link
Contributor Author

SSaishruthi commented Jun 18, 2019

@Squadrick

Inputs to None condition should be multiclass but not multilabel. That's why I created a separate function for None as they require a different set of inputs. Can we have it like this or if you can suggest me an alternative that would be great.

One solution I thought I can try is to have two sets of input, pass both (actual1, actual2, pred1, pred2) and use only multiclass(actual1, pred1) for none. Will that work?

Thanks

@Squadrick
Copy link
Member

@SSaishruthi Oh, my bad. In that case, leave it as is.

@SSaishruthi
Copy link
Contributor Author

@Squadrick updated the scripts. Can you please trigger the test again?

Copy link
Member

@Squadrick Squadrick left a comment

Choose a reason for hiding this comment

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

@SSaishruthi LGTM! Thanks for the contribution.

@Squadrick Squadrick merged commit ebebe3c into tensorflow:master Jun 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants