Implemented margin ranking loss #2264

AndreiMihalea · 2020-03-07T10:42:28Z

Implemented the margin ranking loss as proposed in the issue #2253 I opened. Can also be related to #2200

mlpack-bot · 2020-03-07T10:42:30Z

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

AndreiMihalea · 2020-03-07T10:54:49Z

Forgot to pull the latest update. Will reopen when done.

mlpack-bot · 2020-03-07T11:16:52Z

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

kartikdutt18

Hi @AndreiMihalea, Thanks for taking the time to work on this. This looks good to me. Just few minor comments. 👍
Thanks.

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

src/mlpack/tests/loss_functions_test.cpp

…n for input / output

rcurtin · 2020-03-12T14:54:14Z

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.)

Deleted .vscode

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

zoq · 2020-03-17T15:10:28Z

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

+    typename ThirdInputType,
+    typename OutputType
+  >
+  void Backward(const FirstInputType& x1,


See comment above.

Modified with input1 / input2 and target.

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

…ranking-loss

…r/setter

…into marginranking-loss

kartikdutt18 · 2020-03-26T18:02:41Z

Hey Andrie, No rush, just wanted your opinion on it. I think @zoq will provide better insight on how you should proceed, till then I would suggest not making any changes incase the current approach makes more sense.

kartikdutt18 · 2020-03-28T07:14:48Z

As discussed on IRC, I think it's okay to move forward with changes suggested (Take concatenated input), I think that makes the most sense.

AndreiMihalea · 2020-03-28T18:45:02Z

As discussed on IRC, I think it's okay to move forward with changes suggested (Take concatenated input), I think that makes the most sense.

I modified the implementation. Hope it's ok now.

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

favre49

Could you also add to HISTORY.md?

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

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

Co-Authored-By: favre49 <40389657+favre49@users.noreply.github.com>

favre49

This looks good to me, thanks for keeping up with the reviews :)

kartikdutt18 · 2020-04-02T07:54:03Z

Looks good to me as well. Just one last thing, recently we merged #2339, where we templated the return type instead of directly using double. @AndreiMihalea, do you mind changing to that to conform with the rest of the code base. I will post a message in each function that has anything to do with loss functions. I hope that's okay. Thanks a lot. 👍

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

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 👍

AndreiMihalea · 2020-04-02T15:02:54Z

Looks good to me as well. Just one last thing, recently we merged #2339, where we templated the return type instead of directly using double. @AndreiMihalea, do you mind changing to that to conform with the rest of the code base. I will post a message in each function that has anything to do with loss functions. I hope that's okay. Thanks a lot.

Modified it accordingly. Thanks.

kartikdutt18 · 2020-04-02T15:03:59Z

Great, No more comments from my side. Thanks a lot for the awesome work. 👍

mlpack-bot

Second approval provided automatically after 24 hours. 👍

mlpack-bot · 2020-04-03T05:14:31Z

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 src/mlpack/core.hpp and 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. 👍

favre49 · 2020-04-05T05:32:18Z

All the tests are passing, I think we can merge this now. Thanks for keeping up with the repo changes, and thanks @kartikdutt18 for pointing it out.

Implemented margin ranking loss

491e030

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

Pulled the latest update to loss_functions_test

a820fd9

AndreiMihalea closed this Mar 7, 2020

AndreiMihalea added 3 commits March 7, 2020 12:57

Added the margin ranking loss import

fa5f2e9

Solved some style checks

6f72b58

Solved conflict

83c73e0

AndreiMihalea reopened this Mar 7, 2020

Solved the rest of style checks

e0b76b7

kartikdutt18 reviewed Mar 7, 2020

View reviewed changes

Added description for hyperparameter margin and a template descriptio…

9949edc

…n for input / output

birm added c: methods t: added feature and removed s: unlabeled labels Mar 7, 2020

retrigger checks; see if python27 passes now

c600714

AndreiMihalea added 3 commits March 17, 2020 16:19

Removed rvalues references

7233ca1

Removed rvalues references

5559e4f

Delete settings.json

8e34c3a

Deleted .vscode

zoq reviewed Mar 17, 2020

View reviewed changes

zoq removed the s: unanswered label Mar 17, 2020

AndreiMihalea added 4 commits March 17, 2020 17:21

Merge branch 'master' of https://github.com/mlpack/mlpack into margin…

310b311

…ranking-loss

Modified the parameter names and the comments before the margin gette…

bd892fe

…r/setter

Merge branch 'marginranking-loss' of github.com:AndreiMihalea/mlpack …

f526d66

…into marginranking-loss

Modified y to target

32d6d89

AndreiMihalea added 3 commits March 28, 2020 13:18

Merge remote-tracking branch 'upstream/master' into marginranking-loss

6d0aaeb

Started working on changing the input in margin ranking loss

90898db

Merged the two inputs into a single one for the margin ranking loss

c6c2f9e

prince776 reviewed Mar 28, 2020

View reviewed changes

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

AndreiMihalea added 2 commits March 28, 2020 22:17

Solved type in input variable name

5c84271

Rebuild

dcc9fcb

favre49 reviewed Mar 31, 2020

View reviewed changes

AndreiMihalea and others added 4 commits March 31, 2020 18:16

Update src/mlpack/methods/ann/loss_functions/margin_ranking_loss.hpp

d3068f0

Co-Authored-By: favre49 <40389657+favre49@users.noreply.github.com>

Update src/mlpack/methods/ann/loss_functions/margin_ranking_loss.hpp

3208350

Co-Authored-By: favre49 <40389657+favre49@users.noreply.github.com>

Update src/mlpack/methods/ann/loss_functions/margin_ranking_loss.hpp

13585be

Co-Authored-By: favre49 <40389657+favre49@users.noreply.github.com>

Moved the description and renamed variables

fd5e1b9

favre49 approved these changes Apr 2, 2020

View reviewed changes

Changed return type accordingly to mlpack#2339

3c2c3ce

AndreiMihalea added 2 commits April 2, 2020 21:27

Rebuild

b84745a

Rebuild

0f31459

mlpack-bot bot approved these changes Apr 3, 2020

View reviewed changes

mlpack-bot bot removed the s: needs review label Apr 3, 2020

AndreiMihalea added 3 commits April 3, 2020 11:11

Retrigger tests

8ca0543

Rebuild

51c05e6

Retrigger tests

c57eeeb

favre49 merged commit 9d75357 into mlpack:master Apr 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented margin ranking loss #2264

Implemented margin ranking loss #2264

AndreiMihalea commented Mar 7, 2020

mlpack-bot bot commented Mar 7, 2020

AndreiMihalea commented Mar 7, 2020

mlpack-bot bot commented Mar 7, 2020

kartikdutt18 left a comment •

edited

rcurtin commented Mar 12, 2020

zoq Mar 17, 2020

AndreiMihalea Mar 17, 2020

kartikdutt18 commented Mar 26, 2020

kartikdutt18 commented Mar 28, 2020

AndreiMihalea commented Mar 28, 2020

favre49 left a comment

favre49 left a comment

kartikdutt18 commented Apr 2, 2020 •

edited

kartikdutt18 commented Apr 2, 2020

AndreiMihalea commented Apr 2, 2020

kartikdutt18 commented Apr 2, 2020

mlpack-bot bot left a comment

mlpack-bot bot commented Apr 3, 2020

favre49 commented Apr 5, 2020

Implemented margin ranking loss #2264

Implemented margin ranking loss #2264

Conversation

AndreiMihalea commented Mar 7, 2020

mlpack-bot bot commented Mar 7, 2020

AndreiMihalea commented Mar 7, 2020

mlpack-bot bot commented Mar 7, 2020

kartikdutt18 left a comment • edited

Choose a reason for hiding this comment

rcurtin commented Mar 12, 2020

zoq Mar 17, 2020

Choose a reason for hiding this comment

AndreiMihalea Mar 17, 2020

Choose a reason for hiding this comment

kartikdutt18 commented Mar 26, 2020

kartikdutt18 commented Mar 28, 2020

AndreiMihalea commented Mar 28, 2020

favre49 left a comment

Choose a reason for hiding this comment

favre49 left a comment

Choose a reason for hiding this comment

kartikdutt18 commented Apr 2, 2020 • edited

kartikdutt18 commented Apr 2, 2020

AndreiMihalea commented Apr 2, 2020

kartikdutt18 commented Apr 2, 2020

mlpack-bot bot left a comment

Choose a reason for hiding this comment

mlpack-bot bot commented Apr 3, 2020

favre49 commented Apr 5, 2020

kartikdutt18 left a comment •

edited

kartikdutt18 commented Apr 2, 2020 •

edited