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 atan op #6288

Merged
merged 15 commits into from
Jul 6, 2021
Merged

revise atan op #6288

merged 15 commits into from
Jul 6, 2021

Conversation

songbell
Copy link
Contributor

Details:

  • Add detailed description to Atan Spec
  • Change to use RTTI decl/define for Atan OP
  • Add type prop , visitor (attributes) tests for Atan OP.

Tickets:

  • 37421

@songbell songbell requested a review from a team as a code owner June 22, 2021 06:36
@songbell songbell requested a review from a team June 22, 2021 06:36
@openvino-pushbot openvino-pushbot added category: docs OpenVINO documentation category: Core OpenVINO Core (aka ngraph) labels Jun 22, 2021
@songbell songbell force-pushed the bell/revise_op_atan branch from b8f85db to 3de2e95 Compare June 22, 2021 06:52
ilyachur
ilyachur reviewed Jun 22, 2021
View reviewed changes
ngraph/test/visitors/op/atan.cpp Outdated

TEST(attributes, atan_op)
{
NodeBuilder::get_ops().register_factory<op::Atan>();
Copy link
Contributor

Choose a reason for hiding this comment

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

ould you please rebase your PR to the latest master. And please use parametrized visitor API test which was introduced in this PR: #6181

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed, changed to use parametrized visitor API test

@songbell songbell force-pushed the bell/revise_op_atan branch from 3de2e95 to 26e2488 Compare June 22, 2021 08:04
@ilyachur
Copy link
Contributor

Please resolve merge conflicts.

ilyachur
ilyachur reviewed Jun 24, 2021
View reviewed changes
ngraph/test/visitors/op/atan.cpp Outdated

using Types = ::testing::Types<UnaryOperatorType<ngraph::op::v0::Atan, element::f32>>;

INSTANTIATE_TYPED_TEST_CASE_P(visitor_without_attribute,
Copy link
Contributor

Choose a reason for hiding this comment

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

Please rebase your PR to the latest master and use INSTANTIATE_TYPED_TEST_SUITE_P macro

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done rebase, use new macro for the test

ggalieroc-zz
ggalieroc-zz suggested changes Jun 29, 2021
View reviewed changes
ngraph/test/op_eval/atan.cpp Outdated Show resolved Hide resolved
ggalieroc-zz
ggalieroc-zz reviewed Jun 29, 2021
View reviewed changes
Copy link

@ggalieroc-zz ggalieroc-zz left a comment

Choose a reason for hiding this comment

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

Please correct Inputs/Output sections as follows:

**Inputs**
* **1**: A tensor of type *T* and arbitrary shape. **Required.**
**Outputs**
* **1**: The result of element-wise *Atan* operation applied to the input tensor. A tensor of type *T* and same shape as the input tensor.

Summarizing:

  • Types have to be surrounded by *
  • Always specify type and shape of inputs/outputs

ilyachur
ilyachur reviewed Jun 29, 2021
View reviewed changes
ngraph/test/op_eval/atan.cpp Outdated
using namespace std;
using namespace ngraph;

TEST(op_eval, atan)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add SLT for Atan operation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

per #6361, drop the op_eval test, been covered in backend test

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean, could you verify that we have SLT for this operation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, the single layer test is in inference-engine/tests/functional/inference_engine/serialization/single_layer/activation.cpp

@ggalieroc-zz
Copy link

Additionally, this operation as spec indicates, should have reference implementation specific for integer types. Please check #6429 for implementation details

songbell and others added 3 commits July 1, 2021 22:35
@songbell @ggalieroc
Update docs/ops/arithmetic/Atan_1.md
e0c2896 
Co-authored-by: Gabriele Galiero Casay <[email protected]>
@songbell @ggalieroc
Update docs/ops/arithmetic/Atan_1.md
aacd1c3 
Co-authored-by: Gabriele Galiero Casay <[email protected]>
@songbell @ggalieroc
Update ngraph/core/src/op/atan.cpp
57c694b 
Co-authored-by: Gabriele Galiero Casay <[email protected]>
@songbell songbell requested a review from ggalieroc-zz July 1, 2021 15:00
@songbell songbell force-pushed the bell/revise_op_atan branch from f65e77c to 3f1a39f Compare July 1, 2021 15:06
ilyachur
ilyachur reviewed Jul 5, 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.

Looks like ngraph/test/type_prop/atan.cpp is empty file

ggalieroc-zz
ggalieroc-zz reviewed Jul 5, 2021
View reviewed changes
docs/ops/arithmetic/Atan_1.md Outdated

**Inputs**

* **1**: An tensor of type *T*. **Required.**
* **1**: An tensor of type *T* and arbitrary shape. **Required.**
Copy link

Choose a reason for hiding this comment

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

typo: An tensor of ---> A tensor of ...

@songbell songbell requested a review from a team July 5, 2021 09:18
@songbell songbell force-pushed the bell/revise_op_atan branch 3 times, most recently from f3514e6 to 0faa531 Compare July 6, 2021 02:03
@ilyachur ilyachur merged commit bee21b5 into openvinotoolkit:master Jul 6, 2021
@tiger100256-hu tiger100256-hu mentioned this pull request Jul 12, 2021
Merged
andrew-k-park pushed a commit to andrew-k-park/openvino that referenced this pull request Jul 14, 2021
@songbell @ggalieroc
revise atan op (openvinotoolkit#6288)
964c62f 
* revise atan op

* use parametrized vistor API

* use new gtest macros

* Update docs/ops/arithmetic/Atan_1.md

Co-authored-by: Gabriele Galiero Casay <[email protected]>

* Update docs/ops/arithmetic/Atan_1.md

Co-authored-by: Gabriele Galiero Casay <[email protected]>

* Update ngraph/core/src/op/atan.cpp

Co-authored-by: Gabriele Galiero Casay <[email protected]>

* update doc to follow the rules

* create type_prop for atan

* drop op_eval to be covered in backend

* add the missing type prop case

* add integer type ref impl

* fix clang issue

Co-authored-by: Gabriele Galiero Casay <[email protected]>
rnugmanx pushed a commit to rnugmanx/openvino that referenced this pull request Aug 26, 2021
@songbell @ggalieroc
revise atan op (openvinotoolkit#6288)
5629eaf 
* revise atan op

* use parametrized vistor API

* use new gtest macros

* Update docs/ops/arithmetic/Atan_1.md

Co-authored-by: Gabriele Galiero Casay <[email protected]>

* Update docs/ops/arithmetic/Atan_1.md

Co-authored-by: Gabriele Galiero Casay <[email protected]>

* Update ngraph/core/src/op/atan.cpp

Co-authored-by: Gabriele Galiero Casay <[email protected]>

* update doc to follow the rules

* create type_prop for atan

* drop op_eval to be covered in backend

* add the missing type prop case

* add integer type ref impl

* fix clang issue

Co-authored-by: Gabriele Galiero Casay <[email protected]>
andrei-cv pushed a commit to andrei-cv/openvino that referenced this pull request Aug 30, 2021
@songbell @ggalieroc @andrei-cv
revise atan op (openvinotoolkit#6288)
3b6c33a 
* revise atan op

* use parametrized vistor API

* use new gtest macros

* Update docs/ops/arithmetic/Atan_1.md

Co-authored-by: Gabriele Galiero Casay <[email protected]>

* Update docs/ops/arithmetic/Atan_1.md

Co-authored-by: Gabriele Galiero Casay <[email protected]>

* Update ngraph/core/src/op/atan.cpp

Co-authored-by: Gabriele Galiero Casay <[email protected]>

* update doc to follow the rules

* create type_prop for atan

* drop op_eval to be covered in backend

* add the missing type prop case

* add integer type ref impl

* fix clang issue

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

@ggalieroc-zz ggalieroc-zz ggalieroc-zz approved these changes

@ilyachur ilyachur ilyachur approved these changes

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

Successfully merging this pull request may close these issues.
4 participants
@songbell @ilyachur @ggalieroc-zz @openvino-pushbot