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

Proposal reference implementation #3924

Merged
merged 49 commits into from
Feb 8, 2021
Merged

Proposal reference implementation #3924

merged 49 commits into from
Feb 8, 2021

Conversation

blesniewski
Copy link
Contributor

@blesniewski blesniewski commented Jan 19, 2021 •
edited
Loading

Reference implementation for Proposal

  • Reference implementation
  • Enable SLT:
  • enabled CPU SLT, added custom compare functions to only verify the outputs until the terminating values.
  • disabled Myriad SLT (the myriad implementation gives different outputs than rest of the implementations)
  • disabled GPU SLT (different probabilities results)
  • Unit tests coverage
  • Review the ngraph op class:
  • added check for inputs[0,1] shape in validate_and_infer_types, so we know we have valid anchor count
  • improved shape inference for cases with dynamic input shapes
  • Clarify documentation where needed:
  • explicitly state that for the first output, in case there is less outputs than space, there will be a terminating -1.
  • add types section for the Proposal v0 spec

@blesniewski blesniewski self-assigned this Jan 19, 2021
@blesniewski blesniewski marked this pull request as ready for review January 21, 2021 15:21
@blesniewski blesniewski requested a review from a team as a code owner January 21, 2021 15:21
@blesniewski blesniewski requested review from a team January 21, 2021 15:21
lazarevevgeny
lazarevevgeny suggested changes Jan 25, 2021
View reviewed changes
ngraph/core/src/op/proposal.cpp Show resolved Hide resolved
ngraph/core/src/op/proposal.cpp Outdated Show resolved Hide resolved
ngraph/core/src/op/proposal.cpp Outdated Show resolved Hide resolved
lazarevevgeny
lazarevevgeny suggested changes Feb 2, 2021
View reviewed changes
Copy link
Contributor

@lazarevevgeny lazarevevgeny left a comment

Choose a reason for hiding this comment

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

The spec looks good. But the shape infer function still needs improvements.

ngraph/core/src/op/proposal.cpp Outdated Show resolved Hide resolved
ngraph/core/src/op/proposal.cpp Outdated Show resolved Hide resolved
ngraph/test/type_prop/proposal.cpp Show resolved Hide resolved
@blesniewski blesniewski requested a review from ilyachur February 3, 2021 13:54
@iefode iefode self-requested a review February 4, 2021 08:58
@ilyachur ilyachur merged commit 210ad81 into openvinotoolkit:master Feb 8, 2021
@blesniewski blesniewski deleted the proposal_ref_impl branch February 8, 2021 08:13
LiyangLingIntel pushed a commit to Flex-plaidml-team/openvino that referenced this pull request Apr 23, 2021
@LiyangLingIntel
Proposal reference implementation (openvinotoolkit#3924)
86b0b70 
* Reference implementation for Proposal, enable CPU SLT

* code style fix

* add type prop test for invalid anchor count

* add unit test

* fix shapes in attribute test

* temp workaround- disable maring end of boxes list

* Disable CPU smoke test- spec misalignment

* code style fixes

* add some details to the specification

* disable myriadx proposal slt

* review changes, using usigned int and size_t

* improve proposal op shape inference to cover dynamic too, add unit test coverage

* remove unused variable in test body

* remove batch size in tests where its not used

* add post nms topn initialization in tests where it was missing

* review comments

* style fix

* style fix 2

* add tests, remove unused variables, change shape inference checks

* style fix

* add input tensors type checks and test coverage

* align input type in attribute and ngraphreader tests to match specification

* fix wrong dimension in error message

* proposalv4 ref impl

* enable single layer and unit tests for proposalv4 ref impl

* align output termination with cpu, enable cpu slt

* custom slt compares to detect less-than-predicted number of boxes

* custom slt compares to detect less-than-predicted number of boxes

* Clarify output termination in spec

* review comments

* smaller input data for unit tests

* add check for batch_dim being static

* disable gpu slt for proposal

* test data style fix

* test data style fix 2

* add type section to specification

* shape inference improvement

* multiply expected 1st dim in tests by post_nms_topn

* add checks and testcases for dynamic ranks

* indentation, review comments

* reduce code redundancy in ref implementation

* remove comment

* Fix typo in proposal1 spec

* Fix typo in proposal4 spec

(cherry picked from commit 210ad81)
YangleiZouIntel pushed a commit to plaidml/openvino that referenced this pull request May 25, 2021
@LiyangLingIntel
Update Proposal Op (#154)
13b0a23 
* Proposal reference implementation (openvinotoolkit#3924)

* Reference implementation for Proposal, enable CPU SLT

* code style fix

* add type prop test for invalid anchor count

* add unit test

* fix shapes in attribute test

* temp workaround- disable maring end of boxes list

* Disable CPU smoke test- spec misalignment

* code style fixes

* add some details to the specification

* disable myriadx proposal slt

* review changes, using usigned int and size_t

* improve proposal op shape inference to cover dynamic too, add unit test coverage

* remove unused variable in test body

* remove batch size in tests where its not used

* add post nms topn initialization in tests where it was missing

* review comments

* style fix

* style fix 2

* add tests, remove unused variables, change shape inference checks

* style fix

* add input tensors type checks and test coverage

* align input type in attribute and ngraphreader tests to match specification

* fix wrong dimension in error message

* proposalv4 ref impl

* enable single layer and unit tests for proposalv4 ref impl

* align output termination with cpu, enable cpu slt

* custom slt compares to detect less-than-predicted number of boxes

* custom slt compares to detect less-than-predicted number of boxes

* Clarify output termination in spec

* review comments

* smaller input data for unit tests

* add check for batch_dim being static

* disable gpu slt for proposal

* test data style fix

* test data style fix 2

* add type section to specification

* shape inference improvement

* multiply expected 1st dim in tests by post_nms_topn

* add checks and testcases for dynamic ranks

* indentation, review comments

* reduce code redundancy in ref implementation

* remove comment

* Fix typo in proposal1 spec

* Fix typo in proposal4 spec

(cherry picked from commit 210ad81)

* Replace nms partial sort to stable sort

Co-authored-by: Bartosz Lesniewski <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lazarevevgeny lazarevevgeny lazarevevgeny approved these changes

@ilyachur ilyachur ilyachur approved these changes

@ggalieroc-zz ggalieroc-zz Awaiting requested review from ggalieroc-zz

@pelszkow pelszkow Awaiting requested review from pelszkow

@iefode iefode Awaiting requested review from iefode

Assignees

@blesniewski blesniewski

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@blesniewski @lazarevevgeny @ilyachur @pelszkow @ggalieroc-zz