-
Notifications
You must be signed in to change notification settings - Fork 19.7k
[OpenVINO backend] Fix and enable numpy.rot90 #21967
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
base: master
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @PES2UG23CS205, 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 significantly enhances the Keras OpenVINO backend by introducing robust and correctly functioning support for the 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. 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
The pull request removes the NumPyTestRot90 entry from the keras/src/backend/openvino/excluded_concrete_tests.txt file, indicating that this test is no longer excluded for the OpenVINO backend. There were no review comments provided.
|
Something weird happened here. Github shows that both files are fully changed. My guess is that the line breaks changed from Unix still to DOS style. Can you undo this and rebase? Thanks! |
|
Thanks for pointing this out. You’re right, this looks like a line-ending (CRLF vs LF) issue. |
2e62ad8 to
7c8580a
Compare
|
Thanks for pointing this out it was a CRLF vs LF issue. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #21967 +/- ##
==========================================
- Coverage 82.68% 82.67% -0.02%
==========================================
Files 588 588
Lines 61491 61582 +91
Branches 9629 9640 +11
==========================================
+ Hits 50846 50915 +69
- Misses 8152 8174 +22
Partials 2493 2493
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:
|
|
I attempted to run api_gen.py locally, but hit a known TensorFlow/JAX dependency conflict (ml_dtypes version mismatch). |
keras/src/backend/openvino/numpy.py
Outdated
| end_mask=all_ones_mask, | ||
| ) | ||
| return OpenVINOKerasTensor(result.output(0)) | ||
| def rot90(x, k=1, axes=(0, 1)): |
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.
Rename x to array. This is what's making the tests fail.
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 review!
I’ve renamed the argument from x to array to match NumPy’s signature and pushed an updated commit.
I can run it myself. Just curious though, can't you upgrade TensorFlow? |
|
I’ve fixed the OpenVINO |
This PR fixes and enables numpy.rot90 support for the OpenVINO backend in Keras.
The implementation follows NumPy semantics and aligns with existing OpenVINO backend patterns by expressing rot90 using supported OpenVINO ops.
What was done
Implemented numpy.rot90 in keras/src/backend/openvino/numpy.py
Normalized and validated k and axes to match NumPy behavior
Implemented rotation using transpose and reverse OpenVINO ops
Removed NumPyTestRot90 from excluded_concrete_tests.txt
Fixed OpenVINO test failures related to rot90