Validate axis1 != axis2 in ops.diagonal#22593
Validate axis1 != axis2 in ops.diagonal#22593rstar327 wants to merge 1 commit 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 adds axis normalization and validation to the diagonal operation in keras/src/ops/numpy.py, specifically handling negative indices and ensuring axis1 and axis2 are not identical. The reviewer suggests using the canonicalize_axis utility instead of manual calculations to improve robustness and maintain consistency with the rest of the codebase.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #22593 +/- ##
=======================================
Coverage 83.30% 83.30%
=======================================
Files 596 596
Lines 67951 67989 +38
Branches 10577 10590 +13
=======================================
+ Hits 56604 56640 +36
- Misses 8600 8601 +1
- Partials 2747 2748 +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:
|
keerthanakadiri
added
the
stat:awaiting keras-eng
There was a problem hiding this comment.
@rstar327 Thank you for this contribution. Aligning the validation with NumPy's behavior for identical axes is a great fix for the issue mentioned.
As a community reviewer, I have a few suggestions to make this PR merge-ready:
Utility Usage: Please consider using the existing keras.src.utils.axis_utils.canonicalize_axis utility instead of manual axis normalization. It ensures consistency across the codebase and provides built-in bounds checking.
Coverage Improvement: The Codecov report indicates a 60% patch coverage failure. To resolve this, please add a specific test case in keras/src/ops/numpy_test.py that passes axis1 == axis2 to verify the ValueError is correctly raised.
Once these updates are implemented, the coverage issue should resolve, making it easier for the Keras engineering team to provide a final approval.
hertschuh
left a comment
hertschuh
left a comment
There was a problem hiding this comment.
Thanks for looking at this!
Please also add a unit test exercising this in this method:
https://github.com/keras-team/keras/blob/master/keras/src/ops/numpy_test.py#L5195
| ax1 = self.axis1 if self.axis1 >= 0 else self.axis1 + ndim | ||
| ax2 = self.axis2 if self.axis2 >= 0 else self.axis2 + ndim |
There was a problem hiding this comment.
please use ax1 = canonicalize_axis(self.axis1, ndim) which does some validation.
| x_ndim = len(x.shape) | ||
| ax1 = axis1 if axis1 >= 0 else axis1 + x_ndim | ||
| ax2 = axis2 if axis2 >= 0 else axis2 + x_ndim | ||
| if ax1 == ax2: | ||
| raise ValueError( | ||
| "`axis1` and `axis2` cannot be the same. " | ||
| f"Received: axis1={axis1}, axis2={axis2}" | ||
| ) |
There was a problem hiding this comment.
The current pattern used is to not have any code in these functions, they're supposed to delegate to the backend functions.
So please move to whichever backend doesn't throw a proper exception (tensorflow is sounds like).
There was a problem hiding this comment.
Actually, never mind. Validation is fine. But:
- use
canonicalize_axishere - move it before the
if any_symbolic_tensor - remove the other one in
compute_output_shape, it's not longer needed
hertschuh
added
stat:awaiting response from contributor
and removed
stat:awaiting keras-eng
6 participants
Summary
Add validation that
axis1andaxis2are not the same inkeras.ops.diagonal, matching NumPy's behavior (ValueError: axis1 and axis2 cannot be the same).Previously, passing
axis1 == axis2on eager TensorFlow tensors caused a crypticInvalidArgumentErrorfrom the transpose op. Now it raises a clearValueErrorfor both symbolic and eager inputs.Fixes #22528