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

【PaddlePaddle Hackathon 4】add paddle one_hot_v2 op #15859

Merged
merged 21 commits into from
May 22, 2023
Merged

【PaddlePaddle Hackathon 4】add paddle one_hot_v2 op #15859

merged 21 commits into from
May 22, 2023

Conversation

Patrick-Star125
Copy link
Contributor

@Patrick-Star125 Patrick-Star125 commented Feb 21, 2023

Details:

add one_hot_v2 operation in Paddle front end

Reference:

unit-test

image

@Patrick-Star125 Patrick-Star125 requested a review from a team as a code owner February 21, 2023 16:22
@github-actions github-actions bot added the category: PDPD FE OpenVINO PaddlePaddle FrontEnd label Feb 21, 2023
@Patrick-Star125
Copy link
Contributor Author

The inputs received by PEPE frontend API are relatively simplified comparing to the OV op OneHot. paddle op don't support attributes on_value, off_value and axis.

@Patrick-Star125
Copy link
Contributor Author

@OpenVINO-dev-contest seems like parameter depth_tensor had been deprecated, should I keep PADDLE_OP_CHECK or just ignore this input.
image
image

@openvino-dev-samples
Copy link
Contributor

@OpenVINO-dev-contest seems like parameter depth_tensor had been deprecated, should I keep PADDLE_OP_CHECK or just ignore this input. image image

Could confirm it with Paddle‘s team, I still see this parameter in its official documents
https://github.com/PaddlePaddle/Paddle/blob/release/2.4/python/paddle/nn/functional/input.py#L109

@Patrick-Star125
Copy link
Contributor Author

I leaved a issue about this question, I will comment on this pr as soon as paddle team reply.

@Patrick-Star125
Copy link
Contributor Author

@OpenVINO-dev-contest I understand that only when the shape of num_class is 0-D, it is valid as input. I have added a corresponding test sample.

auto-merge was automatically disabled March 4, 2023 05:46

Head branch was pushed to by a user without write access

@ilya-lavrenov ilya-lavrenov added ExternalPR External contributor PaddlePaddle Hackathon a Intel and Baidu joint Hackathon event labels Mar 4, 2023
auto depth_value = node.get_input("depth_tensor");
depth = std::make_shared<default_opset::Squeeze>(depth_value);
} else {
auto depth_value = node.get_attribute<int>("depth");
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi can we leave it as const, and also set a default value for the attributes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

1.const had been added
2.input without num_class is not permitted in paddle, so I think it is presumably not needful to set a default value. If it is necessary, the default number of classes could be set as one greater than the largest class value in the input tensor, according to the pytorch implement.

import sys


def one_hot_v2_1(name: str, x, num_classes):
Copy link
Contributor

Choose a reason for hiding this comment

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

Since there are some duplicated code, can we combine these 2 functions together? You can refer to top_k_v2

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

@openvino-dev-samples
Copy link
Contributor

Details:

add one_hot_v2 operation in Paddle front end

Reference:

unit-test

image

Pls update the URL with English version of OP description, thanks

@Patrick-Star125
Copy link
Contributor Author

URL updated

depth = std::make_shared<default_opset::Squeeze>(depth_value);
} else {
const auto depth_value = node.get_attribute<int>("depth", -1);
if (depth_value == -1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why should we consider the depth as -1 or a negative, is it a available value in Paddle‘s model ? if yes, could you help to add a new case in uni-test ?

Copy link
Contributor Author

@Patrick-Star125 Patrick-Star125 Mar 17, 2023

Choose a reason for hiding this comment

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

Like I mentioned before, input without num_class is not permitted in Paddle‘s model and negative value is also not available. So I think it is better not to set a default value for num_class. If it must has a default value, I would recommend -1, same as the pytorch implement. If we don't need a default value, I will remove it latter.

num_classes (int) – Total number of classes. If set to -1, the number of classes will be inferred as one greater than the largest class value in the input tensor.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks. Could you help to confirm if paddle will throw an exception when we set the num_class as -1/negative ? If Paddle will reject this case, i think we can remove the default value in OV's frontend.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not valid when set the num_class as -1/negative
image
I have removed default value as well.

@openvino-comment-bot
Copy link

Can one of the admins verify this patch?

@xczhai
Copy link
Contributor

xczhai commented May 18, 2023

@Patrick-Star125
Copy link
Contributor Author

Patrick-Star125 commented May 18, 2023

So I need to add note at the end of paddle limitation?

one_hot_v2 attribute num_class is non-default.

I'm not sure whether it is a good description.

@ceciliapeng2011 ceciliapeng2011 enabled auto-merge (squash) May 18, 2023 11:25
@xczhai
Copy link
Contributor

xczhai commented May 18, 2023

@Patrick-Star125 How about passing input with N-dims?

auto-merge was automatically disabled May 19, 2023 07:36

Head branch was pushed to by a user without write access

@Patrick-Star125 Patrick-Star125 requested review from a team as code owners May 19, 2023 07:36
@Patrick-Star125 Patrick-Star125 requested review from zKulesza and removed request for a team May 19, 2023 07:36
@github-actions github-actions bot added the category: docs OpenVINO documentation label May 19, 2023
@Patrick-Star125
Copy link
Contributor Author

sure, done

@ceciliapeng2011 ceciliapeng2011 enabled auto-merge (squash) May 22, 2023 02:47
@ceciliapeng2011 ceciliapeng2011 merged commit 0963c23 into openvinotoolkit:master May 22, 2023
kblaszczak-intel added a commit to kblaszczak-intel/openvino that referenced this pull request May 23, 2023
adding one_hot_v2 to PdPd
aligning with openvinotoolkit#15859
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: docs OpenVINO documentation category: PDPD FE OpenVINO PaddlePaddle FrontEnd ExternalPR External contributor PaddlePaddle Hackathon a Intel and Baidu joint Hackathon event
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants