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

Adding triplet margin loss function #2762

Merged
merged 13 commits into from Dec 23, 2020
Merged

Adding triplet margin loss function #2762

merged 13 commits into from Dec 23, 2020

Conversation

ayushsingh11
Copy link
Contributor

Hi all, this PR is in reference to issue #2200. I have cherry-picked commits from issue #2208 and added minor changes according to the discussion on it. Please review it and inform if any changes are required.

2 matrices are taken as input :

  1. input - It consists of concatenated anchor and positive samples.
  2. target - It consists of negative samples.

@mlpack-bot
Copy link

mlpack-bot bot commented Dec 12, 2020

Thanks for opening your first pull request in this repository! Someone will review it when they have a chance. In the mean time, please be sure that you've handled the following things, to make the review process quicker and easier:

  • All code should follow the style guide
  • Documentation added for any new functionality
  • Tests added for any new functionality
  • Tests that are added follow the testing guide
  • Headers and license information added to the top of any new code files
  • HISTORY.md updated if the changes are big or user-facing
  • All CI checks should be passing

Thank you again for your contributions! 👍

Added Triplet margin loss function
Replace BOOST_SERIALIZATION_NVP with CEREAL_NVP.
@ayushsingh11
Copy link
Contributor Author

Thanks @NippunSharma . Added your changes and a few more in the implementation of Boost. Now the build was successful and test cases are also passing.

@ayushsingh11
Copy link
Contributor Author

Thanks @zoq , made the changes you suggested. Hoping everything else is fine.

Copy link
Member

@zoq zoq left a comment

Choose a reason for hiding this comment

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

No more comments from my side, thanks for picking this up.

Copy link

@mlpack-bot mlpack-bot bot left a comment

Choose a reason for hiding this comment

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

Second approval provided automatically after 24 hours. 👍

@mlpack-bot
Copy link

mlpack-bot bot commented Dec 17, 2020

Hello there! Thanks for your contribution. I see that this is your first contribution to mlpack. If you'd like to add your name to the list of contributors in COPYRIGHT.txt and you haven't already, please feel free to push a change to this PR---or, if it gets merged before you can, feel free to open another PR.

In addition, if you'd like some stickers to put on your laptop, I'd be happy to help get them in the mail for you. Just send an email with your physical mailing address to stickers@mlpack.org, and then one of the mlpack maintainers will put some stickers in an envelope for you. It may take a few weeks to get them, depending on your location. 👍

Copy link
Member

@shrit shrit left a comment

Choose a reason for hiding this comment

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

Everything looks good to me, thanks for the contributions 👍

@ayushsingh11
Copy link
Contributor Author

Thanks @shrit for the review.

@shrit
Copy link
Member

shrit commented Dec 21, 2020

@ayushsingh11 There is a conflict in the history file, can you resolve the conflict so I can merge this pull request?

@ayushsingh11
Copy link
Contributor Author

@shrit resolved the conflict.

@zoq zoq merged commit 7da22c5 into mlpack:master Dec 23, 2020
@zoq
Copy link
Member

zoq commented Dec 23, 2020

Thanks again for the contribution.

This was referenced Oct 14, 2022
@rcurtin rcurtin mentioned this pull request Oct 23, 2022
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.

None yet

5 participants