Add support for dynamic dimensions in ops.slice.compute_output_spec.#22154
Add support for dynamic dimensions in ops.slice.compute_output_spec.#22154hertschuh merged 1 commit intokeras-team:masterfrom
ops.slice.compute_output_spec.#22154Conversation
Also added verification that the shape of the inputs, the `shape` parameter and the `indices` have the same length.
Summary of ChangesHello @hertschuh, 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 improves 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. Changelog
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
|
![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 enhances ops.slice.compute_output_spec to support dynamic dimensions and adds input validation. The changes correctly handle dynamic shapes when shape contains -1 and start_indices is a KerasTensor. The added tests cover these new scenarios.
I've identified a potential issue in the validation logic for start_indices when it's a KerasTensor, where its length isn't checked against the input rank. I've provided a suggestion to make this validation more robust.
| if hasattr(start_indices, "__len__") and len(start_indices) != len( | ||
| inputs.shape | ||
| ): | ||
| raise ValueError( | ||
| "When using -1 in `shape`, `start_indices` should not be a " | ||
| "KerasTensor. " | ||
| "The number of dimensions in `start_indices` must match the " | ||
| "number of dimensions in `inputs`. Received " | ||
| f"start_indices={start_indices} and inputs.shape={inputs.shape}" | ||
| ) |
There was a problem hiding this comment.
The validation for start_indices does not correctly handle KerasTensor inputs. The hasattr(start_indices, "__len__") check returns False for a KerasTensor, causing the validation of its length against the input rank to be skipped. This could lead to runtime errors or incorrect behavior if a KerasTensor with an incorrect number of indices is provided.
I suggest a more robust validation that explicitly handles KerasTensor and provides clearer error messages. It would also be good to add a test case for an invalid KerasTensor start_indices to test_slice_invalid_inputs.
| if hasattr(start_indices, "__len__") and len(start_indices) != len( | |
| inputs.shape | |
| ): | |
| raise ValueError( | |
| "When using -1 in `shape`, `start_indices` should not be a " | |
| "KerasTensor. " | |
| "The number of dimensions in `start_indices` must match the " | |
| "number of dimensions in `inputs`. Received " | |
| f"start_indices={start_indices} and inputs.shape={inputs.shape}" | |
| ) | |
| num_dims = len(inputs.shape) | |
| indices_len = None | |
| if isinstance(start_indices, KerasTensor): | |
| if len(start_indices.shape) != 1: | |
| raise ValueError( | |
| "Argument `start_indices` must be a 1D tensor, but got " | |
| f"a tensor of rank {len(start_indices.shape)}." | |
| ) | |
| indices_len = start_indices.shape[0] | |
| elif hasattr(start_indices, "__len__"): | |
| indices_len = len(start_indices) | |
| if indices_len is not None and indices_len != num_dims: | |
| raise ValueError( | |
| "The number of values in `start_indices` must match the rank " | |
| f"of `inputs`. Expected {num_dims} but got {indices_len}. " | |
| f"Received: start_indices={start_indices}, " | |
| f"inputs.shape={inputs.shape}" | |
| ) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #22154 +/- ##
=======================================
Coverage 82.78% 82.79%
=======================================
Files 592 592
Lines 63357 63365 +8
Branches 9941 9945 +4
=======================================
+ Hits 52452 52460 +8
Misses 8354 8354
Partials 2551 2551
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
5 participants
Also added verification that the shape of the inputs, the
shapeparameter and theindiceshave the same length.