Added openvino backend support for numpy.median (Issue #30115) #21379
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #21379 +/- ##
==========================================
- Coverage 82.67% 82.66% -0.02%
==========================================
Files 565 565
Lines 55064 55132 +68
Branches 8569 8579 +10
==========================================
+ Hits 45525 45573 +48
- Misses 7441 7457 +16
- Partials 2098 2102 +4
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:
|
Swaglun
force-pushed
the
median_support_ov
branch
from
43ac109 to
d92b60a
Compare
|
@rkazants PR is ready for review. Apologies for all the commits, it took me a second to fix my local testing to run on openvino backend on macOS. |
Swaglun
changed the title
|
/gemini review |
![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 numpy.median to the OpenVINO backend. The implementation handles scenarios for the axis and keepdims parameters. A critical bug was identified in the logic for handling multiple reduction axes, making that code path unreachable. Additionally, high-severity issues were found where the code relies on static shape information, leading to failures with dynamic input shapes. Suggestions have been provided to address these points and improve robustness.
| axis_as_range, ov_axis, ov_opset.constant([0], Type.i32) | ||
| ).output(0) | ||
| # only move axis dims if tuple contains more than 1 axis | ||
| if ov_axis_positive.get_partial_shape().rank.get_length() > 1: |
There was a problem hiding this comment.
The condition ov_axis_positive.get_partial_shape().rank.get_length() > 1 always evaluates to False because ov_axis_positive is 1-dimensional. This makes the code block for handling multiple reduction axes unreachable. Check the number of axes provided in the axis list instead.
if len(axis) > 1:| k_value = ( | ||
| x.get_partial_shape().get_dimension(index=axis_norm).get_length() | ||
| ) |
There was a problem hiding this comment.
The value of k_value is derived from the static partial shape. For dynamic shapes, derive k_value from the tensor's shape at runtime using ov_opset.shape_of.
x_shape = ov_opset.shape_of(x, Type.i32).output(0)
k_value = ov_opset.gather(
x_shape,
ov_opset.constant(axis_norm, Type.i32).output(0),
ov_opset.constant(0, Type.i32).output(0),
).output(0)| if median_eval.get_partial_shape().rank.get_length() != x_rank_org: | ||
| median_eval = ov_opset.unsqueeze(median_eval, ov_axis).output(0) |
There was a problem hiding this comment.
This condition relies on static rank information from get_partial_shape() and will fail for inputs with dynamic shapes. When keepdims=True, the rank of the output should be preserved by consistently adding the reduction axis back with ov_opset.unsqueeze.
median_eval = ov_opset.unsqueeze(median_eval, ov_axis).output(0)
Added decomposition for numpy.median using OpenVINO backend.