-
Notifications
You must be signed in to change notification settings - Fork 19.6k
[OpenVINO backend] support tob_k and enable tests #21436
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?
[OpenVINO backend] support tob_k and enable tests #21436
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #21436 +/- ##
=======================================
Coverage 82.76% 82.77%
=======================================
Files 565 565
Lines 55378 55386 +8
Branches 8646 8646
=======================================
+ Hits 45833 45845 +12
+ Misses 7442 7439 -3
+ Partials 2103 2102 -1
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:
|
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.
@fchollet, looks good to me. Recommend to merge
@Mohamed-Ashraf273 can you please resolve the conficts? |
/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 PR implements top_k
for the OpenVINO backend and enables the math operations test suite. The implementation of top_k
looks correct.
My main feedback is regarding the test exclusions. While it's a valid strategy to enable a whole test file and then exclude failing tests, I noticed that test_logsumexp
is being excluded, even though logsumexp
seems to be implemented. This might indicate a bug in the existing implementation. Please see my detailed comment.
Also, there's a small typo in the PR title (tob_k
instead of top_k
).
MathOpsCorrectnessTest::test_istft5 | ||
MathOpsCorrectnessTest::test_istft6 | ||
MathOpsCorrectnessTest::test_logdet | ||
MathOpsCorrectnessTest::test_logsumexp |
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.
I see that MathOpsCorrectnessTest::test_logsumexp
is being added to the exclusion list. However, the logsumexp
function appears to be implemented in the OpenVINO backend (keras/src/backend/openvino/math.py
).
Could you clarify why this test is being excluded? If the implementation is correct, this test should be passing. If it's failing, it might indicate a bug in the logsumexp
implementation that should be investigated.
2dfc78b
to
f0131f0
Compare
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.
@fchollet, looks good to me. Recommend to merge
@rkazants
@fchollet
@mattdangerw