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

NCFNetwork addition in ann #1422

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@haritha1313
Contributor

haritha1313 commented Jun 5, 2018

Evaluate(), Gradient() functions yet to be implemented.
Rough structure to analyze handling of two input vectors(user and item).

curUserInput = std::move(userInput);
curItemInput = std::move(itemInput);
Forward(std::move(curUserInput), std::move(curUserInput));

This comment has been minimized.

@zoq

zoq Jun 6, 2018

Member

Correct me if I'm wrong but the userModel will work on the user data and the itemModel will work on the item data and the output is going to be merged together right? If that is true we could reuse the existing infrastructure and merge the output of both networks internally. Let me know what you think and I can provide some pseudo code.

@zoq

zoq Jun 6, 2018

Member

Correct me if I'm wrong but the userModel will work on the user data and the itemModel will work on the item data and the output is going to be merged together right? If that is true we could reuse the existing infrastructure and merge the output of both networks internally. Let me know what you think and I can provide some pseudo code.

This comment has been minimized.

@haritha1313

haritha1313 Jun 6, 2018

Contributor

You are right. The merged (multiplymerge/concat) output will further pass through two more layers, to give final output. If the existing classes can create this network, that will be great. Pseudo code will be much helpful. Thanks.

@haritha1313

haritha1313 Jun 6, 2018

Contributor

You are right. The merged (multiplymerge/concat) output will further pass through two more layers, to give final output. If the existing classes can create this network, that will be great. Pseudo code will be much helpful. Thanks.

This comment has been minimized.

@zoq

zoq Jun 6, 2018

Member

So one idea I had was to implement a subview layer, which takes the merged input (user/item) and only selects the interesting part for a particular network, something like:

FFN<NegativeLogLikelihood<>, NguyenWidrowInitialization> modelA;
modelA.Add<Subview<> >(0, 10); // here we take the first 10 elements from the input
modelA.Add<IdentityLayer<> >();
modelA.Add<LinearNoBias<> >(10, 2);

FFN<NegativeLogLikelihood<>, NguyenWidrowInitialization> modelB;
modelA.Add<Subview<> >(10, 20); // here we take the last 10 elements
modelB.Add<IdentityLayer<> >();
modelB.Add<LinearNoBias<> >(10, 2);

FFN<NegativeLogLikelihood<>, NguyenWidrowInitialization> modelMain;
modelB.Add<MergeMultiply<> >(modelA, modelB);

in the Train function we can merge both inputs and use the existing functionalty as before.

Another idea is to avoid an extra subview layer and to do the merge of the output inside the NCF class. I guess this is what you currently doing, let me know what you think about each idea.

@zoq

zoq Jun 6, 2018

Member

So one idea I had was to implement a subview layer, which takes the merged input (user/item) and only selects the interesting part for a particular network, something like:

FFN<NegativeLogLikelihood<>, NguyenWidrowInitialization> modelA;
modelA.Add<Subview<> >(0, 10); // here we take the first 10 elements from the input
modelA.Add<IdentityLayer<> >();
modelA.Add<LinearNoBias<> >(10, 2);

FFN<NegativeLogLikelihood<>, NguyenWidrowInitialization> modelB;
modelA.Add<Subview<> >(10, 20); // here we take the last 10 elements
modelB.Add<IdentityLayer<> >();
modelB.Add<LinearNoBias<> >(10, 2);

FFN<NegativeLogLikelihood<>, NguyenWidrowInitialization> modelMain;
modelB.Add<MergeMultiply<> >(modelA, modelB);

in the Train function we can merge both inputs and use the existing functionalty as before.

Another idea is to avoid an extra subview layer and to do the merge of the output inside the NCF class. I guess this is what you currently doing, let me know what you think about each idea.

This comment has been minimized.

@haritha1313

haritha1313 Jun 6, 2018

Contributor

The first idea does solve the problem of passing multiple input matrices. But there was an issue with using multiplymerge in ffn. We had considered using sequential instead but this didn't work out when trying to add further layers after multiplymerge(linear, sigmoid) and was giving 'double free or corruption' error. Unless I am implementing the network wrong, this will also have to be sorted out to implement the first idea.
With the second one, you are right. It is now based on merging outputs in NCF class, but even here I'm concerned with adding further layers after the merging part (since it is manual merging, and not part of the network) to form a continuous network.
I think first one is a better approach since the training problem will be completely solved and no manual merging is required. Just need to resolve that error.

@haritha1313

haritha1313 Jun 6, 2018

Contributor

The first idea does solve the problem of passing multiple input matrices. But there was an issue with using multiplymerge in ffn. We had considered using sequential instead but this didn't work out when trying to add further layers after multiplymerge(linear, sigmoid) and was giving 'double free or corruption' error. Unless I am implementing the network wrong, this will also have to be sorted out to implement the first idea.
With the second one, you are right. It is now based on merging outputs in NCF class, but even here I'm concerned with adding further layers after the merging part (since it is manual merging, and not part of the network) to form a continuous network.
I think first one is a better approach since the training problem will be completely solved and no manual merging is required. Just need to resolve that error.

This comment has been minimized.

@zoq

zoq Jun 6, 2018

Member

Okay, I'll take a look into the sequential issue tomorrow.

@zoq

zoq Jun 6, 2018

Member

Okay, I'll take a look into the sequential issue tomorrow.

This comment has been minimized.

@zoq

zoq Jun 7, 2018

Member

I found a couple of issues, which needs to be fixed, will open a PR, in the meantime do you like to work on the subview layer?

@zoq

zoq Jun 7, 2018

Member

I found a couple of issues, which needs to be fixed, will open a PR, in the meantime do you like to work on the subview layer?

This comment has been minimized.

@haritha1313

haritha1313 Jun 8, 2018

Contributor

Sure, that sounds like a good plan. Shall I close this PR then? I can just open another one for subview layer.

@haritha1313

haritha1313 Jun 8, 2018

Contributor

Sure, that sounds like a good plan. Shall I close this PR then? I can just open another one for subview layer.

This comment has been minimized.

@zoq

zoq Jun 8, 2018

Member

Let's keep this one open, once the changes are merged we can reuse this one. For the subview layer let's open separate PR.

@zoq

zoq Jun 8, 2018

Member

Let's keep this one open, once the changes are merged we can reuse this one. For the subview layer let's open separate PR.

This comment has been minimized.

@zoq

zoq Jun 8, 2018

Member

Here are the modifications: #1427, this one comes with a simple example, let me know what you think (The subview layer would be the first layer of the Sequential layer).

@zoq

zoq Jun 8, 2018

Member

Here are the modifications: #1427, this one comes with a simple example, let me know what you think (The subview layer would be the first layer of the Sequential layer).

@haritha1313

This comment has been minimized.

Show comment
Hide comment
@haritha1313

haritha1313 Aug 23, 2018

Contributor

@zoq Shall I close this PR as this idea is no more being worked on?

Contributor

haritha1313 commented Aug 23, 2018

@zoq Shall I close this PR as this idea is no more being worked on?

@zoq

This comment has been minimized.

Show comment
Hide comment
@zoq

zoq Aug 23, 2018

Member

Good idea, let's close this one.

Member

zoq commented Aug 23, 2018

Good idea, let's close this one.

@zoq zoq closed this Aug 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment