-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Fix GatherND broadcasting for batch dimensions #32660
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a test in src/core/tests/type_prop/gather_nd.cpp.
I added |
| NODE_VALIDATION_CHECK( | ||
| op, | ||
| DimType::broadcast_merge(output_dims[dim_idx], data_pshape[dim_idx], indices_pshape[dim_idx]), | ||
| "Batch dimensions of data and indices must be broadcastable."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GatherND op specification mentions that batch needs to be equal, so it would also need to be updated.
Do we know whether reference/plugin implementations would handle such broadcast? I think it would make sense to add few tests to make sure result of such broadcasted GatherND would produce expected outputs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed refernece implementation, docs and CPU plugin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually such change is request of new version of operator.
It has to be check if three is no compatibility with previous version of OV.
If this is for ONNX then mayb ONNX FE can process node on front end to provide compatible graph for OV
… implementation, CPU plugin, tests and documentation
|
@nshchego, could you please review the CPU part? |
| NODE_VALIDATION_CHECK( | ||
| op, | ||
| DimType::broadcast_merge(output_dims[dim_idx], data_pshape[dim_idx], indices_pshape[dim_idx]), | ||
| "Batch dimensions of data and indices must be broadcastable."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually such change is request of new version of operator.
It has to be check if three is no compatibility with previous version of OV.
If this is for ONNX then mayb ONNX FE can process node on front end to provide compatible graph for OV
Details:
Problem:
OpenVINO rejects ONNX models with GatherND when batch dimensions are broadcastable but not equal (e.g., data=[1,3,4,4], indices=[2,2,2], batch_dims=1). However, ONNXRuntime accepts and successfully executes these models.
Root Cause:
Solution:
Implemented full broadcasting support for GatherND batch dimensions to match ONNX specification and ONNXRuntime behavior:
Shape Inference (gather_nd_shape_inference.hpp):
Reference Implementation (gather_nd.hpp):
CPU Plugin (gather_nd.cpp, gather_nd.h):
Documentation (gather-nd-5.rst, gather-nd-8.rst):
Tests:
Tickets: