Move slogdet to linalg instead of numpy#22678
Move slogdet to linalg instead of numpy#22678goyaladitya05 wants to merge 2 commits intokeras-team:masterfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request migrates the slogdet operation from keras.ops.numpy to keras.ops.linalg across all backends and introduces a manual implementation for the OpenVINO backend. Feedback points out missing imports in the OpenVINO backend and an incorrect output dtype specification in the Slogdet operation for complex and integer inputs. Additionally, the slogdet docstring must include a code example to comply with the Keras API design guidelines.
| LinalgOpsCorrectnessTest::test_norm_2_-2 | ||
| LinalgOpsCorrectnessTest::test_norm_2_2 | ||
| LinalgOpsCorrectnessTest::test_norm_2_nuc | ||
| LinalgOpsCorrectnessTest::test_slogdet |
There was a problem hiding this comment.
apperently after improvisation, this test fails for the openvino backend (the backend implementaion might have bugs). To keep the scope of this PR solely for the refactor, added it to the exclusion list for now. Will address this in #22673.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #22678 +/- ##
==========================================
+ Coverage 77.13% 83.04% +5.90%
==========================================
Files 596 596
Lines 68989 68999 +10
Branches 10790 10790
==========================================
+ Hits 53216 57297 +4081
+ Misses 12939 8886 -4053
+ Partials 2834 2816 -18
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.
The issue is that slogdet has been in numpy for years. Moving is would be a breaking change.
It would stop working for people who have been doing keras.ops.numpy.slogdet.
And also, Functional models serialized with slodget will refer to keras.src.ops.numpy.Slogdet, so we can't move it otherwise deserialization will fail.
What we can do is add an alias in the export. If we really want to move the code to linalg.py, it's doable, but we need to have a alias of the Slogdet class in numpy.py (I think importing it would be enough).
Then I think it would be better to leave it as it is, because it's not really causing any issues at the moment. |
Description
slogdetbelongs to the linalg submodule (as in NumPy, JAX, PyTorch, and TensorFlow), but was mistakenly placed innumpy.pyacross all backends. This PR moves it tolinalg.pywhere it belongs. Also improvised the tests.Updated
logdetinmath.py(JAX and NumPy backends) to importslogdetfromlinalginstead ofnumpy.Contributor Agreement
Please check all boxes below before submitting your PR for review: