Adding MultiLabel SoftMargin Loss #2345

iamshnoo · 2020-03-31T09:21:43Z

In this PR, I am adding the Multi Label Soft Margin Loss function.
Here is a link to a Jupyter notebook to verify the outcomes presented in the test cases added.

…rgin_loss

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

ojhalakshya · 2020-04-01T11:48:44Z

Hi @iamshnoo
could you also add tests for this loss function in the

loss_functions_test.cpp

Thanks

kartikdutt18 · 2020-04-01T17:46:25Z

Hmm, That is strange.

ojhalakshya · 2020-04-01T17:47:45Z

I have got an idea about this.
You can see a different type of tab placed just at the beginning of the line where you are getting ambiguity in different editors.
I think if you just remove that and add normal spaces/tab you will get same view in every editor.

ojhalakshya · 2020-04-01T17:49:01Z

You can zoom in to the pic to find the tab space I am talking about.

kartikdutt18 · 2020-04-01T17:49:12Z

Agreed and you set tab = 2 spaces (this is what mlpack uses).

iamshnoo · 2020-04-01T17:55:13Z

Agreed and you set tab = 2 spaces (this is what mlpack uses).

Yep, that's what I had done initially before the review pointed out the misalignment. Then I tried changing it to 8 spaces and found the problem persisted in a different format.

I have got an idea about this.
You can see a different type of tab placed just at the beginning of the line where you are getting ambiguity in different editors.
I think if you just remove that and add normal spaces/tab you will get same view in every editor.

Yep, I am gonna try this out and see. Thanks for the suggestion.

ojhalakshya · 2020-04-01T17:56:17Z

Happy to help!
Keep up the great work :)

kartikdutt18 · 2020-04-02T09:13:59Z

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 👍

…rgin_loss

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

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

zoq · 2020-05-31T19:02:23Z

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

+    reduction(reduction),
+    weighted(false)
+{
+  if (weights.n_elem)


Might be easier to understand what happens here, if we provide a second constructor to set the weights. Let me know what you think.

Hi zoq, if you look at commit f298c7a, on May 5th, there I re-designed the API of this function to the current format after a long discussion above with @kartikdutt18. The conclusion of that discussion can be seen in this comment.

So, I think initially I had done what you are suggesting (let me know if you mean otherwise) using a overloaded constructor to set the weights. But then Kartik wanted me to design the API like this.

I am okay with both. Even though I would prefer the approach with an overloaded constructor

Hmm, I am guessing the other constructor would also have weights as param and probably a reduction also as param. It might ve redundant to have two constructors with same parameters. We would only have one forward function so we would have take into account both cases. For initialization we check if the vector is empty, if yes, we set reset to true and in forward pass we initialize the weights equal to input features and we set reset to false so we don't initialize every iteration. If weights are passed then we set reset to false and copy weights so no initialization in forward pass. Maybe I missed something, let me know what you think.
Regards.

In terms of API wouldn't an overloaded constructor be redundant. I am guessing one constructor will be with parameter reduction and one with both reduction and weights parameter. We can simply have one constructor and set weights to default value and in forward pass set them to 1 only in the first pass. This allows us to have a forward function which doesn't depend on which constructors object we are dealing with. I have left a comment explaining something similar.

zoq · 2020-05-31T19:03:12Z

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

+    const InputType& input, const TargetType& target)
+{
+  if (!weighted)
+    classWeights.ones(1, input.n_cols);


This could go into the other constructor, see comment above, that way we don't set this in each call of forward.

Totally agree! Detailed comment above.

I thought I suggested using a reset parameter so this initialized only in the first pass. I am using my phone so sorry I can't take a look at the code. Similar thing is done in adaptive pooling layers. Regards.

Co-authored-by: Marcus Edel <marcus.edel@fu-berlin.de>

kartikdutt18 · 2020-06-01T03:57:22Z

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

+  if (!weighted)
+    classWeights.ones(1, input.n_cols);


Hmm, The weights will be initialised every time in forward pass here, You should set weighted to true here to prevent that from happening. As shown here, we use reset to prevent weights from being initialized every time i.e. If user has not passed weights they will be initialized once in forward pass.

Agreed! This was something I missed completely. Corrected in the commit 77463d8.
Thanks to both zoq and you for pointing this out.

mlpack-bot · 2020-07-01T09:58:08Z

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! 👍

iamshnoo · 2020-07-01T11:54:48Z

Let's keep this open please!

mlpack-bot · 2020-07-31T12:07:07Z

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! 👍

rcurtin · 2020-10-10T14:17:10Z

Hey @iamshnoo, just wanted to check on this one---I see it's approved, but it seems like there is still work to do before it's ready for merge?

iamshnoo · 2020-10-10T14:28:12Z

I will pull in the recent upstream changes to this branch in the next commit (hopefully in the next couple of days). Other than the merge conflicts and changing boost to catch2, there probably won't be too many other changes to make. Thanks for the reminder.

jeffin143 · 2021-02-22T14:07:18Z

Hi @iamshnoo I solved the merge conflicts and also migrated test and also the serialisation part. If there is nothing left from your side we can go ahead with the merge

Since test failure are random

cc @zoq @rcurtin

iamshnoo · 2021-02-22T14:51:50Z

Thank you for updating the PR! I feel it's ready from my end. Nothing more to add.

iamshnoo added 4 commits March 25, 2020 00:03

Initial work. Forward/ Backward left.

e1eb983

Merge remote-tracking branch 'upstream/master' into multilabel_softma…

7269357

…rgin_loss

Completed Forward and Backward. Tests left.

a3c3bb7

Merge remote-tracking branch 'upstream/master' into multilabel_softma…

92af440

…rgin_loss

mlpack-bot bot added s: needs review s: unanswered s: unlabeled labels Mar 31, 2020

Fix style errors.

b2604b0

favre49 added c: methods t: added feature and removed s: unanswered s: unlabeled labels Mar 31, 2020

ojhalakshya reviewed Apr 1, 2020

View reviewed changes

src/mlpack/methods/ann/loss_functions/multilabel_softmargin_loss.hpp Outdated Show resolved Hide resolved

ojhalakshya reviewed Apr 1, 2020

View reviewed changes

src/mlpack/methods/ann/loss_functions/multilabel_softmargin_loss.hpp Outdated Show resolved Hide resolved

src/mlpack/methods/ann/loss_functions/multilabel_softmargin_loss.hpp Outdated Show resolved Hide resolved

iamshnoo and others added 4 commits April 1, 2020 21:04

Fix weird indentation issue.

fc6335d

Manually fix the indentation in browser.

b6275ec

Shift line 39 by 4 spaces to left

b42c360

Shift line 39 by 2 spaces to the right

90c48ae

This comment has been minimized.

Sign in to view

ojhalakshya mentioned this pull request Apr 2, 2020

Adding All Loss Functions #2200

Closed

iamshnoo added 2 commits April 4, 2020 19:47

Corrected errors. Wrote test cases.

c0a3ef6

Merge remote-tracking branch 'upstream/master' into multilabel_softma…

194f3ed

…rgin_loss

zoq reviewed May 31, 2020

View reviewed changes

Apply suggestions from code review

6340e64

Co-authored-by: Marcus Edel <marcus.edel@fu-berlin.de>

kartikdutt18 reviewed Jun 1, 2020

View reviewed changes

Fix bug in Forward().

77463d8

mlpack-bot bot added the s: stale label Jul 1, 2020

mlpack-bot bot removed the s: stale label Jul 1, 2020

mlpack-bot bot added the s: stale label Jul 31, 2020

sreenikSS added the s: keep open label Jul 31, 2020

mlpack-bot bot removed the s: stale label Jul 31, 2020

jeffin143 force-pushed the multilabel_softmargin_loss branch from 5933dff to 77463d8 Compare February 21, 2021 17:51

Merge upstream

733db6a

jeffin143 force-pushed the multilabel_softmargin_loss branch 2 times, most recently from f36c858 to e7ef286 Compare February 21, 2021 18:41

Merge upstream

180f917

jeffin143 force-pushed the multilabel_softmargin_loss branch from e7ef286 to 180f917 Compare February 22, 2021 07:11

zoq added this to Need Review in PR Tracking Feb 23, 2021

Merge branch 'master' into multilabel_softmargin_loss

ebaa6c3

shrit added this to the mlpack 4.0.0 milestone Apr 18, 2021

Merge branch 'master' into multilabel_softmargin_loss

bf61e97

jeffin143 merged commit 3f82130 into mlpack:master Aug 14, 2021

This was referenced Oct 14, 2022

Release version 4.0.0 #3285

Closed

Release version 4.0.0 #3286

Closed

rcurtin mentioned this pull request Oct 23, 2022

Release version 4.0.0 #3293

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding MultiLabel SoftMargin Loss #2345

Adding MultiLabel SoftMargin Loss #2345

iamshnoo commented Mar 31, 2020 •

edited

ojhalakshya commented Apr 1, 2020 •

edited

This comment has been minimized.

kartikdutt18 commented Apr 1, 2020

ojhalakshya commented Apr 1, 2020

ojhalakshya commented Apr 1, 2020

kartikdutt18 commented Apr 1, 2020

iamshnoo commented Apr 1, 2020

ojhalakshya commented Apr 1, 2020

kartikdutt18 commented Apr 2, 2020

zoq May 31, 2020

iamshnoo May 31, 2020

kartikdutt18 May 31, 2020

kartikdutt18 Jun 1, 2020

zoq May 31, 2020

iamshnoo May 31, 2020

kartikdutt18 May 31, 2020

kartikdutt18 Jun 1, 2020

iamshnoo Jun 1, 2020

mlpack-bot bot commented Jul 1, 2020

iamshnoo commented Jul 1, 2020

mlpack-bot bot commented Jul 31, 2020

rcurtin commented Oct 10, 2020

iamshnoo commented Oct 10, 2020

jeffin143 commented Feb 22, 2021

iamshnoo commented Feb 22, 2021

Adding MultiLabel SoftMargin Loss #2345

Adding MultiLabel SoftMargin Loss #2345

Conversation

iamshnoo commented Mar 31, 2020 • edited

ojhalakshya commented Apr 1, 2020 • edited

This comment has been minimized.

kartikdutt18 commented Apr 1, 2020

ojhalakshya commented Apr 1, 2020

ojhalakshya commented Apr 1, 2020

kartikdutt18 commented Apr 1, 2020

iamshnoo commented Apr 1, 2020

ojhalakshya commented Apr 1, 2020

kartikdutt18 commented Apr 2, 2020

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

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 Jul 1, 2020

iamshnoo commented Jul 1, 2020

mlpack-bot bot commented Jul 31, 2020

rcurtin commented Oct 10, 2020

iamshnoo commented Oct 10, 2020

jeffin143 commented Feb 22, 2021

iamshnoo commented Feb 22, 2021

iamshnoo commented Mar 31, 2020 •

edited

ojhalakshya commented Apr 1, 2020 •

edited