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

[NGraph] Add scatterNDUpdate and scatterUpdate reference implementations #1494

Merged
merged 1 commit into from
Aug 7, 2020
Merged

[NGraph] Add scatterNDUpdate and scatterUpdate reference implementations #1494

merged 1 commit into from
Aug 7, 2020

Conversation

mandrono
Copy link

@mandrono mandrono commented Jul 27, 2020 •
edited
Loading

Issue: 33266

@mandrono mandrono requested review from a team July 27, 2020 15:04
@mandrono mandrono added category: IE Tests OpenVINO Test: plugins and common category: Core OpenVINO Core (aka ngraph) labels Jul 27, 2020
@mandrono mandrono added this to the 2021.1 milestone Jul 27, 2020
@mandrono mandrono force-pushed the feature/mandrono/scatter_tests branch from a84f436 to 7b16a01 Compare July 28, 2020 07:12
@GlebKazantaev GlebKazantaev removed the request for review from a team July 28, 2020 08:42
@mandrono mandrono force-pushed the feature/mandrono/scatter_tests branch from 7b16a01 to 55daa29 Compare July 29, 2020 17:55
@mandrono
Copy link
Author

@dmitry-gorokhov @antonvor @lazarevevgeny @mvafin take a look please

antonvor
antonvor reviewed Jul 30, 2020
View reviewed changes
Copy link
Contributor

@antonvor antonvor left a comment

Choose a reason for hiding this comment

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

LGTM, but could you please answer my comments?

@lazarevevgeny lazarevevgeny requested a review from ilyachur July 30, 2020 07:34
@ilyachur ilyachur self-assigned this Jul 30, 2020
@mandrono mandrono force-pushed the feature/mandrono/scatter_tests branch from 55daa29 to cd4f520 Compare July 30, 2020 10:43
ilyachur
ilyachur approved these changes Jul 31, 2020
View reviewed changes
Copy link
Contributor

@ilyachur ilyachur left a comment

Choose a reason for hiding this comment

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

LGTM, Just a question before the merge. What is the type of MKLDNN primitive used for ScatterNGUpdate? As I can see you removed ScatterImpl but you add nothing new.

@mandrono
Copy link
Author

mandrono commented Jul 31, 2020 •
edited
Loading

LGTM, Just a question before the merge. What is the type of MKLDNN primitive used for ScatterNGUpdate? As I can see you removed ScatterImpl but you add nothing new.

All types of Scatter primitives used in nGraph implemented in mkldnn_scatter_update_node.h.
Please don't merge changes until we get approval from @dmitry-gorokhov

@mandrono mandrono force-pushed the feature/mandrono/scatter_tests branch 2 times, most recently from 1f09059 to ac902c9 Compare August 3, 2020 10:11
@mandrono mandrono force-pushed the feature/mandrono/scatter_tests branch from ac902c9 to 08e67e4 Compare August 7, 2020 09:47
@dmitry-gorokhov dmitry-gorokhov merged commit 4054364 into openvinotoolkit:master Aug 7, 2020
@mitruska mitruska mentioned this pull request Aug 24, 2020
Merged
mryzhov pushed a commit to mryzhov/openvino that referenced this pull request Aug 26, 2020
@mryzhov
[NGraph] Add scatterNDUpdate and scatterUpdate reference implementati…
8d33329 
…ons (openvinotoolkit#1494)
RomanZm pushed a commit to RomanZm/openvino that referenced this pull request Aug 28, 2020
[NGraph] Add scatterNDUpdate and scatterUpdate reference implementati…
fbb5758 
…ons (openvinotoolkit#1494)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@antonvor antonvor antonvor left review comments

@mvafin mvafin mvafin approved these changes

@ilyachur ilyachur ilyachur approved these changes

@dmitry-gorokhov dmitry-gorokhov dmitry-gorokhov approved these changes

@lazarevevgeny lazarevevgeny Awaiting requested review from lazarevevgeny

Assignees

@ilyachur ilyachur

Labels
category: Core OpenVINO Core (aka ngraph) category: IE Tests OpenVINO Test: plugins and common
Projects
None yet
Milestone
2021.1
Development

Successfully merging this pull request may close these issues.
5 participants
@mandrono @dmitry-gorokhov @ilyachur @mvafin @antonvor