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

Revise equal #6605

Merged
merged 48 commits into from
Aug 11, 2021
Merged

Revise equal #6605

merged 48 commits into from
Aug 11, 2021

Conversation

bszmelcz
Copy link
Contributor

@bszmelcz bszmelcz commented Jul 12, 2021 •
edited
Loading

Details:

  • update spec
  • add SSLTs for all comparison ops
  • add template test for numeric
  • template test for equal added as a part of PR

Tickets:

  • 37436

@bszmelcz bszmelcz requested a review from jdanieck July 13, 2021 20:18
@bszmelcz bszmelcz marked this pull request as ready for review July 14, 2021 09:04
@bszmelcz bszmelcz requested a review from a team as a code owner July 14, 2021 09:04
@bszmelcz bszmelcz requested review from a team, sdurawa and ilyachur July 14, 2021 09:04
@jdanieck jdanieck removed their request for review July 14, 2021 13:41
@ilyachur ilyachur requested a review from rkazants July 15, 2021 06:49
@ilyachur ilyachur added the category: Core OpenVINO Core (aka ngraph) label Jul 15, 2021
@ilyachur ilyachur added this to the 2022.1 milestone Jul 15, 2021
@ilyachur
Copy link
Contributor

@rkazants Please have a look.

@bszmelcz bszmelcz mentioned this pull request Jul 26, 2021
Merged
ilyachur
ilyachur reviewed Jul 27, 2021
View reviewed changes
inference-engine/tests/functional/inference_engine/serialization/single_layer/comparison.cpp Outdated
Serialize();
}

std::map<std::vector<size_t>, std::vector<std::vector<size_t>>> inputShapes = {
Copy link
Contributor

Choose a reason for hiding this comment

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

I propose to have a separate file for each operation. We can use parametrized test, but initialization of parametrized test should be in the separate file.

Copy link
Contributor Author

@bszmelcz bszmelcz Jul 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@ilyachur should I add this change in this PR, or create a new PR solely for it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As this PR is already pretty messy, I would suggest to create a separate PR for this task, but it is up to you

Copy link
Contributor

Choose a reason for hiding this comment

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

I propose to do it in this PR in order to don't return to revising.

@bszmelcz bszmelcz requested a review from ilyachur August 3, 2021 13:23
@ggalieroc-zz
Copy link

Please migrate also these tests to template plugin reference tests:
https://github.com/openvinotoolkit/openvino/blob/master/ngraph/test/backend/numeric.in.cpp

@bszmelcz bszmelcz requested a review from ggalieroc-zz August 7, 2021 22:17
ilyachur
ilyachur reviewed Aug 9, 2021
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.

Why do you add several serialize tests butreference tests only for equal?

@mitruska
Copy link
Contributor

mitruska commented Aug 9, 2021

Why do you add several serialize tests but reference tests only for equal?

I can see equal reference tests have been already added with PR #6728
In this PR additionally "numeric" tests (with Equal op usage) are migrated as @ggalieroc asked in the comment above.
Maybe we should move them to equal.cpp reference tests file as well, to avoid such doubts.

@bszmelcz
Copy link
Contributor Author

bszmelcz commented Aug 9, 2021

Why do you add several serialize tests butreference tests only for equal?

As @mitruska clarified I was asked to migrate numeric.in.cpp backend test to reference test. I am not sure about merging them into one file, since previously they were split and in fact they have different test purpose. Moreover numeric only uses floats as input type. @ilyachur what is your opinion on that?

@ilyachur
Copy link
Contributor

ilyachur commented Aug 9, 2021

Why do you add several serialize tests butreference tests only for equal?

As @mitruska clarified I was asked to migrate numeric.in.cpp backend test to reference test. I am not sure about merging them into one file, since previously they were split and in fact they have different test purpose. Moreover numeric only uses floats as input type. @ilyachur what is your opinion on that?

I agree with @mitruska and think that we can merge these tests.

@ilyachur ilyachur merged commit 289df8d into openvinotoolkit:master Aug 11, 2021
mryzhov pushed a commit to mryzhov/openvino that referenced this pull request Aug 12, 2021
@bszmelcz @mryzhov
Revise equal (openvinotoolkit#6605)
6bfbe58 
* update spec, init backend file for equal op

* add backend, visitors, serialize SLT tests

* add backend test to manifest cause of mismatch of output type with cpu plugin

* add equal to list of trusted ops and to cmakelist file

* refactor backend tests to the new template

* refactor spec

* remove external link in numpy broadcast and update example

* remove comparison.in.cpp file and related tests from manifest

* fix example

* remove redundant arguments

* refactor backend tests

* add pdpd broadcast to the spec, and different precison to SLT test

* add precisions to SLT cpu

* remove unsupported type from SLT

* revert the deletion of comparison.in.cpp file

* remove visitors test, since it will be added in the other PR

* remove equal from CMakeLists.txt

* refactor links in the spec

* revert unwanted changes

* remove equal from unit test manifest

* revert links modification in spec

* add namespace

* split SSLTs for comaprison ops into seperate files

* fix SSLTs names

* add missing new lines

* udpate output type in spec

* rafactor numeric backend test to template

* merge numeric template tests into equal
jiwaszki pushed a commit to jiwaszki/openvino that referenced this pull request Aug 18, 2021
@bszmelcz
Revise equal (openvinotoolkit#6605)
ebacd26 
* update spec, init backend file for equal op

* add backend, visitors, serialize SLT tests

* add backend test to manifest cause of mismatch of output type with cpu plugin

* add equal to list of trusted ops and to cmakelist file

* refactor backend tests to the new template

* refactor spec

* remove external link in numpy broadcast and update example

* remove comparison.in.cpp file and related tests from manifest

* fix example

* remove redundant arguments

* refactor backend tests

* add pdpd broadcast to the spec, and different precison to SLT test

* add precisions to SLT cpu

* remove unsupported type from SLT

* revert the deletion of comparison.in.cpp file

* remove visitors test, since it will be added in the other PR

* remove equal from CMakeLists.txt

* refactor links in the spec

* revert unwanted changes

* remove equal from unit test manifest

* revert links modification in spec

* add namespace

* split SSLTs for comaprison ops into seperate files

* fix SSLTs names

* add missing new lines

* udpate output type in spec

* rafactor numeric backend test to template

* merge numeric template tests into equal
rnugmanx pushed a commit to rnugmanx/openvino that referenced this pull request Aug 26, 2021
@bszmelcz
Revise equal (openvinotoolkit#6605)
90e5a03 
* update spec, init backend file for equal op

* add backend, visitors, serialize SLT tests

* add backend test to manifest cause of mismatch of output type with cpu plugin

* add equal to list of trusted ops and to cmakelist file

* refactor backend tests to the new template

* refactor spec

* remove external link in numpy broadcast and update example

* remove comparison.in.cpp file and related tests from manifest

* fix example

* remove redundant arguments

* refactor backend tests

* add pdpd broadcast to the spec, and different precison to SLT test

* add precisions to SLT cpu

* remove unsupported type from SLT

* revert the deletion of comparison.in.cpp file

* remove visitors test, since it will be added in the other PR

* remove equal from CMakeLists.txt

* refactor links in the spec

* revert unwanted changes

* remove equal from unit test manifest

* revert links modification in spec

* add namespace

* split SSLTs for comaprison ops into seperate files

* fix SSLTs names

* add missing new lines

* udpate output type in spec

* rafactor numeric backend test to template

* merge numeric template tests into equal
andrei-cv pushed a commit to andrei-cv/openvino that referenced this pull request Aug 30, 2021
@bszmelcz @andrei-cv
Revise equal (openvinotoolkit#6605)
b7c6d46 
* update spec, init backend file for equal op

* add backend, visitors, serialize SLT tests

* add backend test to manifest cause of mismatch of output type with cpu plugin

* add equal to list of trusted ops and to cmakelist file

* refactor backend tests to the new template

* refactor spec

* remove external link in numpy broadcast and update example

* remove comparison.in.cpp file and related tests from manifest

* fix example

* remove redundant arguments

* refactor backend tests

* add pdpd broadcast to the spec, and different precison to SLT test

* add precisions to SLT cpu

* remove unsupported type from SLT

* revert the deletion of comparison.in.cpp file

* remove visitors test, since it will be added in the other PR

* remove equal from CMakeLists.txt

* refactor links in the spec

* revert unwanted changes

* remove equal from unit test manifest

* revert links modification in spec

* add namespace

* split SSLTs for comaprison ops into seperate files

* fix SSLTs names

* add missing new lines

* udpate output type in spec

* rafactor numeric backend test to template

* merge numeric template tests into equal
akuporos pushed a commit to akuporos/openvino that referenced this pull request Sep 29, 2021
@bszmelcz @akuporos
Revise equal (openvinotoolkit#6605)
ab703b1 
* update spec, init backend file for equal op

* add backend, visitors, serialize SLT tests

* add backend test to manifest cause of mismatch of output type with cpu plugin

* add equal to list of trusted ops and to cmakelist file

* refactor backend tests to the new template

* refactor spec

* remove external link in numpy broadcast and update example

* remove comparison.in.cpp file and related tests from manifest

* fix example

* remove redundant arguments

* refactor backend tests

* add pdpd broadcast to the spec, and different precison to SLT test

* add precisions to SLT cpu

* remove unsupported type from SLT

* revert the deletion of comparison.in.cpp file

* remove visitors test, since it will be added in the other PR

* remove equal from CMakeLists.txt

* refactor links in the spec

* revert unwanted changes

* remove equal from unit test manifest

* revert links modification in spec

* add namespace

* split SSLTs for comaprison ops into seperate files

* fix SSLTs names

* add missing new lines

* udpate output type in spec

* rafactor numeric backend test to template

* merge numeric template tests into equal
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lazarevevgeny lazarevevgeny lazarevevgeny left review comments

@blesniewski blesniewski blesniewski left review comments

@mitruska mitruska mitruska approved these changes

@ilyachur ilyachur ilyachur approved these changes

@rkazants rkazants Awaiting requested review from rkazants

@sdurawa sdurawa Awaiting requested review from sdurawa

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

Assignees
No one assigned
Labels
category: Core OpenVINO Core (aka ngraph)
Projects
None yet
Milestone
2022.1
Development

Successfully merging this pull request may close these issues.
7 participants
@bszmelcz @ilyachur @sdurawa @blesniewski @ggalieroc-zz @mitruska @lazarevevgeny