-
Notifications
You must be signed in to change notification settings - Fork 19.6k
Implement empty_like function in keras.ops #21840
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
Conversation
Summary of ChangesHello @shashaka, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the Keras operations API by introducing the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces the empty_like operation. The overall structure and API definition are good, but there are some issues with the backend implementations. Specifically, the dtype handling when it is not provided is incorrect across all backends; it should default to the input tensor's dtype, not config.floatx(). I've provided suggestions to fix this. Additionally, the docstring for the new operation is missing a code example, which is required by the Keras API guidelines. Finally, the tests for empty_like are not comprehensive enough to catch the dtype inference bug, and I've suggested an additional test case to cover this.
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces the keras.ops.empty_like function, providing implementations for NumPy, JAX, TensorFlow, and PyTorch backends, while correctly marking it as not supported for OpenVINO. The changes are well-structured and include corresponding tests.
My main feedback is regarding the TensorFlow backend implementation, which currently uses tf.zeros_like. This creates a zero-initialized tensor, which is inconsistent with the function's documentation and its behavior in other backends. I've suggested using tf.empty_like to ensure correctness and consistency.
|
May I ask what the rationale is for adding this op. Is the hope to get better performance than JAX implements Also note that on JAX and TensorFlow, the compiler will be smart enough under some conditions to not materialize the array at all, which is even more performant than not initializing the values. |
|
@hertschuh My main motivation was API consistency with NumPy to make it easier for users porting NumPy code into keras.ops. I will go ahead and close this PR. Thank you again for the detailed explanation and feedback. |
|
Actually, after I wrote this comment, I noticed that keras already implements Please re-open and sorry for the churn. |
hertschuh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #21840 +/- ##
=======================================
Coverage 82.66% 82.66%
=======================================
Files 577 577
Lines 59477 59506 +29
Branches 9329 9330 +1
=======================================
+ Hits 49167 49193 +26
- Misses 7907 7910 +3
Partials 2403 2403
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
hertschuh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the addition!
Adds keras.ops.empty_like, which creates a new tensor with the same shape and dtype as the input tensor, with uninitialized values.
Supported across NumPy, TensorFlow, PyTorch, and JAX backends.
Not supported on OpenVINO.