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

[feature] ragged tensor support and enable ShadowVariable look up for safe_embedding_lookup_sparse #397

Merged
merged 1 commit into from
Apr 18, 2024

Conversation

jq
Copy link
Collaborator

@jq jq commented Apr 9, 2024

Description

  1. enable ShadowVariable look up for safe_embedding_lookup_sparse
  2. support ragged tensor
    Brief Description of the PR:

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?

If you're adding a bugfix or new feature please describe the tests that you ran to verify your changes:
*

@jq jq requested a review from rhdong as a code owner April 9, 2024 16:31
@jq jq force-pushed the master branch 2 times, most recently from 1652941 to 84d1c40 Compare April 9, 2024 17:11
@jq jq requested a review from MoFHeka April 9, 2024 17:14
@jq jq force-pushed the master branch 14 times, most recently from 37e2bbe to 13fa638 Compare April 16, 2024 05:19
@jq jq force-pushed the master branch 3 times, most recently from 2df0e53 to 2aea76e Compare April 17, 2024 01:09
return embeddings


def embedding_lookup_sparse(
Copy link
Member

Choose a reason for hiding this comment

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

In the spirit of TensorFlow API designation, may we merge the logic of processing the ragged tensor into the API with the same name in the dynamic_embedding_ops.py.(it is up on you, considering the complexity)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thanks for review, tf's raggedtensor also live in seperated file, so this maybe ok?

@jq jq force-pushed the master branch 4 times, most recently from c737b3d to 4280d1d Compare April 17, 2024 16:59
@jq jq force-pushed the master branch 5 times, most recently from d1bbbec to ba85400 Compare April 17, 2024 22:26
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 a19f7c0 into tensorflow:master Apr 18, 2024
36 checks passed
@jq jq changed the title enable ShadowVariable look up for safe_embedding_lookup_sparse [feature] ragged tensor support and enable ShadowVariable look up for safe_embedding_lookup_sparse Jul 2, 2024
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.

2 participants