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 triplet margin for distance functions in TripletEvaluator #2862

Merged
merged 7 commits into from
Nov 26, 2024

Conversation

zivicmilos
Copy link
Contributor

@zivicmilos zivicmilos commented Jul 25, 2024

Problem:

Triplet margin exists in TripletLoss but does not exist in TripletEvaluator.


Motivation:

When evaluating the model using TripletEvaluator during, for example, the training it would be a nice thing to have a piece of information not only if the negative is further away from the anchor than the positive, but also if it is further away from the anchor by some predefined margin. Because the triplet evaluator uses 4 different distance metrics, we would need an option for each one, so 4 different margins for each of them.


Solution:

In the commit, I implemented exactly that, option for setting the margin for each distance function (or any subset of them). It is an optional argument to the class constructor, so it can be called perfectly the same as previously, with the difference that now you have more flexibility in setting how far away you want your negative and positive to be for evaluation.


PS. I can also write some evaluator tests, both for TripletEvaluator with and without the margin.

PPS. I ran a make check, so the code follows coding guidelines.


@zivicmilos zivicmilos changed the title Add triplet margin for distance functions Add triplet margin for distance functions in TripletEvaluator Jul 25, 2024
@tomaarsen
Copy link
Collaborator

Hello!

Thanks for this PR. I made some changes as I noticed that cosine, manhattan, and Euclidean were based on "distance", but dot was based on similarity instead. I've now updated all to use similarity, so that higher is always better regardless of which similarity function is used.

I've also renamed the parameter to margin - the triplet_... part is already implied because it's on the TripletEvaluator. And I made it singular because I think most people will use it as a single float.

I also added a test. Please let me know what you think @zivicmilos!

  • Tom Aarsen

@zivicmilos
Copy link
Contributor Author

Hey @tomaarsen!

Thanks for reviewing the PR and adding the improvements.

Everything looks good to me, I just added a few more changes:

  • Renamed main_distance_function into main_similarity_function since we are using similarity, not distance, in TripletEvaluator now
  • Changed the description of similarity_fn_names in the docstring because main_similarity_function (previously called main_distance_function) is not used for selecting the evaluation function if similarity_fn_names not specified. For that purpose, model.similarity_fn_name is used (lines 191 and 192).

@tomaarsen
Copy link
Collaborator

Hello!

Apologies for the delay - I still wanted to add d8f4c3d to ensure that there's no backwards incompatibility when people could previously use TripletEvaluator(..., main_distance_function=...).

What do you think?

  • Tom Aarsen

@zivicmilos
Copy link
Contributor Author

Hi @tomaarsen,

This looks awesome, great catch, thanks for the help!

@tomaarsen tomaarsen merged commit b055b5d into UKPLab:master Nov 26, 2024
9 checks passed
@tomaarsen
Copy link
Collaborator

Thanks a bunch for raising this & for dealing with my slowness, I appreciate it.

  • Tom Aarsen

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.

3 participants