Implement numpy.view for OpenVINO backend#22407
Implement numpy.view for OpenVINO backend#22407rishi-sangare wants to merge 1 commit intokeras-team:masterfrom
Conversation
Summary of ChangesHello, 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 introduces comprehensive support for numpy.view within the OpenVINO backend, a functionality previously unavailable. The implementation addresses the lack of a native bitcast operation in OpenVINO by employing a multi-pronged approach that intelligently handles various data types and tensor characteristics, including constant folding for NumPy arrays and bitwise manipulation for dynamic integer tensors of differing widths. This enhancement significantly expands the capabilities of the OpenVINO backend for data manipulation. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
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
|
|
Warning Gemini is experiencing higher than usual traffic and was unable to create the review. Please try again in a few hours by commenting |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #22407 +/- ##
==========================================
- Coverage 83.04% 82.95% -0.09%
==========================================
Files 596 596
Lines 66693 66789 +96
Branches 10383 10394 +11
==========================================
+ Hits 55382 55407 +25
- Misses 8675 8742 +67
- Partials 2636 2640 +4
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:
|
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements numpy.view for the OpenVINO backend, which is a great addition. The implementation correctly handles constant inputs, same-width integer conversions, and more complex cross-width integer conversions using bitwise operations. The approach is solid. I have a few suggestions to improve code maintainability and readability by reducing duplication and simplifying some logic. Also, adding a docstring to the new view function would be beneficial for users to understand its backend-specific limitations.
| @@ -612,7 +612,180 @@ def array(x, dtype=None): | |||
|
|
|||
|
|
|||
| def view(x, dtype=None): | |||
There was a problem hiding this comment.
This function has a complex implementation with backend-specific limitations (e.g., only supporting integer-to-integer conversions for non-constant tensors). It would be beneficial to add a docstring explaining its behavior and limitations for the OpenVINO backend, as per the Keras API design guidelines (lines 135-151).
keras/src/backend/openvino/numpy.py
Outdated
| shifted = x_uint | ||
| if i > 0: | ||
| shift = ov_opset.constant( | ||
| i * bits_per_elem, src_uint_type | ||
| ).output(0) | ||
| shifted = ov_opset.bitwise_right_shift( | ||
| x_uint, shift | ||
| ).output(0) |
There was a problem hiding this comment.
The if i > 0 check is not strictly necessary. A right shift by 0 is a no-op and will likely be optimized by the compiler. You can simplify this block by always performing the shift. This would make the code more linear and easier to read.
For example:
shift = ov_opset.constant(
i * bits_per_elem, src_uint_type
).output(0)
shifted = ov_opset.bitwise_right_shift(x_uint, shift).output(0)
keras/src/backend/openvino/numpy.py
Outdated
| leading = ov_opset.slice( | ||
| old_shape, | ||
| ov_opset.constant([0], Type.i64).output(0), | ||
| ov_opset.constant([-1], Type.i64).output(0), | ||
| ov_opset.constant([1], Type.i64).output(0), | ||
| ov_opset.constant([0], Type.i64).output(0), | ||
| ).output(0) | ||
| last_dim = ov_opset.gather( | ||
| old_shape, | ||
| ov_opset.constant([-1], Type.i64).output(0), | ||
| ov_opset.constant(0, Type.i64).output(0), | ||
| ).output(0) |
There was a problem hiding this comment.
This block of code for splitting a shape into its leading dimensions and the last dimension is duplicated in _view_int_contract (lines 746-757). To improve maintainability and reduce code duplication, consider extracting this logic into a private helper function. This would be consistent with existing patterns in this file, like _resolve_axis, and would also improve readability by giving a name to this operation.
keras/src/backend/openvino/numpy.py
Outdated
| if i > 0: | ||
| shift = ov_opset.constant( | ||
| i * bits_per_elem, dst_uint_type | ||
| ).output(0) | ||
| byte_i = ov_opset.bitwise_left_shift(byte_i, shift).output(0) | ||
| result = byte_i if result is None else ( | ||
| ov_opset.bitwise_or(result, byte_i).output(0) | ||
| ) |
There was a problem hiding this comment.
This block can be simplified for better readability and maintainability:
- The
if i > 0check for shifting is not necessary, as a left shift by 0 is a no-op. Removing the conditional would make the code more linear. - The
result = byte_i if result is None else ...pattern for initialization can be made cleaner. You could initializeresultto a tensor of zeros with the correct shape before the loop, and then usebitwise_oron every iteration.
rishi-sangare
force-pushed
the
feat/openvino-numpy-view
branch
3 times, most recently
from
f7bdc1c to
f7ad5c6
Compare
rishi-sangare
force-pushed
the
feat/openvino-numpy-view
branch
from
f7ad5c6 to
a5ee876
Compare
4 participants
Summary
Implements
numpy.viewfor the OpenVINO backend, resolving theNotImplementedErrorthat was previously raised.Implementation approach
Since OpenVINO's opset doesn't include a native bitcast operation, the implementation uses three complementary strategies:
numpy.ndarray.view()for lossless bitwise reinterpretation. This handles all dtype pairs including float↔integer.ov_opset.convert, which preserves bit patterns for signed/unsigned pairs of the same width (e.g. int32↔uint32).Tests
Removes four entries from
excluded_concrete_tests.txt:NumpyDtypeTest::test_viewNumpyOneInputOpsCorrectnessTest::test_viewNumpyOneInputOpsDynamicShapeTest::test_viewNumpyOneInputOpsStaticShapeTest::test_viewVerified locally with OpenVINO that all three paths (constant folding, int expand, int contract) produce correct results matching numpy's view behavior.
Fixes #34217