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

added circle loss #197

Merged
merged 4 commits into from
Mar 9, 2023
Merged

added circle loss #197

merged 4 commits into from
Mar 9, 2023

Conversation

SudeepRed
Copy link
Contributor

@SudeepRed SudeepRed commented Mar 9, 2023

added circle loss as additional loss function.

@netlify
Copy link

netlify bot commented Mar 9, 2023

Deploy Preview for capable-unicorn-d5e336 ready!

Name Link
🔨 Latest commit a1b356d
🔍 Latest deploy log https://app.netlify.com/sites/capable-unicorn-d5e336/deploys/6409276df6c42e0008db50bb
😎 Deploy Preview https://deploy-preview-197--capable-unicorn-d5e336.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented Mar 9, 2023

Deploy Preview for capable-unicorn-d5e336 ready!

Name Link
🔨 Latest commit 8d8d414
🔍 Latest deploy log https://app.netlify.com/sites/capable-unicorn-d5e336/deploys/640996245cc0ff00088c6539
😎 Deploy Preview https://deploy-preview-197--capable-unicorn-d5e336.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Copy link
Contributor

@monatis monatis 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 PR, but it needs important fixes before merging. Please address them and try to optimize a small model with it to make sure that it correctly converges.


def __init__(
self,
margin: Optional[float],
Copy link
Contributor

Choose a reason for hiding this comment

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

margin and scale_factor are marked as Optional but they don't have no default values. According the paper, section 4.1, default values are gamma = 256 and margin = 0.25. Let's set those defaults and add a note to the docstring: "Refer to sections 4.1 and 4.5 in the paper for default values and evaluation of margin and scaling_factor hyperparameters."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review @monatis
I have fixed this in 3e99f94

dists = self.distance_metric.distance_matrix(embeddings)
# Calculate loss for all possible triplets first, then filter by group mask
# Shape: (batch_size, batch_size, 1)
sp = dists.unsqueeze(2)
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't get the sp and sn matrices correctly. They should be similarity matrix of positives and similarity matrix of negatives. See this. Similarly, you can get the similarity matrix by calling self.distance_metric.similarity_matrix and extract positive and negative pairs according to groups.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed it in 3e99f94

@SudeepRed SudeepRed requested a review from monatis March 9, 2023 08:18
@monatis monatis merged commit 394dda6 into qdrant:master Mar 9, 2023
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