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

[Good First Issue] [TF FE]: Support Atan2 operation for TensorFlow models #20754

Closed
rkazants opened this issue Oct 30, 2023 · 18 comments · Fixed by #21076
Closed

[Good First Issue] [TF FE]: Support Atan2 operation for TensorFlow models #20754

rkazants opened this issue Oct 30, 2023 · 18 comments · Fixed by #21076
Assignees
Labels
category: TF FE OpenVINO TensorFlow FrontEnd good first issue Good for newcomers no_stale Do not mark as stale
Milestone

Comments

@rkazants
Copy link
Contributor

rkazants commented Oct 30, 2023

Context

OpenVINO component responsible for support of TensorFlow models is called as TensorFlow Frontend (TF FE). TF FE converts a model represented in TensorFlow opset to a model in OpenVINO opset.

In order to infer TensorFlow models with Atan2 operation by OpenVINO, TF FE needs to be extended with this operation support.

What needs to be done?

For Atan2 operation support, you need to add the corresponding loader into TF FE op directory and to register it into the dictionary of Loaders. One loader is responsible for conversion (or decomposition) of one type of TensorFlow operation.

Here is an example of loader implementation for TensorFlow Einsum operation:

OutputVector translate_einsum_op(const NodeContext& node) { 
     auto op_type = node.get_op_type(); 
     TENSORFLOW_OP_VALIDATION(node, op_type == "Einsum", "Internal error: incorrect usage of translate_einsum_op."); 
     auto equation = node.get_attribute<std::string>("equation"); 
  
     OutputVector inputs; 
     for (size_t input_ind = 0; input_ind < node.get_input_size(); ++input_ind) { 
         inputs.push_back(node.get_input(input_ind)); 
     } 
  
     auto einsum = make_shared<Einsum>(inputs, equation); 
     set_node_name(node.get_name(), einsum); 
     return {einsum}; 
 } 

In this example, translate_einsum_op converts TF Einsum into OV Einsum. NodeContext object passed into the loader packs all info about inputs and attributes of Einsum operation. The loader retrieves an attribute of the equation by using the NodeContext::get_attribute() method, prepares input vector, creates Einsum operation from OV opset and returns a vector of outputs.

Responsibility of a loader is to parse operation attributes, prepare inputs and express TF operation via OV operations sub-graph. Example for Einsum demonstrates the resulted sub-graph with one operation. In PR #19007 you can see operation decomposition into multiple node sub-graph.

Hint

The output range values for Atan2 is [-pi; +pi] different from Atan. You need to pay attention to signs of operands x and y. For more information, read the details in wiki.

Example Pull Requests

Resources

Contact points

@openvinotoolkit/openvino-tf-frontend-maintainers

Ticket

TBD

@rkazants rkazants added good first issue Good for newcomers no_stale Do not mark as stale labels Oct 30, 2023
@rkazants rkazants moved this to Contributors Needed in Good first issues Oct 30, 2023
@rkazants rkazants added the category: TF FE OpenVINO TensorFlow FrontEnd label Oct 30, 2023
@rkazants rkazants moved this to Contributors Needed in OpenVINO General Board Oct 30, 2023
@rghvsh
Copy link
Contributor

rghvsh commented Oct 30, 2023

Hey! @rkazants I'd like to work on this project can i get started?

@ilya-lavrenov ilya-lavrenov moved this from Contributors Needed to Assigned in Good first issues Oct 30, 2023
@rkazants
Copy link
Contributor Author

rkazants commented Oct 30, 2023

Hey! @rkazants I'd like to work on this project can i get started?

Sure, very welcome to contribute. The task is yours.

Best regards,
Roman

@rkazants rkazants moved this from Contributors Needed to Assigned in OpenVINO General Board Oct 30, 2023
@rkazants
Copy link
Contributor Author

rkazants commented Nov 1, 2023

Hi @rghvsh, is there progress with this task? Please let me know if you need some help.

Thanks,
Roman

@rghvsh
Copy link
Contributor

rghvsh commented Nov 1, 2023

Hey! @rkazants I've started working on the issue and have made some progress.

Just had a small question. Wanted to make sure that in src/frontends/tensorflow/src/op_table.cpp Atan2 needs to be added under binary operations with opset 8.

Thank You,
-- @rghvsh

@rkazants
Copy link
Contributor Author

rkazants commented Nov 1, 2023

tends/tensorflow/src/op_table.cpp Atan2 ne

Hi @rghvsh, please pay attention that it is Not sufficient to add:

"Atan2", CreatorFunction(translate_binary_op<opset8::Atan>)

That is because opset8::Atan and Atan2 have different inputs number. opset8::Atan is unary operation. You can see description of operations here: https://docs.openvino.ai/2023.1/openvino_docs_ops_opset12.html. Also, TF Atan2 value depends on signs of operands x and y. So you need to develop a decomposition of Atan2 using available operations from OpenVINO opset. Please use already developed loaders for other operations as examples.

Best regards,
Roman

@rghvsh
Copy link
Contributor

rghvsh commented Nov 1, 2023

Got it!

@rghvsh
Copy link
Contributor

rghvsh commented Nov 2, 2023

Hey! @rkazants Is this code correct for this issue wanted to check with and get your feed back before a PR. If there are any mistakes I'll fix them ASAP.

Thank You
-- @rghvsh

#include "common_op_table.hpp"
#include "src/core/reference/include/openvino/reference/atan2.hpp"

using namespace std;
using namespace ov::opset10;

namespace ov {
namespace frontend {
namespace tensorflow {
namespace op {
OutputVector translate_atan2_op(const NodeContext& node) {

default_op_checks(node, 2, {"atan2"});
auto y = node.get_input(0);
auto x = node.get_input(1);
int input_size = sizeof(x)/sizeof(x[0]);

// Creating variable of the same size to store value
auto pout = create_same_type_const_scalar<float32>(x, 0);

// Value of atan2.
auto atan2 = make_shared<atan2>(y, x, pout, input_size);

set_node_name(node.get_name(), atan2);
return {atan2};

}
} // namespace op
} // namespace tensorflow
} // namespace frontend
} // namespace ov

@rkazants
Copy link
Contributor Author

rkazants commented Nov 2, 2023

Hey! @rkazants Is this code correct for this issue wanted to check with and get your feed back before a PR. If there are any mistakes I'll fix them ASAP.

Thank You -- @rghvsh

#include "common_op_table.hpp" #include "src/core/reference/include/openvino/reference/atan2.hpp"

using namespace std; using namespace ov::opset10;

namespace ov { namespace frontend { namespace tensorflow { namespace op { OutputVector translate_atan2_op(const NodeContext& node) {

default_op_checks(node, 2, {"atan2"});
auto y = node.get_input(0);
auto x = node.get_input(1);
int input_size = sizeof(x)/sizeof(x[0]);

// Creating variable of the same size to store value
auto pout = create_same_type_const_scalar<float32>(x, 0);

// Value of atan2.
auto atan2 = make_shared<atan2>(y, x, pout, input_size);

set_node_name(node.get_name(), atan2);
return {atan2};

} } // namespace op } // namespace tensorflow } // namespace frontend } // namespace ov

Hi @rghvsh,

Not quite correct:).

  1. Please start from building OpenVINO because the code is not buildable. There is no atan2 operation in OV, also there is no sizeof method for ov::Outputov::Node. The instructions for build you can find here: https://github.com/openvinotoolkit/openvino/blob/master/docs/dev/build.md.
  2. Build OV sub-graph that will compute Atan2 by the formula from wiki:
    image

Examples to build sub-graph you can find in other loaders. Example: https://github.com/openvinotoolkit/openvino/blob/master/src/frontends/tensorflow_common/src/op/xdivy.cpp.

Best regards,
Roman

@rghvsh
Copy link
Contributor

rghvsh commented Nov 2, 2023

I see I'll start doing this

@rghvsh
Copy link
Contributor

rghvsh commented Nov 3, 2023

Hey! @rkazants Update:

  1. I've built OpenVINO
  2. Started building the subgraph

had a question about the input which will be a NodeClass type object what will be stored in the class can you point me towards the documentation tried finding but got none

Thank You

@rkazants
Copy link
Contributor Author

rkazants commented Nov 3, 2023

Hey! @rkazants Update:

  1. I've built OpenVINO
  2. Started building the subgraph

had a question about the input which will be a NodeClass type object what will be stored in the class can you point me towards the documentation tried finding but got none

Thank You

Hi @rghvsh,

Great, keep this way. I think you mean ov::Node class that is a parent class for different operation classes from the opset. Here is a developer documentation for classes you need: https://docs.openvino.ai/2023.1/groupov_model_cpp_api.html

image

This documentation might be helpful but see already implemented loaders as examples too:)

Best regards,
Roman

@rkazants
Copy link
Contributor Author

rkazants commented Nov 7, 2023

Hi @rghvsh.

Do you have progress working on this task. Please let me know if you need help.

Best regards,
Roman

@rghvsh
Copy link
Contributor

rghvsh commented Nov 7, 2023

Hey @rkazants I've made progress and am working in it.

Please let me know if you need help.

Sure!

Thank You
Raghav

@rghvsh
Copy link
Contributor

rghvsh commented Nov 10, 2023

default_op_checks(node, 2, {"atan2"});
auto y = node.get_input(0);
auto x = node.get_input(1);

Hey @rkazants this stament unpacks the the node object and gets x and y which are of Tensor datatype how to iterate over these(x and y).

@rkazants
Copy link
Contributor Author

default_op_checks(node, 2, {"atan2"}); auto y = node.get_input(0); auto x = node.get_input(1);

Hey @rkazants this stament unpacks the the node object and gets x and y which are of Tensor datatype how to iterate over these(x and y).

Hi @rghvsh,

Not sure how you would like to iterate. You just need to build a graph on down of x and y to express atan2. No need to iterate because you have only two inputs.
Just give you an idea:

auto div_y_x = make_shared<Divide>(y, x);
auto atan = make_shared<Atan>(div_y_x);
auto atan_plus_pi = ...
auto atan_minus_pi = ...
auto zero_const = create_same_type_const_scalar<int32_t>(x, 0);
auto is_x_positive = make_shared<Greater>(x, const_zero);
...

Use Select operation from our opset to represent:

image

Best regards,
Roman

@rghvsh
Copy link
Contributor

rghvsh commented Nov 11, 2023

Hey @rkazants UPDATE :
I've understood how this works and have written the python test just had some question

In the second case we have two conditions how will those be implemented do we have to use somethging like this


 auto result = make_shared<Select>(is_x_negitive  && is_y_positive, atan_plus_pi ,  atan); 

and

is this right way to make a pi constant

auto pi_const = create_same_type_const_scalar<float>(x, pi);

Sorry for asking too many questions its my first time contributing to a large codebase.

Thank You
Raghav

@rkazants
Copy link
Contributor Author

Hi @rghvsh,

Sorry for the delay. Here is an pseudo-code how to represent these conditions using our opset:

// handle the first condition
auto arctan_y_x = ...
auto result = arctan_y_x;

// handle the second condition
auto const_pi = create_same_type_const_scalar<double>(x, std::atan(1.0)*4);
auto x_negative = make_shared<Less>(x, const_zero);
auto y_non_negative = make_shared<GreaterEqual>(y, const_zero);
auto cond1 = make_shared<LogicalAnd>(x_negative, y_non_negative);
auto arctan_y_x_plus_pi = make_shared<Add>(arctan_y_x, const_pi)
auto result = make_shared<Select>(cond1, arctan_y_x_plus_pi, result);

// handle the third consition
auto y_negative = make_shared<Less>(y, const_zero);
auto cond2 = make_shared<LogicalAnd>(x_negative, y_negative);
auto arctan_y_x_minus_pi = make_shared<Add>(arctan_y_x, const_pi)
auto result = make_shared<Select>(cond2, arctan_y_x_minus_pi, result);

//...

For undefined behaviour, we can do nothing. Hope this helps.

Best regards,
Roman

@rghvsh
Copy link
Contributor

rghvsh commented Nov 14, 2023

Hey! @rkazants I just opened a PR #21076 if there are any changes that need to be implemented please let me know.

Thank You
Raghav

@ilya-lavrenov ilya-lavrenov moved this from Assigned to In Review in Good first issues Nov 14, 2023
@ilya-lavrenov ilya-lavrenov moved this from Assigned to Under Review in OpenVINO General Board Nov 14, 2023
@github-project-automation github-project-automation bot moved this from In Review to Closed in Good first issues Dec 6, 2023
@rkazants rkazants moved this from Under Review to Closed in OpenVINO General Board Dec 7, 2023
@ilya-lavrenov ilya-lavrenov added this to the 2023.3 milestone Jan 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: TF FE OpenVINO TensorFlow FrontEnd good first issue Good for newcomers no_stale Do not mark as stale
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants