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

[WIP] NCF class and member functions #1454

Open
wants to merge 23 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@haritha1313
Contributor

haritha1313 commented Jun 29, 2018

The class isn't complete yet. The PR is being made so that review can be done in parallel.

haritha1313 added some commits Jun 29, 2018

@haritha1313

This comment has been minimized.

Show comment
Hide comment
@haritha1313

haritha1313 Jul 7, 2018

Contributor

Functions for getting recommendations and getting evaluation parameters have been added. Since the Train, Gradient, Evaluate functions are being modified to accommodate multiple training instances, and after that only the network can be used to predict ratings, the functions cannot be tested right now as part of the class.

Contributor

haritha1313 commented Jul 7, 2018

Functions for getting recommendations and getting evaluation parameters have been added. Since the Train, Gradient, Evaluate functions are being modified to accommodate multiple training instances, and after that only the network can be used to predict ratings, the functions cannot be tested right now as part of the class.

@zoq

This comment has been minimized.

Show comment
Hide comment
@zoq

zoq Jul 8, 2018

Member

So, once the Train, Gradient, and Evaluate function is implemented we can test the NCF class?

Member

zoq commented Jul 8, 2018

So, once the Train, Gradient, and Evaluate function is implemented we can test the NCF class?

@haritha1313

This comment has been minimized.

Show comment
Hide comment
@haritha1313

haritha1313 Jul 8, 2018

Contributor

I think so, I am working on it as per we had discussed. There might be some debugging necessary in GetRecommendations(), Evaluate() etc once network training is possible. I am also working on ncf_main parallely, so if everything goes well, we can test it from CLI too.

Contributor

haritha1313 commented Jul 8, 2018

I think so, I am working on it as per we had discussed. There might be some debugging necessary in GetRecommendations(), Evaluate() etc once network training is possible. I am also working on ncf_main parallely, so if everything goes well, we can test it from CLI too.

@zoq

This comment has been minimized.

Show comment
Hide comment
@zoq

zoq Jul 8, 2018

Member

Okay, thanks for the clarification, let me know if you need any help.

Member

zoq commented Jul 8, 2018

Okay, thanks for the clarification, let me know if you need any help.

haritha1313 added some commits Jul 12, 2018

Show outdated Hide outdated src/mlpack/methods/cf/ncf_impl.hpp
Show outdated Hide outdated src/mlpack/methods/ann/layer/concat.hpp
Show outdated Hide outdated src/mlpack/methods/cf/ncf_impl.hpp
Show outdated Hide outdated src/mlpack/methods/cf/ncf_impl.hpp
Show outdated Hide outdated src/mlpack/methods/cf/ncf_impl.hpp
Show outdated Hide outdated src/mlpack/methods/cf/ncf_impl.hpp

haritha1313 added some commits Jul 29, 2018

@zoq

This comment has been minimized.

Show comment
Hide comment
@zoq

zoq Aug 6, 2018

Member

When you test the code, can you make sure you are using the sequential layer from: #1441 (the PR contains a fix for the Gradient step, the code should run just fine without the changes but it doesn't perform all gradient steps), the PR should be merged soon, so we just have to rebase the PR against the changes.

Member

zoq commented Aug 6, 2018

When you test the code, can you make sure you are using the sequential layer from: #1441 (the PR contains a fix for the Gradient step, the code should run just fine without the changes but it doesn't perform all gradient steps), the PR should be merged soon, so we just have to rebase the PR against the changes.

haritha1313 added some commits Aug 9, 2018

@zoq

This comment has been minimized.

Show comment
Hide comment
@zoq

zoq Aug 10, 2018

Member

Should we include the code from the neumf branch as a simple test case?

Member

zoq commented Aug 10, 2018

Should we include the code from the neumf branch as a simple test case?

@haritha1313

This comment has been minimized.

Show comment
Hide comment
@haritha1313

haritha1313 Aug 10, 2018

Contributor

@zoq I am not sure which code you meant. The neumf branch only has some edits to existing ann layers. Or did you mean of the sample neumf network?

Contributor

haritha1313 commented Aug 10, 2018

@zoq I am not sure which code you meant. The neumf branch only has some edits to existing ann layers. Or did you mean of the sample neumf network?

@zoq

This comment has been minimized.

Show comment
Hide comment
@zoq

zoq Aug 10, 2018

Member

Sorry for the confusion, I was talking about the simple code we used for testing, from the gist.

Member

zoq commented Aug 10, 2018

Sorry for the confusion, I was talking about the simple code we used for testing, from the gist.

@zoq

With the moddifications I was able to run the model via the Train(...)method, let me know if I should clarify anything.

Show outdated Hide outdated src/mlpack/methods/cf/ncf.hpp
Show outdated Hide outdated src/mlpack/methods/cf/ncf_impl.hpp
}
}
predictors = arma::join_vert(users, items);
responses = arma::conv_to<arma::mat>::from(resp);

This comment has been minimized.

@zoq

zoq Aug 13, 2018

Member

To get the optimizer the actual number of functions, we have to set: numFunctions = responses.n_cols; at the end. numFunctions is of type size_t.

@zoq

zoq Aug 13, 2018

Member

To get the optimizer the actual number of functions, we have to set: numFunctions = responses.n_cols; at the end. numFunctions is of type size_t.

This comment has been minimized.

@zoq

zoq Aug 13, 2018

Member

The optimizer will have access the numFunctions parameter via the introduced NumFunctions() method; see comment above.

@zoq

zoq Aug 13, 2018

Member

The optimizer will have access the numFunctions parameter via the introduced NumFunctions() method; see comment above.

@haritha1313

This comment has been minimized.

Show comment
Hide comment
@haritha1313

haritha1313 Aug 13, 2018

Contributor

@zoq I will try the edits you have suggested and let you know.

Contributor

haritha1313 commented Aug 13, 2018

@zoq I will try the edits you have suggested and let you know.

*/
void NCF::CreateGMF()
{
size_t size = (dataset.n_cols * (neg+1));

This comment has been minimized.

@zoq

zoq Aug 14, 2018

Member

About the batch size issue, the Subview layer is the only one that depends on this information right?

@zoq

zoq Aug 14, 2018

Member

About the batch size issue, the Subview layer is the only one that depends on this information right?

This comment has been minimized.

@haritha1313

haritha1313 Aug 14, 2018

Contributor

Subview layer and Linear layer. Linear is not present in the current code. I'll be pushing that in a while.

@haritha1313

haritha1313 Aug 14, 2018

Contributor

Subview layer and Linear layer. Linear is not present in the current code. I'll be pushing that in a while.

@haritha1313

This comment has been minimized.

Show comment
Hide comment
@haritha1313

haritha1313 Aug 14, 2018

Contributor

The linear layer in line numbers 189, 196, 235, 236 are necessary but cannot be used without using batches, as discussed earlier (memory error).

Contributor

haritha1313 commented Aug 14, 2018

The linear layer in line numbers 189, 196, 235, 236 are necessary but cannot be used without using batches, as discussed earlier (memory error).

haritha1313 added some commits Aug 19, 2018

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