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 linspace op #15835

Merged
merged 16 commits into from
May 19, 2023
Merged

【PaddlePaddle Hackathon 4】add paddle linspace op #15835

merged 16 commits into from
May 19, 2023

Conversation

Patrick-Star125
Copy link
Contributor

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

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

Patrick-Star125 commented Feb 22, 2023

Hi, the source codes of paddle linspace API illustrate that the input scalar(int/float) will be transferred in to tensor.

I wonder if it is needed to provide the supporting of scalar input on linspace.cpp. Same question about unit-test samples also exist.

// generate a range of numbers [0, 1, ..., num)
auto const_zero = std::make_shared<default_opset::Constant>(start.get_element_type(), Shape{}, 0);
auto const_num = std::make_shared<default_opset::Squeeze>(num);
auto range0_n = std::make_shared<default_opset::Range>(const_zero, const_num, const_one, start.get_element_type());
Copy link
Contributor

Choose a reason for hiding this comment

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

I just wonder can we use range for a direct output ? so we do not need Multiply and Add, just a guess

Copy link
Contributor Author

Choose a reason for hiding this comment

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

great idea, it is important to simplify the operator code. but the behavior of Range is subtly different from Linspace even though their input parameters are similar with each other. Using Range for a direct output maybe require the overal re-design of linspace.

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 clarify what's gap between them? can we convert the num in Linspace into step in Range

Copy link
Contributor Author

Choose a reason for hiding this comment

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

step can be calculated by num, which has already implemented in line 30 of linspace.cpp. The key point is the behavior of Range. For instance, same input (start: [0] stop: [1] num: [4]/step: [0.33]), and the result will be:
Range: [0, 0.33, 0.66]
Linspace: [0, 0.33, 0.66, 1]
And the input of Range must be scalar, which means must be taken before range operator.
image
I think it will become more complicate after adjusting the code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Great thanks for your illustration

Copy link
Contributor

Choose a reason for hiding this comment

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

Can start and stop be negative here, and can you put this example ( (start: [0] stop: [1] num: [4]/step: [0.33])) into your test case, thanks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK,test samples had been added

Copy link
Contributor

@ceciliapeng2011 ceciliapeng2011 left a comment

Choose a reason for hiding this comment

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

Would you please rebase and resolve the conflicts?

@Patrick-Star125 Patrick-Star125 requested review from a team as code owners March 4, 2023 05:21
@github-actions github-actions bot added category: GPU OpenVINO GPU plugin and removed category: GPU OpenVINO GPU plugin labels Mar 4, 2023
@Patrick-Star125
Copy link
Contributor Author

conflicts resoved

@ilya-lavrenov ilya-lavrenov added ExternalPR External contributor PaddlePaddle Hackathon a Intel and Baidu joint Hackathon event labels Mar 4, 2023
@OpenVINO-dev-contest
Copy link
Contributor

Details:

add linspace operation in Paddle front end

Reference:

Hello could you also post a English version of Paddle Op description, thanks

namespace paddle {
namespace op {
NamedOutputs linspace(const NodeContext& node) {
auto start = node.get_input("Start");
Copy link
Contributor

Choose a reason for hiding this comment

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

How about the inputs are variables, but not a tensor. e.g int or float
image

@Patrick-Star125
Copy link
Contributor Author

Patrick-Star125 commented Mar 9, 2023

Hi, the source codes of paddle linspace API illustrate that the input scalar(int/float) will be transferred in to tensor.

I wonder if it is needed to provide the supporting of scalar input on linspace.cpp. Same question about unit-test samples also exist.

Hi, @OpenVINO-dev-contest

  1. like I metioned before, all scalars will be transformed into tensor in advance, the source code from paddle can prove that the only attribute input is 'dtype'.
  2. document URL has been changed

@OpenVINO-dev-contest
Copy link
Contributor

Hi, the source codes of paddle linspace API illustrate that the input scalar(int/float) will be transferred in to tensor.

I wonder if it is needed to provide the supporting of scalar input on linspace.cpp. Same question about unit-test samples also exist.

Hi, like I metioned before, all scalars will be transformed into tensor in advance, the source code from paddle can prove that the only attribute input is 'dtype'.

Sorry I missed it. but shall we consider the dtype is not in default?

Hi, the source codes of paddle linspace API illustrate that the input scalar(int/float) will be transferred in to tensor.

I wonder if it is needed to provide the supporting of scalar input on linspace.cpp. Same question about unit-test samples also exist.

Hi, like I metioned before, all scalars will be transformed into tensor in advance, the source code from paddle can prove that the only attribute input is 'dtype'.

Sorry, I missed it, but shall we consider the dtype when it is not in default in some cases. for example input type is fp32 and dtype is int32. I don't know whether it may happen ?

@Patrick-Star125
Copy link
Contributor Author

Sorry, I missed it, but shall we consider the dtype when it is not in default in some cases. for example input type is fp32 and dtype is int32. I don't know whether it may happen ?

The python frontend of paddle could ensure that the input type and attr dtype is same, so I think there is no need for extra examination or adjustment. related code

@OpenVINO-dev-contest
Copy link
Contributor

The python frontend of paddle could ensure that the input type and attr dtype is same, so I think there is no need for extra examination or adjustment. related code

Can you update the uni-test with these case example ? for example input type is fp32 and dtype is int32. just ensure the Paddle front end will do conversion for us.

@Patrick-Star125
Copy link
Contributor Author

Can you update the uni-test with these case example ? for example input type is fp32 and dtype is int32. just ensure the Paddle front end will do conversion for us.

sorry, I misunderstood the function of dtype, adjustment on code and test samples have been conducted now.

auto stop = node.get_input("Stop");
auto num = node.get_input("Num");
std::unordered_map<int, element::Type> umap{{2, element::i32}, {3, element::i64}, {5, element::f32}};
const auto dtype = node.get_attribute<int>("dtype", 5);
Copy link
Contributor

Choose a reason for hiding this comment

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

can we use auto dtype = node.get_attribute<ov::element::Type>("dtype"); to get the the type ?

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Pls add a screenshot of test results, thanks

@openvino-comment-bot
Copy link

Can one of the admins verify this patch?

@ceciliapeng2011 ceciliapeng2011 enabled auto-merge (squash) May 18, 2023 11:22
@ceciliapeng2011 ceciliapeng2011 merged commit 2e54686 into openvinotoolkit:master May 19, 2023
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

None yet

5 participants