Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Op][Core] Add ScatterNDUpdate-14 core and reference #23754

Merged
merged 21 commits into from
Apr 17, 2024
Merged

[Op][Core] Add ScatterNDUpdate-14 core and reference #23754

merged 21 commits into from
Apr 17, 2024

Conversation

mmikolajcz
Copy link
Contributor

Details:

  • Add core implementation for ScatterNDUpdates-14
  • Add reference implementation for ScatterNDUpdates-14

Tickets:

  • 111098
  • 111091
  • 111093

@github-actions github-actions bot added category: Core OpenVINO Core (aka ngraph) category: IE Tests OpenVINO Test: plugins and common category: TEMPLATE OpenVINO Template plugin category: CPP API OpenVINO CPP API bindings labels Mar 28, 2024
@mmikolajcz mmikolajcz added category: Opset OpenVINO Opset and removed category: Core OpenVINO Core (aka ngraph) category: IE Tests OpenVINO Test: plugins and common category: TEMPLATE OpenVINO Template plugin category: CPP API OpenVINO CPP API bindings labels Mar 28, 2024
@mmikolajcz mmikolajcz added this to the 2024.2 milestone Mar 28, 2024
rkazants
rkazants requested changes Mar 30, 2024
View reviewed changes
Copy link
Member

@rkazants rkazants left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks not covering TF case with duplicating indices: https://www.tensorflow.org/api_docs/python/tf/raw_ops/TensorScatterUpdate. We initially agreed to add such support not only reduction modes
Will we support it?

@github-actions github-actions bot added category: Core OpenVINO Core (aka ngraph) category: IE Tests OpenVINO Test: plugins and common category: TEMPLATE OpenVINO Template plugin category: CPP API OpenVINO CPP API bindings labels Apr 4, 2024
@mmikolajcz
Copy link
Contributor Author

It looks not covering TF case with duplicating indices: https://www.tensorflow.org/api_docs/python/tf/raw_ops/TensorScatterUpdate. We initially agreed to add such support not only reduction modes Will we support it?

Yes, sorry, forgot to add test cases for that scenario: https://github.com/openvinotoolkit/openvino/pull/23754/files#diff-b3357cd694cf99989466fe2fb084606a048a1af52c233b36312e3aac6be2a6c8R257-R325
In case of indices duplicates in copy mode, only last update for given duplicated index will be used. Existing reference implementation seem to support this case.
In case you have some cases from TF that should be added to those test suites, feel free to contact me, I'll update them

@rkazants
Copy link
Member

rkazants commented Apr 4, 2024

It looks not covering TF case with duplicating indices: https://www.tensorflow.org/api_docs/python/tf/raw_ops/TensorScatterUpdate. We initially agreed to add such support not only reduction modes Will we support it?

Yes, sorry, forgot to add test cases for that scenario: https://github.com/openvinotoolkit/openvino/pull/23754/files#diff-b3357cd694cf99989466fe2fb084606a048a1af52c233b36312e3aac6be2a6c8R257-R325 In case of indices duplicates in copy mode, only last update for given duplicated index will be used. Existing reference implementation seem to support this case. In case you have some cases from TF that should be added to those test suites, feel free to contact me, I'll update them

Cool, please also update specification with such a case. Should we add additional attribute to ScatterNDUpdate for such mode?
Please also share a link to updated specification.

Best regards,
Roman

@mmikolajcz
Copy link
Contributor Author

mmikolajcz commented Apr 5, 2024 •
edited
Loading

It looks not covering TF case with duplicating indices: https://www.tensorflow.org/api_docs/python/tf/raw_ops/TensorScatterUpdate. We initially agreed to add such support not only reduction modes Will we support it?

Yes, sorry, forgot to add test cases for that scenario: https://github.com/openvinotoolkit/openvino/pull/23754/files#diff-b3357cd694cf99989466fe2fb084606a048a1af52c233b36312e3aac6be2a6c8R257-R325 In case of indices duplicates in copy mode, only last update for given duplicated index will be used. Existing reference implementation seem to support this case. In case you have some cases from TF that should be added to those test suites, feel free to contact me, I'll update them

Cool, please also update specification with such a case. Should we add additional attribute to ScatterNDUpdate for such mode? Please also share a link to updated specification.

Best regards, Roman

You can check specification PR here, it mentions behavior in case of duplicate indices and added attribute used to select reduction modes.

@mmikolajcz mmikolajcz marked this pull request as ready for review April 11, 2024 08:13
@mmikolajcz mmikolajcz requested review from a team as code owners April 11, 2024 08:13
praasz
praasz requested changes Apr 12, 2024
View reviewed changes
src/plugins/template/tests/functional/op_reference/scatter_nd_update.cpp Outdated Show resolved Hide resolved
src/plugins/template/tests/functional/op_reference/scatter_nd_update.cpp Outdated Show resolved Hide resolved
src/plugins/template/tests/functional/op_reference/scatter_nd_update.cpp Show resolved Hide resolved
src/plugins/template/tests/functional/op_reference/scatter_nd_update.cpp Outdated Show resolved Hide resolved
src/core/tests/type_prop/scatter_nd_update.cpp Outdated Show resolved Hide resolved
src/plugins/template/tests/functional/op_reference/scatter_nd_update.cpp Outdated Show resolved Hide resolved
src/core/reference/include/openvino/reference/scatter_nd_update.hpp Outdated Show resolved Hide resolved
src/core/src/op/scatter_nd_update.cpp Outdated Show resolved Hide resolved
src/core/src/op/scatter_nd_update.cpp Outdated Show resolved Hide resolved
src/core/tests/type_prop/scatter_nd_update.cpp Show resolved Hide resolved
@mmikolajcz mmikolajcz requested review from praasz and rkazants April 15, 2024 14:41
@mmikolajcz mmikolajcz requested a review from mitruska April 16, 2024 11:21
mitruska
mitruska approved these changes Apr 17, 2024
View reviewed changes
Copy link
Contributor

@mitruska mitruska left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a reminder to register shape_infer for CPU (can be done as a follow up).
There is a common shape_infer for ScatterNDBase that can be reused for the new version of the op.

openvino/src/plugins/intel_cpu/src/shape_inference/shape_inference.cpp

Line 470 in 34825fe

_OV_OP_SHAPE_INFER_MASK_REG(opset4::ScatterNDUpdate, ShapeInferTA, util::bit::mask()),

@mmikolajcz mmikolajcz added this pull request to the merge queue Apr 17, 2024
Merged via the queue into openvinotoolkit:master with commit 345a31c Apr 17, 2024
109 checks passed
@mmikolajcz mmikolajcz deleted the mateuszm/op/scatternd/core branch April 17, 2024 12:33
mvafin added a commit to mvafin/openvino that referenced this pull request Apr 18, 2024
@mvafin
Revert "[Op][Core] Add ScatterNDUpdate-14 core and reference (openvin…
396c8ee 
…otoolkit#23754)"

This reverts commit 345a31c.
alvoron pushed a commit to alvoron/openvino that referenced this pull request Apr 29, 2024
@mmikolajcz @alvoron
[Op][Core] Add ScatterNDUpdate-14 core and reference (openvinotoolkit…
cc7ab6c 
…#23754)

### Details:
 - *Add core implementation for ScatterNDUpdates-14*
 - *Add reference implementation for ScatterNDUpdates-14*

### Tickets:
 - *111098*
 - *111091*
 - *111093*
@mitruska mitruska mentioned this pull request Oct 10, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@praasz praasz praasz approved these changes

@rkazants rkazants rkazants approved these changes

@mitruska mitruska mitruska approved these changes

Assignees
No one assigned
Labels
category: Core OpenVINO Core (aka ngraph) category: CPP API OpenVINO CPP API bindings category: IE Tests OpenVINO Test: plugins and common category: Opset OpenVINO Opset category: TEMPLATE OpenVINO Template plugin
Projects
None yet
Milestone
2024.2
Development

Successfully merging this pull request may close these issues.
4 participants
@mmikolajcz @rkazants @mitruska @praasz