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

Feature: Add FastAP Loss #199

Merged
merged 5 commits into from
Mar 28, 2023
Merged

Feature: Add FastAP Loss #199

merged 5 commits into from
Mar 28, 2023

Conversation

KarahanS
Copy link
Contributor

@KarahanS KarahanS commented Mar 25, 2023

  • Implementation of FastAP loss. Primarily inspired by Pytorch-metric-learning implementation and official implementation. For more details, refer to the paper https://cs-people.bu.edu/fcakir/papers/fastap_cvpr2019.pdf.
  • Additionally, I updated the constructor of Circle Loss to give distance_metric_name to the constructor of the superclass. (I'm not sure if it was not given on purpose or a small mistake - related PR)
  • Additionally, updated get_anchor_negative_mask and get_anchor_positive_mask to accept labels_b as Optional[Tensor] so that we can call those functions with only one parameter (as I did in FastAP Loss).
  • I added official implementation to the test_fast_ap_loss.py so that we can compare out loss with the "expected" loss value (Inspired by the test file written for pytorch-metric-learning.)

This PR is now open for review. FYI @generall @monatis.
I'd be happy to hear any suggestions, corrections, or improvements.

@netlify
Copy link

netlify bot commented Mar 25, 2023

Deploy Preview for capable-unicorn-d5e336 ready!

Name Link
🔨 Latest commit e08d335
🔍 Latest deploy log https://app.netlify.com/sites/capable-unicorn-d5e336/deploys/64219ab2377bc10008e9ad0b
😎 Deploy Preview https://deploy-preview-199--capable-unicorn-d5e336.netlify.app/_modules/quaterion/loss/fast_ap_loss
📱 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.

@KarahanS KarahanS marked this pull request as ready for review March 25, 2023 23:42
@KarahanS KarahanS mentioned this pull request Mar 25, 2023
2 tasks
@KarahanS KarahanS changed the title [WIP] Add FastAP Loss Feature: Add FastAP Loss Mar 25, 2023
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.

Could you please add a test for this loss as well? You can also run an experiment by using it one of the examples in the repository.

There are two minor issues in the implementation that I commented, otherwise it looks promising.

quaterion/loss/fast_ap_loss.py Outdated Show resolved Hide resolved
quaterion/loss/fast_ap_loss.py Outdated Show resolved Hide resolved
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 handling these reviews. LGTM, I'll merge it.

@monatis monatis merged commit 3f0a96c into qdrant:master Mar 28, 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