[OpenVINO] Implement logdet op#22673
[OpenVINO] Implement logdet op#22673goyaladitya05 wants to merge 6 commits intokeras-team:masterfrom
Conversation
![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 the logdet function for the OpenVINO backend and enables its associated correctness tests. A logic error was identified in the implementation: logdet currently returns the logarithm of the absolute value of the determinant, whereas it should follow the mathematical definition of log(det(x)), returning NaN for tensors with a negative determinant.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements the logdet operation for the OpenVINO backend and enables its corresponding correctness tests. The implementation utilizes slogdet and handles negative determinants by returning NaN. A review comment highlights that the current implementation is not robust to dynamic shapes because the underlying slogdet function uses a Python loop over matrix dimensions, which will fail if the inner dimensions are not statically known.
| def logdet(x): | ||
| # OpenVINO does not have a direct logdet op | ||
| # Can be modified when there is a native op in ov_opset | ||
| from keras.src.backend.openvino.numpy import slogdet | ||
|
|
||
| sign, logabsdet = slogdet(x) | ||
| neg_det = sign < 0 | ||
| # When determinant is negative return NaN instead of the log-abs value. | ||
| nan_value = convert_to_tensor(np.nan, dtype=logabsdet.dtype) | ||
|
|
||
| return select([neg_det], [nan_value], default=logabsdet) |
There was a problem hiding this comment.
The logdet implementation relies on slogdet from keras.src.backend.openvino.numpy. While this is a reasonable fallback given the lack of a native OpenVINO op, the current implementation of slogdet in this backend (lines 5255-5448 of numpy.py) uses a Python loop over the matrix dimension n. This means logdet will only work for matrices with a statically known inner dimension. If x has a dynamic shape for its last two dimensions, slogdet will raise an exception at line 5275. While this might be an existing limitation of the backend's linear algebra support, it's worth noting that this implementation is not fully robust to dynamic shapes.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #22673 +/- ##
=======================================
Coverage 82.93% 82.93%
=======================================
Files 596 596
Lines 69083 69091 +8
Branches 10803 10803
=======================================
+ Hits 57291 57300 +9
Misses 8967 8967
+ Partials 2825 2824 -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:
|
google-ml-butler
bot
added
kokoro:force-run
ready to pull
google-ml-butler
bot
removed
the
ready to pull
google-ml-butler
bot
added
the
ready to pull
|
This is marked as draft, is it ready to merge? |
Hi @hertschuh, |
4 participants
Description
This PR implements logdet op for the OpenVINO backend. Since there does not exist a not a native op for this in the currently available ov opsets and the implementation would require decomposition similar to slogdet, so we delegate to slogdet for now.
Closes: openvinotoolkit/openvino/issues/35312
Contributor Agreement
Please check all boxes below before submitting your PR for review: