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.

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

[ONNX] Return NMS-5 generation #3003

Merged
2 commits merged into from Nov 10, 2020
Merged

[ONNX] Return NMS-5 generation #3003

2 commits merged into from Nov 10, 2020

Conversation

ghost
Copy link

@ghost ghost commented Nov 6, 2020

No description provided.

@ghost ghost added this to the 2021.2 milestone Nov 6, 2020
@ghost ghost requested review from tsocha, mandrono and a team November 6, 2020 09:42
@ghost ghost assigned tsocha and mandrono Nov 6, 2020
@ghost
Copy link
Author

ghost commented Nov 6, 2020

@tsocha @mandrono could you please take a look?
I believe this change is not needed anymore because the NMS-5 is already supported on CPU (#2768). @mandrono am I right?
BTW, the initial change with generating NMS-4 looked strange from my perspective, because onnx_importer is a common part, and used not only by CPU plugin. This change has broken very critical models for VPU plugin because we have already moved to NMS-5 usage.

@openvino-pushbot openvino-pushbot added the category: Core OpenVINO Core (aka ngraph) label Nov 6, 2020
@tsocha tsocha requested a review from postrational November 6, 2020 10:34
@tsocha
Copy link
Contributor

tsocha commented Nov 6, 2020

@AndrewBakalinIntel
Originaly the reason why ONNX Importer switched to NMS-4 is that because NMS-5 was merged into master without required transformations.
As you can see you have to xfail a model when NMS-4 is changed to NMS-5 and this is a regression, we should avoid this IMO.

From the logs I can see that there is a type conversion missing.
Could you add it to this file?
https://github.com/AndrewBakalinIntel/openvino/blob/ab/return-nms-5/inference-engine/src/transformations/src/transformations/convert_precision.cpp#L224-L238

@ghost ghost requested review from GlebKazantaev and ilyachur as code owners November 6, 2020 10:59
@ghost
Copy link
Author

ghost commented Nov 6, 2020

@tsocha @postrational CI is still failing, most probably because #2450 is still not merged.
It is a big problem that the operation was merged without appropriate transformations, but I still think we need to keep generating NMS-5 and solve the problem appears instead of masking them (at least it will help us to track all the current issues), because it anyway should be supported on CPU for the upcoming release. So I suggest to suppress the failing models (as it was actually for some time before), it will unblock a lot of critical models for VPU and we will continue to track the remained issues.

@mandrono
Copy link

mandrono commented Nov 6, 2020

@AndrewBakalinIntel
Yes, NMS-5 is already supported on CPU plug-in

@ghost ghost merged commit 8d4f8c4 into openvinotoolkit:master Nov 10, 2020
mryzhov pushed a commit to mryzhov/openvino that referenced this pull request Dec 16, 2020
* [ONNX] Return NMS-5 generation

* Disable failed models
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: Core OpenVINO Core (aka ngraph)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants