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 Hinge embedding loss function #2229

Merged
merged 7 commits into from Mar 18, 2020

Conversation

@ojhalakshya
Copy link
Contributor

ojhalakshya commented Feb 22, 2020

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.

@ojhalakshya ojhalakshya changed the title Hinge embedding loss function Adding Hinge embedding loss function Feb 22, 2020
Copy link
Contributor

kartikdutt18 left a comment

Some small changes, other than that it looks fine.

src/mlpack/tests/loss_functions_test.cpp Outdated Show resolved Hide resolved
src/mlpack/tests/loss_functions_test.cpp Outdated Show resolved Hide resolved
Copy link
Member

birm left a comment

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!

Copy link
Contributor

kartikdutt18 left a comment

One really minor style issue.

src/mlpack/tests/loss_functions_test.cpp Outdated Show resolved Hide resolved
@ojhalakshya ojhalakshya requested a review from zoq Feb 24, 2020
const TargetType&& target,
OutputType&& output)
{
for (size_t i = 0; i < input.n_elem; i++)

This comment has been minimized.

Copy link
@birm

birm Mar 3, 2020

Member

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

This comment has been minimized.

Copy link
@kartikdutt18

kartikdutt18 Mar 5, 2020

Contributor

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

This comment has been minimized.

Copy link
@ojhalakshya

ojhalakshya Mar 14, 2020

Author Contributor

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

@ojhalakshya ojhalakshya closed this Mar 8, 2020
@ojhalakshya ojhalakshya force-pushed the ojhalakshya:Hinge-Embedding-Loss-Function branch from 2b9bf44 to 0d8c3a0 Mar 8, 2020
@ojhalakshya ojhalakshya reopened this Mar 10, 2020
@ojhalakshya ojhalakshya force-pushed the ojhalakshya:Hinge-Embedding-Loss-Function branch 4 times, most recently from 0b6af9b to 08a7099 Mar 10, 2020
@rcurtin

This comment has been minimized.

Copy link
Member

rcurtin commented Mar 12, 2020

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 ojhalakshya force-pushed the ojhalakshya:Hinge-Embedding-Loss-Function branch from ff2a9fc to 6f1971a Mar 13, 2020
@ojhalakshya

This comment has been minimized.

Copy link
Contributor Author

ojhalakshya commented Mar 13, 2020

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

Screenshot from 2020-03-13 10-21-18

Thanks.

@rcurtin

This comment has been minimized.

Copy link
Member

rcurtin commented Mar 13, 2020

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

This comment has been minimized.

Copy link
Contributor Author

ojhalakshya commented Mar 13, 2020

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.

@ojhalakshya ojhalakshya force-pushed the ojhalakshya:Hinge-Embedding-Loss-Function branch 2 times, most recently from f9ddb47 to a0dd14e Mar 13, 2020
@ojhalakshya ojhalakshya requested review from favre49 and birm Mar 13, 2020
@ojhalakshya ojhalakshya force-pushed the ojhalakshya:Hinge-Embedding-Loss-Function branch 2 times, most recently from f600880 to ad3f83d Mar 14, 2020
Copy link
Member

favre49 left a comment

Everything seems good to me :) Good work!

@ojhalakshya ojhalakshya force-pushed the ojhalakshya:Hinge-Embedding-Loss-Function branch from ad3f83d to fdcd685 Mar 16, 2020
@ojhalakshya

This comment has been minimized.

Copy link
Contributor Author

ojhalakshya commented Mar 16, 2020

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.
Screenshot from 2020-03-16 13-16-24

@ojhalakshya ojhalakshya force-pushed the ojhalakshya:Hinge-Embedding-Loss-Function branch from fdcd685 to 5cc4d0f Mar 16, 2020
@ojhalakshya ojhalakshya requested a review from birm Mar 16, 2020
@ojhalakshya

This comment has been minimized.

Copy link
Contributor Author

ojhalakshya commented Mar 16, 2020

@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

This comment has been minimized.

Copy link
Member

birm commented Mar 17, 2020

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

ojhalakshya added 3 commits Mar 8, 2020
* 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
bug fixes
minor change
@ojhalakshya ojhalakshya force-pushed the ojhalakshya:Hinge-Embedding-Loss-Function branch from 5cc4d0f to b37f3c7 Mar 17, 2020
@ojhalakshya

This comment has been minimized.

Copy link
Contributor Author

ojhalakshya commented Mar 17, 2020

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
birm approved these changes Mar 17, 2020
Copy link
Member

birm left a comment

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

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

This comment has been minimized.

Copy link
Contributor

kartikdutt18 commented Mar 17, 2020

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

@ojhalakshya ojhalakshya force-pushed the ojhalakshya:Hinge-Embedding-Loss-Function branch from af8d812 to 854adf2 Mar 17, 2020
@ojhalakshya ojhalakshya force-pushed the ojhalakshya:Hinge-Embedding-Loss-Function branch from 854adf2 to 778f4d2 Mar 17, 2020
@kartikdutt18

This comment has been minimized.

Copy link
Contributor

kartikdutt18 commented Mar 18, 2020

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

@kartikdutt18

This comment has been minimized.

Copy link
Contributor

kartikdutt18 commented Mar 18, 2020

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

This comment has been minimized.

Copy link
Contributor Author

ojhalakshya commented Mar 18, 2020

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

This comment has been minimized.

Copy link
Contributor

kartikdutt18 commented Mar 18, 2020

Agreed, Thanks again. :)

@birm

This comment has been minimized.

Copy link
Member

birm commented Mar 18, 2020

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

@kartikdutt18

This comment has been minimized.

Copy link
Contributor

kartikdutt18 commented Mar 18, 2020

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

This comment has been minimized.

Copy link
Contributor Author

ojhalakshya commented Mar 18, 2020

@kartikdutt18 @birm
Thanks for the review. :)

@kartikdutt18

This comment has been minimized.

Copy link
Contributor

kartikdutt18 commented Mar 18, 2020

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

@birm birm merged commit a967547 into mlpack:master Mar 18, 2020
17 of 18 checks passed
17 of 18 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
LaTeX Documentation Checks Build finished.
Details
Memory Checks Build finished.
Details
Static Code Analysis Checks Build finished.
Details
Style Checks Build finished.
Details
mlpack.mlpack Build #20200318.3 succeeded
Details
mlpack.mlpack (Linux Julia) Linux Julia succeeded
Details
mlpack.mlpack (Linux Markdown) Linux Markdown succeeded
Details
mlpack.mlpack (Linux Plain) Linux Plain succeeded
Details
mlpack.mlpack (Linux Python27) Linux Python27 succeeded
Details
mlpack.mlpack (Linux Python37) Linux Python37 succeeded
Details
mlpack.mlpack (Windows VS14 Plain) Windows VS14 Plain succeeded
Details
mlpack.mlpack (Windows VS15 Plain) Windows VS15 Plain succeeded
Details
mlpack.mlpack (Windows VS16 Plain) Windows VS16 Plain succeeded
Details
mlpack.mlpack (macOS Julia) macOS Julia succeeded
Details
mlpack.mlpack (macOS Plain) macOS Plain succeeded
Details
mlpack.mlpack (macOS Python27) macOS Python27 succeeded
Details
mlpack.mlpack (macOS Python37) macOS Python37 succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.