Added Triplet margin loss function. #2208

prince776 · 2020-02-14T18:31:21Z

Hi, I've added Triplet margin loss function in reference to issue #2200 . Please tell me if any changes are required.

Since triplet loss takes three matrices: anchor, positive and negative, its forward and backward functions are slightly different than other previous functions which took only input and targe.

kartikdutt18

Implementation looks fine, I think positiveType and negativeType would always be same to support matrix operations however I'm not sure at this point. I'll take a closer look at the code first thing tomorrow. Nice work.

kartikdutt18 · 2020-02-14T18:47:29Z

src/mlpack/methods/ann/loss_functions/triplet_margin_loss.hpp

+  OutputDataType& OutputParameter() { return outputParameter; }
+
+  //! Get the output parameter.
+  double& Margin() const { return margin; }


There wont be & here. Thanks.

kartikdutt18

Some small style issues. Rest of it looks great. Thanks for all the work.

src/mlpack/methods/ann/loss_functions/triplet_margin_loss.hpp

kartikdutt18 · 2020-02-15T03:45:58Z

src/mlpack/methods/ann/loss_functions/triplet_margin_loss.hpp

+   * @param input The propagated input activation.
+   * @param target The target vector.
+   */
+template<typename AnchorType, typename PositiveType, typename NegativeType>


This should align with * from the comments. This also true in other places as well.

Let's keep it like this only, no need to reduce templates, I checked couple of other programs they use different template even though they must have same datatype.

src/mlpack/methods/ann/loss_functions/triplet_margin_loss.hpp

kartikdutt18 · 2020-02-15T03:55:39Z

src/mlpack/methods/ann/loss_functions/triplet_margin_loss_impl.hpp

+    const PositiveType&& positive,
+    const NegativeType&& negative,
+    OutputType&& output
+    )


Could you place this bracket in line 50.

Could you also resolve this.

kartikdutt18 · 2020-02-15T03:56:15Z

src/mlpack/methods/ann/loss_functions/triplet_margin_loss_impl.hpp

+    Archive& ar,
+    const unsigned int /* version */)
+{
+  ar & BOOST_SERIALIZATION_NVP(margin);


Nice, most people missed that.

kartikdutt18

Rest of it looks fine. However a member will provide better insight. Thanks.

kartikdutt18 · 2020-02-15T17:28:33Z

src/mlpack/methods/ann/loss_functions/triplet_margin_loss.hpp

+  /**
+   * Create the TripletMarginLoss object with Hyperparameter margin.
+   * Hyperparameter margin defines the minimum numeric value by which distance
+   * between Anchor and Negative sample should be higher than the distance between


Could you try doing the following :

minimum value by which the distance between Anchor and Negative sample must exceed the distance between ...
This would increase readability.
Or if you come up with something that is better worded use that.

kartikdutt18 · 2020-02-15T17:28:52Z

src/mlpack/methods/ann/loss_functions/triplet_margin_loss_impl.hpp

+    const PositiveType&& positive,
+    const NegativeType&& negative,
+    OutputType&& output
+    )


Could you also resolve this.

rcurtin · 2020-03-12T14:56:36Z

This code touches the ANN code, and there was recently a big refactoring of the ANN code in #2259, so be sure to merge the master branch into your branch here to make sure that nothing will fail if this PR is merged. 👍 (I'm pasting this message into all possibly relevant PRs.)

favre49 · 2020-03-18T11:30:55Z

src/mlpack/tests/loss_functions_test.cpp

+  anchor = arma::mat("2 3 5");
+  positive = arma::mat("10 12 13");
+  negative = arma::mat("4 5 7");
+  double error = module.Forward(std::move(anchor),


I don't think we need std::move anywhere in this file anymore

favre49 · 2020-03-18T11:31:09Z

src/mlpack/tests/loss_functions_test.cpp

+  arma::mat anchor, positive, negative, output;
+  TripletMarginLoss<> module;
+
+  // Test the Forward function on a user generator input and compare it against


user generated

src/mlpack/methods/ann/loss_functions/triplet_margin_loss.hpp

kartikdutt18 · 2020-03-26T03:18:18Z

Hey Prince, There is a change (maybe), Instead of taking three inputs what do you think about taking two concatenated inputs?
For more details refer to this link.

kartikdutt18 · 2020-04-02T08:01:46Z

Hey there, This code touches the loss function portion of mlpack's codebase. Recently #2339 was merged where the switch was made to templated return type instead of just using double. So before we merge this, kindly make the changes for the same. I'm pasting this in all relevant PRs. Thanks a lot 👍

favre49

Could you also add to HISTORY.md? Thanks for keeping up with all the repo changes by the way.

favre49 · 2020-04-03T12:34:54Z

src/mlpack/methods/ann/loss_functions/triplet_margin_loss.hpp

+namespace ann /** Artificial Neural Network. */ {
+
+/**
+ * The TripletMarginLoss function's objective is that the distance between


I'm not sure I can understand this. Perhaps you could take cues from the description from Wikipedia or something?

favre49 · 2020-04-03T12:35:27Z

src/mlpack/methods/ann/loss_functions/triplet_margin_loss.hpp

+ * For more information, see the following paper.
+ *
+ * @code
+ * @article{Janocha2017


It seems you have put down the same paper twice

favre49 · 2020-04-03T12:37:54Z

src/mlpack/tests/loss_functions_test.cpp

+  positive = arma::mat("10 12 13");
+  negative = arma::mat("4 5 7");
+
+  input = {


I think we can put this in a single line?

I did it so it is easy to see it's a matrix of dimensions 2x3. I can change that if you say.

favre49

I think these will be my last comments before approval. Could you merge and correct HISTORY.md?

favre49 · 2020-04-09T05:42:22Z

src/mlpack/methods/ann/loss_functions/triplet_margin_loss.hpp

+   * Computes the Triplet Margin Loss function.
+   *
+   * @param input The propagated input activation. It should be
+   * concatenated anchor and positive samples.


I think you'll need to tab here

favre49 · 2020-04-09T05:42:33Z

src/mlpack/methods/ann/loss_functions/triplet_margin_loss.hpp

+   * Ordinary feed backward pass of a neural network.
+   *
+   * @param input The propagated input activation. It should be
+   * concatenated anchor and positive samples.


You'll need to tab here as well

favre49 · 2020-04-09T05:45:26Z

src/mlpack/methods/ann/loss_functions/triplet_margin_loss_impl.hpp

+    const InputType& input,
+    const TargetType& target)
+{
+  arma::mat anchor = input.submat(0, 0, input.n_rows / 2 - 1, input.n_cols - 1);


Correct me if I'm wrong, but will this not have to be InputType in case the input is arma::fmat?

mlpack-bot · 2020-05-09T14:06:18Z

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions! 👍

ayushsingh11 · 2020-11-24T04:16:47Z

Hi @prince776 and @favre49, I would like to contribute in addition of Triplet margin loss to the repository.
But I am a little confused, all the recommendations above have been incorporated, right? How can I contribute?

Added Triplet margin loss function.

13632ae

mlpack-bot bot added s: needs review s: unanswered s: unlabeled labels Feb 14, 2020

Style fixes

3963042

kartikdutt18 reviewed Feb 14, 2020

View reviewed changes

Fixed getter for margin

0290a6b

zoq added c: methods t: added feature and removed s: unanswered s: unlabeled labels Feb 14, 2020

kartikdutt18 reviewed Feb 15, 2020

View reviewed changes

prince776 added 2 commits February 15, 2020 13:10

Cited the FaceNet paper and minor style fixes

c798969

Cited the FaceNet paper and minor style fixes

bc26222

kartikdutt18 reviewed Feb 15, 2020

View reviewed changes

Minor fixes

9e5bba6

Hemal-Mamtora mentioned this pull request Mar 12, 2020

Triplet Loss Function for Siamese Network (issue #1699) #1868

Closed

Refactored to avoid r value references

1ff498a

favre49 reviewed Mar 18, 2020

View reviewed changes

minor changes

0fe31e0

prince776 added 2 commits March 27, 2020 23:33

Fixed Forward() and Backward() to match Layer API

f7f9279

style fix

7cc8257

templated return type of Forward() method

c17b652

favre49 reviewed Apr 3, 2020

View reviewed changes

Fixed comments. Updated history.md

7c24b1c

favre49 reviewed Apr 9, 2020

View reviewed changes

minor fixes

4d75977

prince776 added 3 commits April 9, 2020 19:05

resolving merge conflicts

af47685

Merge branch 'master' into add-triplet-margin-loss

21d5248

Added back the things removed for merging

5f02fad

mlpack-bot bot added the s: stale label May 9, 2020

mlpack-bot bot closed this May 16, 2020

ayushsingh11 mentioned this pull request Nov 24, 2020

Adding All Loss Functions #2200

Closed

ayushsingh11 mentioned this pull request Dec 12, 2020

Adding triplet margin loss function #2762

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Triplet margin loss function. #2208

Added Triplet margin loss function. #2208

prince776 commented Feb 14, 2020

kartikdutt18 left a comment

kartikdutt18 Feb 14, 2020

kartikdutt18 left a comment

kartikdutt18 Feb 15, 2020

kartikdutt18 Feb 15, 2020

kartikdutt18 Feb 15, 2020

kartikdutt18 Feb 15, 2020

kartikdutt18 Feb 15, 2020

kartikdutt18 left a comment •

edited

kartikdutt18 Feb 15, 2020

kartikdutt18 Feb 15, 2020

rcurtin commented Mar 12, 2020

favre49 Mar 18, 2020

favre49 Mar 18, 2020

kartikdutt18 commented Mar 26, 2020

kartikdutt18 commented Apr 2, 2020

favre49 left a comment

favre49 Apr 3, 2020

favre49 Apr 3, 2020

favre49 Apr 3, 2020

prince776 Apr 3, 2020 •

edited

favre49 left a comment

favre49 Apr 9, 2020

favre49 Apr 9, 2020

favre49 Apr 9, 2020

mlpack-bot bot commented May 9, 2020

ayushsingh11 commented Nov 24, 2020

Added Triplet margin loss function. #2208

Added Triplet margin loss function. #2208

Conversation

prince776 commented Feb 14, 2020

kartikdutt18 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kartikdutt18 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kartikdutt18 left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rcurtin commented Mar 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kartikdutt18 commented Mar 26, 2020

kartikdutt18 commented Apr 2, 2020

favre49 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prince776 Apr 3, 2020 • edited

Choose a reason for hiding this comment

favre49 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlpack-bot bot commented May 9, 2020

ayushsingh11 commented Nov 24, 2020

kartikdutt18 left a comment •

edited

prince776 Apr 3, 2020 •

edited