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

[feat] Implementing threshold consistent margin loss #720

Merged
merged 6 commits into from
Nov 2, 2024

Conversation

ir2718
Copy link
Contributor

@ir2718 ir2718 commented Oct 16, 2024

Hi,

this PR implements the loss presented in the paper Threshold-Consistent Margin Loss for Open-World Deep Metric Learning. The loss is combined with a base loss provided by the user. The default hyperparameters are chosen using the values provided in the section 5.2.

Aside from the implementation, I also added a section in the docs and a test case.

@ir2718 ir2718 changed the title [feat] Implementing threshold consistent nargin loss [feat] Implementing threshold consistent margin loss Oct 16, 2024
@KevinMusgrave
Copy link
Owner

KevinMusgrave commented Oct 28, 2024

Thank you @ir2718!

I wonder if we should get rid of the base_loss argument, and let the user add the two together themselves. For example, if the base loss is ContrastiveLoss then:

loss_fn = ThresholdConsistentMarginLoss()
base_loss_fn = ContrastiveLoss()

# when computing loss
loss = loss_fn(embeddings, labels) + base_loss_fn(embeddings, labels)

And if a user really wants to combine them into a single function call, they can wrap it in a function:

def total_loss():
    return  loss_fn(embeddings, labels) + base_loss_fn(embeddings, labels)

Or use the MultipleLosses wrapper:

from pytorch_metric_learning.losses import MultipleLosses
loss_fn = MultipleLosses([ContrastiveLoss(), ThresholdConsistentMarginLoss()])

@ir2718
Copy link
Contributor Author

ir2718 commented Oct 28, 2024

Oh, I wasn't aware MultipleLosses existed. I removed the base loss as requested and updated the docs accordingly.

@KevinMusgrave
Copy link
Owner

Thanks @ir2718!

@KevinMusgrave KevinMusgrave merged commit c4dcb70 into KevinMusgrave:dev Nov 2, 2024
19 checks passed
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