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

[TF FE] Support Case operation #28027

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

shubdas9902
Copy link

@shubdas9902 shubdas9902 commented Dec 12, 2024

Details:

  • Added support for the Case operation from TensorFlow in the OpenVINO framework.
  • Created a new header file case.hpp to define the translation function for the Case operation.
  • Implemented the translate_case_op function in case.cpp to handle the conversion of the TensorFlow Case operation to OpenVINO.
  • Registered the Case operation using the TF_OP_CONVERTER macro in the operation converter framework.

Tickets:

@shubdas9902 shubdas9902 requested a review from a team as a code owner December 12, 2024 03:49
@github-actions github-actions bot added the category: TF FE OpenVINO TensorFlow FrontEnd label Dec 12, 2024
@sys-openvino-ci sys-openvino-ci added the ExternalPR External contributor label Dec 12, 2024
@ilya-lavrenov ilya-lavrenov added the pr: needs tests PR needs tests updating label Dec 12, 2024
@ilya-lavrenov ilya-lavrenov modified the milestone: 2025.0 Dec 12, 2024
@@ -140,6 +142,7 @@ const std::map<std::string, CreatorFunction> get_supported_ops() {
{"Asinh", CreatorFunction(translate_unary_op<v3::Asinh>)},
{"Atan", CreatorFunction(translate_unary_op<v0::Atan>)},
{"Atanh", CreatorFunction(translate_unary_op<v3::Atanh>)},
{"Case", CreatorFunction(translate_case_op)},
Copy link
Contributor

Choose a reason for hiding this comment

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

please add layer tests for this Operation

Copy link
Author

Choose a reason for hiding this comment

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

@rkazants It would be greatly appreciated if you could guide me on the appropriate folder where I should add the layer tests for this operation. As I am new to open-source contributions, your guidance would be very helpful in ensuring I follow the correct structure and conventions for adding the test case for this operation.

@rkazants rkazants changed the title Feature/issue 20534 add case op support tf fe [TF FE] Support Case operation Dec 12, 2024
@shubdas9902
Copy link
Author

shubdas9902 commented Dec 16, 2024

@rkazants I added the layer tests what should I do next?

@p-wysocki
Copy link
Contributor

bump @rkazants

Comment on lines +1 to +18
#ifndef CASE_HPP
#define CASE_HPP

#include "openvino/frontend/tensorflow/node_context.hpp"

namespace ov {
namespace frontend {
namespace tensorflow {
namespace op {

OutputVector translate_case_op(const ov::frontend::tensorflow::NodeContext& node);

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

#endif // CASE_HPP
Copy link
Contributor

Choose a reason for hiding this comment

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

not needed header file. Please check other operation translators for which we don't have header

@@ -0,0 +1,72 @@
#include "openvino/frontend/tensorflow/node_context.hpp"
Copy link
Contributor

Choose a reason for hiding this comment

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

please add copyright as for other src files.

@@ -0,0 +1,72 @@
#include "openvino/frontend/tensorflow/node_context.hpp"
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment on lines +14 to +17
// Validate the operation type
auto op_type = node.get_op_type();
TENSORFLOW_OP_VALIDATION(node, op_type == "Case",
"Internal error: incorrect usage of translate_case_op.");
Copy link
Contributor

Choose a reason for hiding this comment

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

please use default_op_checks instead

"[TensorFlow Frontend] Case operation must have at least one branch.");

// The first input is the condition for selecting the branch
auto cond = node.get_input(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

let us rename it to branch_index

Comment on lines +45 to +52

if (current_model) {
if_op->set_else_body(current_model);
} else {
// Default empty else body
auto placeholder_model = std::make_shared<Model>(OutputVector{}, ParameterVector{});
if_op->set_else_body(placeholder_model);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see that you properly set input parameters for then and else bodies. please check implementation of If translators. The same question for outputs of bodies.

Also, you need to have different conditions for each nested If. It is like a sort of branch_index == i where i is an index of branch.


def branch_fn_2():
return tf.multiply(input_data, tf.constant(2.0, dtype=tf.float32))

Copy link
Contributor

Choose a reason for hiding this comment

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

let us have 4 branches

test_data_basic = [
dict(input_shape=[1, 2], cond=True),
dict(input_shape=[3, 3], cond=False),
]
Copy link
Contributor

Choose a reason for hiding this comment

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

let us check for four branch index values "0, 1, 2, 3"

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 ExternalPR External contributor pr: needs tests PR needs tests updating
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Good First Issue][TF FE]: Support Case operation for TensorFlow models
5 participants