[OpenVINO backend] Support for nanarmax and nanar min #22383
[OpenVINO backend] Support for nanarmax and nanar min #22383andersendsa wants to merge 2 commits intokeras-team:masterfrom
Conversation
Summary of ChangesHello, 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 enhances the OpenVINO backend's compatibility with NumPy operations by introducing support for 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 adds support for nanargmax and nanargmin to the OpenVINO backend, which is a great addition. The implementation correctly handles NaN values by replacing them with infinities. The changes also include enabling the corresponding tests and fixing dynamic axis resolution.
I've identified a few areas for improvement:
- There's some code duplication in the logic for resolving static axis values, which could be refactored into a helper function.
- The error message for dynamic axes could be more descriptive, as per the repository's style guide.
- There's an edge case with an empty
axistuple that behaves differently from NumPy. - A small redundant check in both new functions can be removed.
Overall, the changes are good, and with these refinements, the code will be more robust and maintainable.
Note: Security Review did not run due to the size of the PR.
| if axis is None: | ||
| return OpenVINOKerasTensor(x) |
There was a problem hiding this comment.
When axis is an empty tuple () or list [], _resolve_axis returns None for the axis. In this case, the function returns the input tensor x unmodified. This behavior is inconsistent with NumPy's nanargmax, which raises a TypeError for an empty tuple axis. Returning the input tensor is likely incorrect as no reduction is performed. Consider raising a TypeError to match NumPy's behavior for this edge case. The same issue exists in the nanargmin implementation.
| if hasattr(axis, "__class__") and "Output" in axis.__class__.__name__: | ||
| axis_node = axis.get_node() | ||
| if hasattr(axis_node, "get_data"): | ||
| axis_val = axis_node.get_data() | ||
| if axis_val is not None: | ||
| if hasattr(axis_val, "ndim") and axis_val.ndim > 0 and axis_val.size > 0: | ||
| axis = int(axis_val[0]) | ||
| elif hasattr(axis_val, "size") and axis_val.size == 1: | ||
| axis = int(axis_val.item()) | ||
| else: | ||
| axis = int(axis_val) |
There was a problem hiding this comment.
This logic to resolve a static axis value from an OpenVINO tensor is duplicated in argmax (lines 537-551) and argmin (lines 584-598). To improve maintainability and reduce code redundancy, consider extracting this logic into a private helper function. This function could attempt to resolve the axis to an integer and be used in all three places.
Additionally, the type check hasattr(axis, "__class__") and "Output" in axis.__class__.__name__ is fragile. It's better to use isinstance(axis, ov.Output). The argmax and argmin functions use a redundant or condition with both checks. A single, robust isinstance check should be used in the new helper function.
| else: | ||
| axis = int(axis_val) | ||
| else: | ||
| raise ValueError("axis must be static for argmax") |
There was a problem hiding this comment.
According to the Keras API design guidelines (line 140), error messages should be contextual and actionable. This error message could be improved by providing more context, such as the type of axis received and what is expected. A similar improvement can be applied to the argmin function.
For example:
raise ValueError(
f"For the OpenVINO backend, `argmax` requires a static `axis` value, "
f"but received a dynamic tensor. Please provide a static integer "
f"for the `axis` argument."
)References
- Error messages should be contextual, informative, and actionable, explaining what happened, what was expected, and how the user can fix it. (link)
| if result_ov.get_element_type() != Type.i32: | ||
| nan_value = ov_opset.convert(nan_value, result_ov.get_element_type()) |
There was a problem hiding this comment.
The argmax function is configured to return indices of type i32. Therefore, result_ov.get_element_type() should always be Type.i32. This conditional check appears to be redundant and can be removed for simplification. A similar redundant check exists in the nanargmin implementation (lines 2956-2957).
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #22383 +/- ##
==========================================
- Coverage 82.96% 82.92% -0.04%
==========================================
Files 596 596
Lines 66269 66353 +84
Branches 10321 10348 +27
==========================================
+ Hits 54982 55026 +44
- Misses 8667 8692 +25
- Partials 2620 2635 +15
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:
|
|
hi @hertschuh this pr passes all the tests and is ready for review |
| if hasattr(axis, "__class__") and "Output" in axis.__class__.__name__: | ||
| axis_node = axis.get_node() | ||
| if hasattr(axis_node, "get_data"): | ||
| axis_val = axis_node.get_data() | ||
| if axis_val is not None: | ||
| if ( | ||
| hasattr(axis_val, "ndim") | ||
| and axis_val.ndim > 0 | ||
| and axis_val.size > 0 | ||
| ): | ||
| axis = int(axis_val[0]) | ||
| elif hasattr(axis_val, "size") and axis_val.size == 1: | ||
| axis = int(axis_val.item()) | ||
| else: | ||
| axis = int(axis_val) | ||
|
|
There was a problem hiding this comment.
Why was this added? Tensor axes are not supported.
| if isinstance(axis, ov.Output) or ( | ||
| hasattr(axis, "__class__") and "Output" in axis.__class__.__name__ | ||
| ): | ||
| axis_node = axis.get_node() | ||
| if hasattr(axis_node, "get_data"): | ||
| axis_val = axis_node.get_data() | ||
| if axis_val is not None: | ||
| if ( | ||
| hasattr(axis_val, "ndim") | ||
| and axis_val.ndim > 0 | ||
| and axis_val.size > 0 | ||
| ): | ||
| axis = int(axis_val[0]) | ||
| elif hasattr(axis_val, "size") and axis_val.size == 1: | ||
| axis = int(axis_val.item()) | ||
| else: | ||
| axis = int(axis_val) | ||
| else: | ||
| raise ValueError("axis must be static for argmax") | ||
| else: | ||
| raise ValueError("axis must be static for argmax") |
There was a problem hiding this comment.
Why was this added? Tensor axes are not supported.
| if isinstance(axis, ov.Output) or ( | ||
| hasattr(axis, "__class__") and "Output" in axis.__class__.__name__ | ||
| ): | ||
| axis_node = axis.get_node() | ||
| if hasattr(axis_node, "get_data"): | ||
| axis_val = axis_node.get_data() | ||
| if axis_val is not None: | ||
| if ( | ||
| hasattr(axis_val, "ndim") | ||
| and axis_val.ndim > 0 | ||
| and axis_val.size > 0 | ||
| ): | ||
| axis = int(axis_val[0]) | ||
| elif hasattr(axis_val, "size") and axis_val.size == 1: | ||
| axis = int(axis_val.item()) | ||
| else: | ||
| axis = int(axis_val) | ||
| else: | ||
| raise ValueError("axis must be static for argmin") | ||
| else: | ||
| raise ValueError("axis must be static for argmin") |
There was a problem hiding this comment.
Why was this added? Tensor axes are not supported.
4 participants
Details:
Implement nanarmax and nanarmin operation for the OpenVINO backend.
Closes : openvinotoolkit/openvino#34557