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

seanytak/ncf test tuning #1092

Merged

Conversation

seanytak
Copy link
Contributor

Description

This PR adds a training notebook and script for the NCF model to show readers how NNI can be applied to different types of models (the SVD and NCF) as a part of the model selection process.
Adds a notebook comparing NCF and SVD under the NNI framework.
Adds a training script for NCF to be used with the notebook.

Related Issues

#1091

Checklist:

  • I have followed the contribution guidelines and code style for this project.
  • I have added tests covering my contributions.
  • I have updated the documentation accordingly.
  • This PR is being made to staging and not master.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

You'll be able to see Jupyter notebook diff and discuss changes. Powered by ReviewNB.

@msftclas
Copy link

msftclas commented Apr 23, 2020

CLA assistant check
All CLA requirements met.

Copy link
Collaborator

@yueguoguo yueguoguo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome. LGTM

@miguelgfierro
Copy link
Collaborator

@niwilso I'm trying to add you as reviewer but for some reason I can't. Feel free to add comments and suggestions as well

Copy link
Collaborator

@miguelgfierro miguelgfierro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stoping this for merging, since there is an error in the notebook of svd nni

@@ -0,0 +1,930 @@
{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it came as a surprise that you are comparing both algos instead of doing hyperparameter tuning on NCF. I think for simplicity it would be better to have a notebook with hyperparameter tuning for NCF.

About having a comparison between SVD and NCF, is there a benefit of having 1 notebook with the comparison vs comparing the outputs of both independent notebooks?


Reply via ReviewNB

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Originally I was thinking that this new notebook would focus more on the comparison between model architectures after they've been tuned through NNI. With only the SVD and NCF model though comparing the outputs of the two notebooks may serve the same purpose. I'll refactor the notebook to focus on tuning for NCF, thanks for the suggestion!

Copy link
Collaborator

@miguelgfierro miguelgfierro Apr 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you please ping me here when the change is done? sorry I have so many messages coming from github

@anargyri
Copy link
Collaborator

There is a broken link to the training script.
There is no SVD trained in the notebook, is there? But there are references to SVD in the text.

@seanytak
Copy link
Contributor Author

Hi @anargyri addressed your comment in recent commits. Thanks!

@anargyri
Copy link
Collaborator

anargyri commented May 1, 2020

There is still a broken link (NCF Training Script).

Copy link
Collaborator

@miguelgfierro miguelgfierro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

really nice work thank you

@miguelgfierro miguelgfierro merged commit 069d050 into recommenders-team:staging May 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants