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 logical and #6731

Merged
merged 28 commits into from
Aug 3, 2021
Merged

Revise logical and #6731

merged 28 commits into from
Aug 3, 2021

Conversation

pszmel
Copy link
Contributor

@pszmel pszmel commented Jul 21, 2021

Details:

  • update docs
  • create visitor test
  • update backend test
  • create type_prop tests
  • add op to constants.py

Tickets:

  • 37448

@pszmel pszmel marked this pull request as ready for review July 23, 2021 07:27
@pszmel pszmel requested a review from a team as a code owner July 23, 2021 07:27
@pszmel pszmel requested review from a team, ilyachur and lazarevevgeny and removed request for a team July 23, 2021 07:27
@ilyachur ilyachur requested review from sdurawa and pelszkow July 26, 2021 04:11
@ilyachur ilyachur added this to the 2022.1 milestone Jul 26, 2021
@ilyachur ilyachur added the category: Core OpenVINO Core (aka ngraph) label Jul 26, 2021
@pelszkow pelszkow requested a review from ilyachur July 26, 2021 07:07
@pelszkow
Copy link
Contributor

@pszmel, update with master, please.

@pszmel
Copy link
Contributor Author

pszmel commented Jul 26, 2021

@ilyachur, @pelszkow I decided to remove the template from op_reference, because logicalAnd supports only boolean for input and output.

@pszmel pszmel requested review from ilyachur, pelszkow and sdurawa July 27, 2021 17:47
@pelszkow pelszkow removed their request for review July 28, 2021 04:36
mitruska
mitruska reviewed Jul 29, 2021
View reviewed changes
ngraph/core/src/op/and.cpp
Comment on lines -64 to -69
NGRAPH_TYPE_CASE(evaluate_logand, i32, arg0, arg1, out, broadcast_spec);
NGRAPH_TYPE_CASE(evaluate_logand, i64, arg0, arg1, out, broadcast_spec);
NGRAPH_TYPE_CASE(evaluate_logand, u32, arg0, arg1, out, broadcast_spec);
NGRAPH_TYPE_CASE(evaluate_logand, u64, arg0, arg1, out, broadcast_spec);
NGRAPH_TYPE_CASE(evaluate_logand, f16, arg0, arg1, out, broadcast_spec);
NGRAPH_TYPE_CASE(evaluate_logand, f32, arg0, arg1, out, broadcast_spec);
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't you update has_evaluate() types too?

openvino/ngraph/core/src/op/and.cpp

Lines 80 to 95 in 37ff06e

bool op::v1::LogicalAnd::has_evaluate() const
{
NGRAPH_OP_SCOPE(v1_LogicalAnd_has_evaluate);
switch (get_input_element_type(0))
{
case ngraph::element::boolean:
case ngraph::element::i32:
case ngraph::element::i64:
case ngraph::element::u32:
case ngraph::element::u64:
case ngraph::element::f16:
case ngraph::element::f32: return true;
default: break;
}
return false;
}

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. Removed

@pszmel pszmel requested review from mitruska and dkozykowski August 2, 2021 09:29
dkozykowski
dkozykowski approved these changes Aug 2, 2021
View reviewed changes
Copy link
Contributor

@dkozykowski dkozykowski left a comment

Choose a reason for hiding this comment

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

LGTM

@ilyachur ilyachur enabled auto-merge (squash) August 3, 2021 06:22
@ilyachur ilyachur merged commit a30bd0c into openvinotoolkit:master Aug 3, 2021
rnugmanx pushed a commit to rnugmanx/openvino that referenced this pull request Aug 26, 2021
@pszmel
Revise logical and (openvinotoolkit#6731)
79eea07 
* update docs

* add host tensors validation

* create type_prop tests

* create serialization single layer test

* create visitor test

* create op_reference test

* add logicalAnd to constants.py

* create additional op_reference tests

* add check for number of visited attributes in visitor test

* update auto_broadcast description

* remoove backend test

* update LogicalNot params name

* remove backend test from CMakeList

* create util function for type_prop tests

* update op_reference tests

* remove typo in docs

* remove unsupported types from evaluate

* fix bug in op_reference test

* refactor visitor test

* update math formula in the spec

* update has_evaluate types
andrei-cv pushed a commit to andrei-cv/openvino that referenced this pull request Aug 30, 2021
@pszmel @andrei-cv
Revise logical and (openvinotoolkit#6731)
3524d02 
* update docs

* add host tensors validation

* create type_prop tests

* create serialization single layer test

* create visitor test

* create op_reference test

* add logicalAnd to constants.py

* create additional op_reference tests

* add check for number of visited attributes in visitor test

* update auto_broadcast description

* remoove backend test

* update LogicalNot params name

* remove backend test from CMakeList

* create util function for type_prop tests

* update op_reference tests

* remove typo in docs

* remove unsupported types from evaluate

* fix bug in op_reference test

* refactor visitor test

* update math formula in the spec

* update has_evaluate types
akuporos pushed a commit to akuporos/openvino that referenced this pull request Sep 29, 2021
@pszmel @akuporos
Revise logical and (openvinotoolkit#6731)
b3890af 
* update docs

* add host tensors validation

* create type_prop tests

* create serialization single layer test

* create visitor test

* create op_reference test

* add logicalAnd to constants.py

* create additional op_reference tests

* add check for number of visited attributes in visitor test

* update auto_broadcast description

* remoove backend test

* update LogicalNot params name

* remove backend test from CMakeList

* create util function for type_prop tests

* update op_reference tests

* remove typo in docs

* remove unsupported types from evaluate

* fix bug in op_reference test

* refactor visitor test

* update math formula in the spec

* update has_evaluate types
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pelszkow pelszkow pelszkow left review comments

@dkozykowski dkozykowski dkozykowski approved these changes

@mitruska mitruska mitruska approved these changes

@ilyachur ilyachur ilyachur approved these changes

@lazarevevgeny lazarevevgeny Awaiting requested review from lazarevevgeny

@sdurawa sdurawa Awaiting requested review from sdurawa

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.
6 participants
@pszmel @pelszkow @ilyachur @mitruska @dkozykowski @sdurawa