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

Fix bugs in RBM notebooks #1581

Merged
merged 14 commits into from
Dec 15, 2021
Merged

Fix bugs in RBM notebooks #1581

merged 14 commits into from
Dec 15, 2021

Conversation

pradnyeshjoshi
Copy link
Collaborator

@pradnyeshjoshi pradnyeshjoshi commented Dec 13, 2021

Description

  1. RBM algorithm in its current implementation requires ratings to be integers. Added code to map float ratings to integers in RBM quick start and deep dive notebooks.
  2. Minor latex change in RBM deep dive notebook so that the equations render properly.
  3. Disabled TF warnings in RBM deep dive notebook.
  4. Previously the batch size for precision calculation, which is a part of the training loop, was hardcoded and equal to the number of users (which caused OOM issue during model fitting for 10M and 20M movielens datasets). Changing this batch size resolves the OOM error during model fitting.

Related Issues

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 branch and not to main branch.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@pradnyeshjoshi pradnyeshjoshi self-assigned this Dec 13, 2021
@codecov-commenter
Copy link

codecov-commenter commented Dec 13, 2021

Codecov Report

Merging #1581 (593c2d6) into staging (dc3455d) will decrease coverage by 0.01%.
The diff coverage is n/a.

❗ Current head 593c2d6 differs from pull request most recent head af4fb2a. Consider uploading reports for the commit af4fb2a to get more accurate results
Impacted file tree graph

@@             Coverage Diff             @@
##           staging    #1581      +/-   ##
===========================================
- Coverage    61.89%   61.88%   -0.02%     
===========================================
  Files           84       84              
  Lines         8458     8458              
===========================================
- Hits          5235     5234       -1     
- Misses        3223     3224       +1     
Flag Coverage Δ
pr-gate 61.88% <ø> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
recommenders/models/rbm/rbm.py 73.18% <ø> (ø)
recommenders/evaluation/spark_evaluation.py 86.60% <0.00%> (-0.45%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ca70a97...af4fb2a. Read the comment docs.

@pradnyeshjoshi pradnyeshjoshi changed the title Pradjoshi/test rbm Fix bugs in RBM notebooks Dec 13, 2021
@@ -60,7 +60,7 @@
"System version: 3.7.11 (default, Jul 27 2021, 14:32:16) \n",
"[GCC 7.5.0]\n",
"Pandas version: 1.3.4\n",
"Tensorflow version: 2.6.1\n"
"Tensorflow version: 1.15.5\n"
Copy link
Collaborator

Choose a reason for hiding this comment

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

hey @pradjoshi can you merge the latest stating to your branch, reinstall using TF 2 and rereun again?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hi @miguelgfierro I have reinstalled recommenders using TF2 and rerun the notebooks.

@review-notebook-app
Copy link

review-notebook-app bot commented Dec 15, 2021

View / edit / reply to this conversation on ReviewNB

miguelgfierro commented on 2021-12-15T08:54:38Z
----------------------------------------------------------------

Same comment as before with the verbosity


pradnyeshjoshi commented on 2021-12-15T16:54:45Z
----------------------------------------------------------------

Andreas resolved this.

@review-notebook-app
Copy link

review-notebook-app bot commented Dec 15, 2021

View / edit / reply to this conversation on ReviewNB

miguelgfierro commented on 2021-12-15T08:54:38Z
----------------------------------------------------------------

we need to revisit the change in metrics, maybe in a different PR. Is there an issue to track this?


pradnyeshjoshi commented on 2021-12-15T16:54:14Z
----------------------------------------------------------------

I have created a task for it. Should I create an issue on GitHub instead?

@review-notebook-app
Copy link

review-notebook-app bot commented Dec 15, 2021

View / edit / reply to this conversation on ReviewNB

miguelgfierro commented on 2021-12-15T08:54:39Z
----------------------------------------------------------------

Line #41.    print("Pandas version: {}".format(pd.__version__))

maybe here it would be good to also add tf version


pradnyeshjoshi commented on 2021-12-15T16:58:41Z
----------------------------------------------------------------

Done.

@review-notebook-app
Copy link

review-notebook-app bot commented Dec 15, 2021

View / edit / reply to this conversation on ReviewNB

miguelgfierro commented on 2021-12-15T08:54:40Z
----------------------------------------------------------------

This is super verbose. Ideally what we want is just the old output, with the metrics loop:

training epoch 0 rmse 0.961369 
training epoch 10 rmse 0.902065 
training epoch 20 rmse 0.864718 
training epoch 30 rmse 0.848415 
done training, Training time 10.9566452 
Train set accuracy 0.3561482 
Test set accuracy 0.3516242 

It looks the command to show only the errors is not working:

tf.get_logger().setLevel('ERROR') # only show error messages

Maybe there is a new command in TF. If you can't find it, it is better to manually erase the cell


pradnyeshjoshi commented on 2021-12-15T16:52:10Z
----------------------------------------------------------------

Looks like Andreas resolved this already!

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.

@pradnyeshjoshi to me this looks ok to go, after the minor comments on logs.

@anargyri do you want to include this in the release?

@anargyri
Copy link
Collaborator

@pradnyeshjoshi to me this looks ok to go, after the minor comments on logs.

@anargyri do you want to include this in the release?

I don't know, maybe if there is time. First we need to resolve the tests that fail in the release.

@anargyri
Copy link
Collaborator

View / edit / reply to this conversation on ReviewNB

miguelgfierro commented on 2021-12-15T08:54:38Z ----------------------------------------------------------------

Same comment as before with the verbosity

Yes, this is strange. Other algos like deeprec don't have this amount of output. Maybe you could check what the difference is from those.

@anargyri
Copy link
Collaborator

View / edit / reply to this conversation on ReviewNB
miguelgfierro commented on 2021-12-15T08:54:38Z ----------------------------------------------------------------
Same comment as before with the verbosity

Yes, this is strange. Other algos like deeprec don't have this amount of output. Maybe you could check what the difference is from those.

I think I fixed it according to tensorflow/tensorflow#18146

Copy link
Collaborator Author

Looks like Andreas resolved this already!


View entire conversation on ReviewNB

Copy link
Collaborator Author

I have created a task for it. Should I create an issue on GitHub instead?


View entire conversation on ReviewNB

Copy link
Collaborator Author

Andreas resolved this.


View entire conversation on ReviewNB

Copy link
Collaborator Author

Done.


View entire conversation on ReviewNB

@anargyri
Copy link
Collaborator

I have created a task for it. Should I create an issue on GitHub instead?

View entire conversation on ReviewNB

The task should be enough.

@anargyri anargyri merged commit ba24adc into staging Dec 15, 2021
@miguelgfierro miguelgfierro deleted the pradjoshi/test_rbm branch December 15, 2021 20:12
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.

4 participants