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 methods for removing parameters from Function #3854

Merged
merged 13 commits into from
Feb 4, 2021
Merged

Ngraph: add methods for removing parameters from Function #3854

merged 13 commits into from
Feb 4, 2021

Conversation

sadolini
Copy link
Contributor

No description provided.

@openvino-pushbot openvino-pushbot added category: IE Tests OpenVINO Test: plugins and common category: Core OpenVINO Core (aka ngraph) labels Jan 14, 2021
GlebKazantaev
GlebKazantaev suggested changes Jan 18, 2021
View reviewed changes
ngraph/core/src/function.cpp Show resolved Hide resolved
ngraph/core/src/function.cpp Show resolved Hide resolved
itikhono
itikhono approved these changes Jan 18, 2021
View reviewed changes
Copy link
Contributor

@itikhono itikhono left a comment

Choose a reason for hiding this comment

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

in general LGTM

@sadolini sadolini requested a review from ilyachur January 25, 2021 07:55
@ilyachur
Copy link
Contributor

@sadolini Please fix CI

itikhono
itikhono reviewed Jan 27, 2021
View reviewed changes
Copy link
Contributor

@itikhono itikhono left a comment

Choose a reason for hiding this comment

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

Deleting parameters may change their indexation, we do not have tests for this.
Could you please check this case?
Also, it's better to verify for real function (full pipeline: read -> ngraph -> deleting parameter -> validate_and_infer_type call -> conversion to cnn network -> inference)

GlebKazantaev
GlebKazantaev approved these changes Feb 2, 2021
View reviewed changes
Copy link
Contributor

@GlebKazantaev GlebKazantaev left a comment

Choose a reason for hiding this comment

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

nGraph part looks good.

@sadolini sadolini assigned sadolini and ilyachur and unassigned sadolini Feb 3, 2021
@ilyachur ilyachur merged commit dc1b605 into openvinotoolkit:master Feb 4, 2021
@ilyachur ilyachur added this to the 2021.3 milestone Feb 4, 2021
@sadolini sadolini deleted the ngraph_parameter_remove branch February 4, 2021 07:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iefode iefode iefode approved these changes

@itikhono itikhono itikhono approved these changes

@GlebKazantaev GlebKazantaev GlebKazantaev approved these changes

@ilyachur ilyachur ilyachur approved these changes

@slyalin slyalin Awaiting requested review from slyalin

Assignees

@ilyachur ilyachur

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

Successfully merging this pull request may close these issues.
6 participants
@sadolini @ilyachur @GlebKazantaev @itikhono @iefode @openvino-pushbot