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 the notebook for dynamic_embedding demo #315

Merged
merged 2 commits into from
Apr 3, 2023

Conversation

thushv89
Copy link
Contributor

@thushv89 thushv89 commented Mar 29, 2023

Description

This adds a notebook to demonstrate the usage of the dynamic_embedding module in the tensorflow_recommender_addons library.

Fixes # (issue)

Type of change

  • Bug fix
  • New Tutorial
  • Updated or additional documentation
  • Additional Testing
  • New Feature

Checklist:

  • I've properly formatted my code according to the guidelines
    • By running yapf
    • By running clang-format
  • This PR addresses an already submitted issue for TensorFlow Recommenders-Addons
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works

How Has This Been Tested?

N/A

@google-cla
Copy link

google-cla bot commented Mar 29, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@rhdong
Copy link
Member

rhdong commented Apr 3, 2023

Hi @thushv89, thank you for your excellent job. To avoid you missing my comments in the ReviewNB, just paste them here. Hope these comments can be helpful:

  • The de.keras.layers.BasicEmbedding could be changed to de.keras.layers.Embedding, for we have v0.6.0 release to support them.
  • It looks like the embedding size fluctuates too much, which may hurt the AUC of DE restrict. So setting the restrict configuration num_reserved from 0.8 to 0.95 would be better, and re-run testing is needed. -- This change may help get a higher AUC on restrict mode. (Please confirm the TFRA version is 0.6.0 when running the test.)
    Thank you so much!

@rhdong rhdong self-requested a review April 3, 2023 13:37
Copy link
Member

@rhdong rhdong left a comment

Choose a reason for hiding this comment

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

LGTM

@rhdong rhdong merged commit f5e10d3 into tensorflow:master Apr 3, 2023
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.

None yet

2 participants