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

[ONNX] GRU and RNN operators. #607

Merged
merged 20 commits into from
Jun 3, 2020
Merged

[ONNX] GRU and RNN operators. #607

merged 20 commits into from
Jun 3, 2020

Conversation

arogowie-intel
Copy link
Contributor

No description provided.

@arogowie-intel arogowie-intel added the ONNX Related to support for ONNX standard. label May 26, 2020
@arogowie-intel arogowie-intel requested a review from a team May 26, 2020 15:26
@arogowie-intel arogowie-intel self-assigned this May 26, 2020
@arogowie-intel arogowie-intel requested review from a team and itikhono May 27, 2020 13:20
@arogowie-intel
Update UTs.
8e189d2 
- Update few tests accuracy tolerance
- Update rnn_fwd_activations with new reference values and model.
@arogowie-intel
Merge remote-tracking branch 'upstream/master'
bc2f372
mbencer
mbencer approved these changes Jun 1, 2020
View reviewed changes
Copy link
Contributor

@mbencer mbencer left a comment

Choose a reason for hiding this comment

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

Great objective abstraction for recurrent onnx ops!

ngraph/src/ngraph/frontend/onnx_import/utils/recurrent.cpp
{
const auto& ng_inputs = node.get_ng_inputs();

m_map[OpInput::X] = ng_inputs.at(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add assert to ensure that all inputs have static shape?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Maybe not all are required, I'll review it once again and try to assert only what's necessary.

jane-intel
jane-intel approved these changes Jun 2, 2020
View reviewed changes
Copy link
Contributor

@jane-intel jane-intel left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, but please resolve the comments and conflicts

itikhono
itikhono reviewed Jun 2, 2020
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.

LGTM, but OpenVINO don't support W/R/B as inputs, please add several tests where W/R/B are Constants.

@arogowie-intel
Copy link
Contributor Author

LGTM, but OpenVINO don't support W/R/B as inputs, please add several tests where W/R/B are Constants.

@itikhono I've just added one such test case, and although I don't get errors about transformation, IE_CPU produces bad results.

@itikhono
Copy link
Contributor

itikhono commented Jun 2, 2020

LGTM, but OpenVINO don't support W/R/B as inputs, please add several tests where W/R/B are Constants.

@itikhono I've just added one such test case, and although I don't get errors about transformation, IE_CPU produces bad results.

:( We need to find the root cause. I'll investigate it today.

@itikhono itikhono self-requested a review June 2, 2020 12:59
@arogowie-intel
Copy link
Contributor Author

The IE_CPU test with W,R as constants will fail due to the lack of support for multioutput in nG IE backend. Nevertheless the first output provides correct results.

@lazarevevgeny lazarevevgeny merged commit 3a80f04 into openvinotoolkit:master Jun 3, 2020
@ilya-lavrenov ilya-lavrenov added category: Core OpenVINO Core (aka ngraph) feature New feature request labels Jun 4, 2020
@ilya-lavrenov ilya-lavrenov added this to the 2020.4 milestone Jun 4, 2020
redradist pushed a commit to redradist/openvino that referenced this pull request Oct 6, 2023
@apavliuk55
[NVIDIA] Add Gather-8 (negative indices) support (openvinotoolkit#607)
13f6ac7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@postrational postrational postrational approved these changes

@mbencer mbencer mbencer approved these changes

@mitruska mitruska mitruska approved these changes

@itikhono itikhono itikhono approved these changes

@tomdol tomdol tomdol approved these changes

@tsocha tsocha tsocha approved these changes

@jane-intel jane-intel jane-intel approved these changes

Assignees

@arogowie-intel arogowie-intel

Labels
category: Core OpenVINO Core (aka ngraph) feature New feature request ONNX Related to support for ONNX standard.
Projects
None yet
Milestone
2020.4
Development

Successfully merging this pull request may close these issues.
10 participants
@arogowie-intel @itikhono @postrational @mbencer @mitruska @tomdol @tsocha @jane-intel @ilya-lavrenov @lazarevevgeny