Adding Hinge embedding loss function by ojhalakshya · Pull Request #2229 · mlpack/mlpack

ojhalakshya · 2020-02-22T17:53:48Z

Hello,
This loss function in regarding to my previous issue #2200 in which I opened up for help from all guys working hard to contribute. There I mentioned many loss functions which are now being implemented.
I also am trying to implement the Hinge Embedding Loss Function now.
It can be referred from both pytorch and tensorflow.
Hope to contribute more.
Thanks.

kartikdutt18

Some small changes, other than that it looks fine.

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

src/mlpack/tests/loss_functions_test.cpp

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

birm

Great work! I've added suggestions based upon @zoq 's comments, just to make that a bit easier. I think with that, the addition to HISTORY.md, and the merge conflict fix in the testing file, you should be good!

kartikdutt18

One really minor style issue.

src/mlpack/tests/loss_functions_test.cpp

birm · 2020-03-03T14:07:29Z

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

+    const TargetType&& target,
+    OutputType&& output)
+{
+  for (size_t i = 0; i < input.n_elem; i++)


I wonder if we can avoid the for loop here too?

I think this can be done ((1 - target(i) * input(i)) >= 0) % -target .

I have implemented the changes, kindly have a look when you get time.
Thanks.

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

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

ojhalakshya · 2020-03-13T04:59:21Z

Hey @zoq @rcurtin
I am not able to get a hold of this fail, been working to fix this from like 5 days but still no clue.
Apart from this error I have pretty much everything covered up.
Can you help me a bit in this?
The error occurs in the loss_function_test.cpp line no 522 and it generates this error when I use

HingeEmbeddingLoss<> module;

and when I just remove the <>

HingeEmbeddingLoss module;

then the template error doesn't show and only the last 4 errors show up.

I am not sure with the last 4 errors in both the cases as Forward and Backward function are defined correctly and when I build without the loss_function_test.cpp file the build works 100%.

Thanks.

rcurtin · 2020-03-13T11:42:05Z

You can see in the error message that it is referencing struct LossFunctionsTest::HingeEmbeddingLoss. Do you have a class or struct called HingeEmbeddingLoss defined locally in the loss_functions_test.cpp file? (This might happen if, for instance, you've named one of the tests HingeEmbeddingLoss.)

ojhalakshya · 2020-03-13T12:28:30Z

Hi @rcurtin
Thanks for the reply and the solution also, I was able to fix the same because of it.
Can you please also have a look at the whole pr so if it is fine.
Thanks.

HISTORY.md

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

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

favre49

Everything seems good to me :) Good work!

kartikdutt18 · 2020-03-15T04:32:12Z

Yes I am, You will also need to do the same for this layer / function as well. Thanks for all the work though 👍 .

ojhalakshya · 2020-03-16T07:45:22Z

Hey @birm
I have made some changes, I think now everything is fine. I have attached the build and test results.
Can you see into this when you get time.
Thanks.

ojhalakshya · 2020-03-16T14:55:18Z

@rcurtin
Can you kindly have a look when you get time. Although azure pipelines are not working, I did build this atleast 4 times with the tests also and I think it works fine now.
I wanted to know if it works fine in other systems also.
@kartikdutt18 can you also try this one if comfortable.
Thanks.

birm · 2020-03-17T01:14:02Z

I think that azure is working again; can you trick github into running those tests?

* completed hardshrink_function.hpp * resetting master * complete hardsrhink_function.hpp * activation_functions_test.cpp * cmake changes * base layer changes * style correction * starting implementing hard shrink as layer * new changes * changes in tests * inv changes * test changes * test changes * deleting prev function * test style changes * more style changes * comment changes * minor changes * comment corrections hard shrink * minor changes * dummy commit * Style fix for parameter lambda * hardshrink.hpp to hardshrink_impl.hpp fn function shift * style fix Co-authored-by: Marcus Edel <marcus.edel@fu-berlin.de> * completed hardshrink_function.hpp * resetting master * complete hardsrhink_function.hpp * activation_functions_test.cpp * cmake changes * base layer changes * style correction * starting implementing hard shrink as layer * new changes * changes in tests * inv changes * test changes * test changes * deleting prev function * test style changes * more style changes * comment changes * minor changes * comment corrections hard shrink * minor changes * dummy commit * Style fix for parameter lambda * hardshrink.hpp to hardshrink_impl.hpp fn function shift * style fix Co-authored-by: Marcus Edel <marcus.edel@fu-berlin.de> dummy commit Redirected link on NUMfocus logo test bugs fixing rebasing branch

edited copyright.txt changes in History.md bug fix

minor change

ojhalakshya · 2020-03-17T04:33:55Z

I am not sure about the LoadSaveTest in ctest in which the macOS Plain failed, as I can see all builds and all the corresponding tests had passed successfully see here.
@kartikdutt18 can you verify if this is good to go?
Also lets see if other tests are working good, lately there have been problems with azure.
Thanks.

birm

Thanks and great addition!
I don't think there's any relevance in the loadsave test. I'll add an issue for that.

kartikdutt18 · 2020-03-17T05:30:08Z

Yes it is good to go, The load save test fails because of an armadillo dependency issue and so it's not relevant to your PR. Thanks for all the work 👍.

kartikdutt18 · 2020-03-18T03:08:06Z

Hey @ojhalakshya, Could you resolve the merge conflicts. Thanks a lot 👍

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

Or if you could do a rebase before we merge this, that would be really great. I think we have fixed every possible build so all tests should pass. Thanks again 👍.

ojhalakshya · 2020-03-18T03:12:00Z

Also if you could do a rebase before we merge this, that would be really great. I think we have fixed every possible build so all tests should pass. Thanks again .

I think its up to date now after resolving the conflict.

kartikdutt18 · 2020-03-18T03:13:59Z

Agreed, Thanks again. :)

birm · 2020-03-18T03:15:12Z

@kartikdutt18 do you have any other review additions or conditions other than the tests passing?

kartikdutt18 · 2020-03-18T03:17:02Z

No, it's good to go for me 👍. Once the tests pass I think we can merge this, I think will since they did yesterday but might be better to wait for them anyway.

ojhalakshya · 2020-03-18T04:07:30Z

@kartikdutt18 @birm
Thanks for the review. :)

kartikdutt18 · 2020-03-18T04:10:29Z

Really nice to see green builds. Thanks a lot @ojhalakshya for the great work.

ojhalakshya changed the title ~~Hinge embedding loss function~~ Adding Hinge embedding loss function Feb 22, 2020

kartikdutt18 reviewed Feb 22, 2020

View reviewed changes

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

src/mlpack/tests/loss_functions_test.cpp Outdated Show resolved Hide resolved

src/mlpack/tests/loss_functions_test.cpp Outdated Show resolved Hide resolved

zoq added c: methods s: needs review t: added feature labels Feb 22, 2020

zoq reviewed Feb 22, 2020

View reviewed changes

birm requested changes Feb 22, 2020

View reviewed changes

kartikdutt18 reviewed Feb 23, 2020

View reviewed changes

src/mlpack/tests/loss_functions_test.cpp Outdated Show resolved Hide resolved

ojhalakshya requested a review from zoq February 24, 2020 18:54

birm reviewed Mar 3, 2020

View reviewed changes

ojhalakshya closed this Mar 8, 2020

ojhalakshya force-pushed the Hinge-Embedding-Loss-Function branch from 2b9bf44 to 0d8c3a0 Compare March 8, 2020 04:09

ojhalakshya reopened this Mar 10, 2020

ojhalakshya force-pushed the Hinge-Embedding-Loss-Function branch 4 times, most recently from 0b6af9b to 08a7099 Compare March 11, 2020 10:41

ojhalakshya force-pushed the Hinge-Embedding-Loss-Function branch from ff2a9fc to 6f1971a Compare March 13, 2020 04:38

favre49 reviewed Mar 13, 2020

View reviewed changes

ojhalakshya force-pushed the Hinge-Embedding-Loss-Function branch 2 times, most recently from f9ddb47 to a0dd14e Compare March 13, 2020 21:11

ojhalakshya requested review from birm and favre49 March 13, 2020 21:12

ojhalakshya force-pushed the Hinge-Embedding-Loss-Function branch 2 times, most recently from f600880 to ad3f83d Compare March 14, 2020 04:43

favre49 approved these changes Mar 14, 2020

View reviewed changes

ojhalakshya force-pushed the Hinge-Embedding-Loss-Function branch from ad3f83d to fdcd685 Compare March 16, 2020 07:02

ojhalakshya force-pushed the Hinge-Embedding-Loss-Function branch from fdcd685 to 5cc4d0f Compare March 16, 2020 08:26

ojhalakshya requested a review from birm March 16, 2020 08:27

ojhalakshya and others added 3 commits March 17, 2020 08:45

rvalue refactor corrections

ab800f7

edited copyright.txt changes in History.md bug fix

bug fixes

b37f3c7

minor change

ojhalakshya force-pushed the Hinge-Embedding-Loss-Function branch from 5cc4d0f to b37f3c7 Compare March 17, 2020 03:16

birm approved these changes Mar 17, 2020

View reviewed changes

mlpack-bot bot removed the s: needs review label Mar 17, 2020

Merge branch 'master' into Hinge-Embedding-Loss-Function

0d92025

dummy commit

af8d812

ojhalakshya force-pushed the Hinge-Embedding-Loss-Function branch from af8d812 to 854adf2 Compare March 17, 2020 07:01

style fix

778f4d2

ojhalakshya force-pushed the Hinge-Embedding-Loss-Function branch from 854adf2 to 778f4d2 Compare March 17, 2020 07:17

Merge branch 'master' into Hinge-Embedding-Loss-Function

b52d5fe

birm merged commit a967547 into mlpack:master Mar 18, 2020

Uh oh!

Conversation

ojhalakshya commented Feb 22, 2020

Uh oh!

kartikdutt18 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

birm left a comment

Choose a reason for hiding this comment

Uh oh!

kartikdutt18 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

birm Mar 3, 2020

Choose a reason for hiding this comment

Uh oh!

kartikdutt18 Mar 3, 2020

Choose a reason for hiding this comment

Uh oh!

ojhalakshya Mar 14, 2020

Choose a reason for hiding this comment

Uh oh!

rcurtin commented Mar 12, 2020

Uh oh!

ojhalakshya commented Mar 13, 2020

Uh oh!

rcurtin commented Mar 13, 2020

Uh oh!

ojhalakshya commented Mar 13, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

favre49 left a comment

Choose a reason for hiding this comment

Uh oh!

kartikdutt18 commented Mar 15, 2020

Uh oh!

ojhalakshya commented Mar 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ojhalakshya commented Mar 16, 2020

Uh oh!

birm commented Mar 17, 2020

Uh oh!

ojhalakshya commented Mar 17, 2020

Uh oh!

birm left a comment

Choose a reason for hiding this comment

Uh oh!

kartikdutt18 commented Mar 17, 2020

Uh oh!

kartikdutt18 commented Mar 18, 2020

Uh oh!

kartikdutt18 commented Mar 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ojhalakshya commented Mar 18, 2020

Uh oh!

kartikdutt18 commented Mar 18, 2020

Uh oh!

birm commented Mar 18, 2020

Uh oh!

kartikdutt18 commented Mar 18, 2020

Uh oh!

ojhalakshya commented Mar 18, 2020

Uh oh!

kartikdutt18 commented Mar 18, 2020

Uh oh!

Reviewers

Assignees

Labels

ojhalakshya commented Mar 16, 2020 •

edited

Loading

kartikdutt18 commented Mar 18, 2020 •

edited

Loading