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

[losses] migrate npairs multilabel loss #466

Merged
merged 2 commits into from
Sep 3, 2019

Conversation

WindQAQ
Copy link
Member

@WindQAQ WindQAQ commented Aug 30, 2019

Closes #26. Finally the last one :-)

  • Migrate npairs_loss_multilabel from contrib

  • Rename npairs_loss_multilabel to npairs_multilabel_loss, which makes sense to addons' naming style

  • Modify input spec from

    • labels (list of sparse tensor)
    • embedding_anchor
    • embedding_posititve

    to

    • y_true (either tensor or sparse tensor)
    • y_pred (similarity matrix, which is the multiplication of anchor and positive)

Copy link
Member

@facaiy facaiy left a comment

Choose a reason for hiding this comment

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

Looks good, Tzu-wei!

By the way, can we provide a helper method which calculates similarity_matrix? It might be user-friendly for tf 1.x users. What do you think?

@facaiy facaiy requested a review from Squadrick September 2, 2019 00:35
@facaiy
Copy link
Member

facaiy commented Sep 2, 2019

@Squadrick Hi, Dheeraj. I think the PR looks good. Do you have time to double check it? Thanks :-)

@WindQAQ
Copy link
Member Author

WindQAQ commented Sep 2, 2019

Looks good, Tzu-wei!

By the way, can we provide a helper method which calculates similarity_matrix? It might be user-friendly for tf 1.x users. What do you think?

Seems good, but it will be a one line function like tf.matmul(a, b).

@facaiy
Copy link
Member

facaiy commented Sep 2, 2019

@WindQAQ Oh, you're right, forget it

@Squadrick
Copy link
Member

Maybe add a test to check that the new loss is compatible with the Keras Model API? It would be great if we add it under utils and every loss has to use the check. What do y'all think?

@facaiy
Copy link
Member

facaiy commented Sep 3, 2019

Like layer_test or https://github.com/tensorflow/tensorflow/blob/0197a2d8a3070af763cb67227835ee63df095e6d/tensorflow/python/keras/losses_test.py#L68 ? I have no objection. How about filing an issue and finishing it in next PR?

Copy link
Member

@facaiy facaiy left a comment

Choose a reason for hiding this comment

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

I'd like to merge it at first, thank you, Tzu-Wei!

@facaiy facaiy merged commit 68e8bb9 into tensorflow:master Sep 3, 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.

Implement Metric Losses
6 participants