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

【PaddlePaddle Hackathon 3 No.102】support paddle elementwise_floordiv #13059

Merged
merged 2 commits into from
Oct 20, 2022
Merged

【PaddlePaddle Hackathon 3 No.102】support paddle elementwise_floordiv #13059

merged 2 commits into from
Oct 20, 2022

Conversation

taixiurong
Copy link
Contributor

@taixiurong taixiurong commented Sep 15, 2022 •
edited
Loading

@taixiurong taixiurong requested review from a team as code owners September 15, 2022 04:13
@taixiurong taixiurong mentioned this pull request Sep 15, 2022
Closed
@taixiurong taixiurong changed the title 【Hackathon No.102】support paddle elementwise_floordiv 【PaddlePaddle Hackathon 3 No.102】support paddle elementwise_floordiv Sep 15, 2022
openvino-dev-samples
openvino-dev-samples reviewed Sep 15, 2022
View reviewed changes
Copy link
Contributor

@openvino-dev-samples openvino-dev-samples left a comment

Choose a reason for hiding this comment

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

hi @taixiurong thanks for your contribution.

src/core/tests/frontend/paddle/test_models/gen_scripts/generate_elementwise_ops.py Outdated Show resolved Hide resolved
@openvino-dev-samples
Copy link
Contributor

Hi, @meiyang-intel @ceciliapeng2011 could you also help to review this PR, thanks

@ceciliapeng2011 ceciliapeng2011 added category: PDPD FE OpenVINO PaddlePaddle FrontEnd PaddlePaddle Hackathon a Intel and Baidu joint Hackathon event labels Sep 16, 2022
ceciliapeng2011
ceciliapeng2011 reviewed Sep 16, 2022
View reviewed changes
src/core/tests/frontend/paddle/test_models/gen_scripts/generate_elementwise_ops.py Outdated
with paddle.static.program_guard(paddle.static.Program(), paddle.static.Program()):
node_x = paddle.static.data(name = 'x', shape = x.shape, dtype = in_dtype)
node_y = paddle.static.data(name = 'y', shape = y.shape, dtype = in_dtype)
if paddle_ver == "1.8":
Copy link
Contributor

Choose a reason for hiding this comment

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

We have no intention to support paddle 1.8. So I would suggest to remove corresponding code and test cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

Think twice. Let's settle down this version after we know more about "axis" discrepancy.

Copy link
Contributor Author

@taixiurong taixiurong Sep 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

this is only for test different api in paddle; other elementwise_op only support paddle 1.8; when those elementwise ops support paddle2.x , you can remove corresponding code and test cases .

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please use paddle.version to detect paddle version, instead of using the argument paddle_ver?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if use paddle.version, axis != -1 cannot add in tese cases, you want test axis == -1 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

if use paddle.version, axis != -1 cannot add in tese cases, you want test axis == -1 ?

We have no intention to support paddle 1.8. We don't hope the name of this argument bring any confusion to developers and users.
But in the op mapper, we could keep your implementation to make some legacy paddle models are supported as well.

So can you simply create the test cases without the implication of paddle version?

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably simply use both fluid and paddle 2.1 api to create cases. That's all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

update

src/core/tests/frontend/paddle/test_models/gen_scripts/generate_elementwise_ops.py Show resolved Hide resolved
@openvino-dev-samples
Copy link
Contributor

Please fix clang format error.

It could fixed with 2 options -
Method1:

apt install clang-format-9
enable clang with cmake option: -DENABLE_CLANG_FORMAT=ON
make clang_format_fix_all

Method2:
Select the code your wish to format in VScode and left click it, then 'Format Selection'

openvino-dev-samples
openvino-dev-samples reviewed Sep 23, 2022
View reviewed changes
src/core/tests/frontend/paddle/test_models/gen_scripts/generate_elementwise_ops.py
data_x.astype(dtype), data_y.astype(dtype), axis, dtype)

data_x = np.random.randint(1, 10, [2, 5, 3, 4])
data_y = np.random.randint(1, 5, [3, 4])
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @taixiurong could add the test case with negative value, seems Paddle's implementation is aligned with Torch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @taixiurong could add the test case with negative value, seems Paddle's implementation is aligned with Torch.

  1. yes, i review the paddle code , floor_div to call std::trunc(a/b), https://github.com/PaddlePaddle/Paddle/blob/develop/paddle/phi/kernels/funcs/elementwise_functor.h#L573
    but the paddle give a response：

image

PaddlePaddle/Paddle#46379
2. in this test case , i use a = -2, b = 1, is ok. a = -4, b = 3, z = floor_div(a/b), z = -1。 so want to modiy this logic in openvino?

Copy link
Contributor

Choose a reason for hiding this comment

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

@ceciliapeng2011 @meiyang-intel what's your opinion ?

Copy link
Contributor Author

@taixiurong taixiurong Sep 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@OpenVINO-dev-contest hi paddle fix this bug,
doc: https://github.com/PaddlePaddle/Paddle/pull/46419/files
code: https://github.com/PaddlePaddle/Paddle/pull/45051/files
i fix the code in openvino, tese code like:
image

the tese result:
image

do want to push new code ?

Copy link
Contributor

Choose a reason for hiding this comment

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

of course, you can try it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

update

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you test float to check if OpenVINO and Paddle can have the same result too? I am afraid they won't.

OpenVINO divide op has an attribute "pythondiv", but works with integer only. That's why the integer tests pass.

openvino/src/core/reference/include/ngraph/runtime/reference/divide.hpp

Line 30 in 7bbd428

if (pythondiv) {

openvino/src/core/include/openvino/op/divide.hpp

Line 29 in 7bbd428

bool pythondiv,

Copy link
Contributor

Choose a reason for hiding this comment

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

@jane-intel Do you have comments about the algorithm discrepancy of openvino divide and paddle floor_divide? Thanks.

Copy link
Contributor Author

@taixiurong taixiurong Sep 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

https://github.com/PaddlePaddle/Paddle/blob/develop/paddle/phi/kernels/elementwise_kernel.cc#L144
https://www.paddlepaddle.org.cn/documentation/docs/zh/api/paddle/floor_divide_cn.html#floor-divide
paddle support int and int64, not support float.
the paddle finally to call static_cast(a / b).
https://github.com/PaddlePaddle/Paddle/blob/develop/paddle/phi/kernels/funcs/elementwise_functor.h#L573

in openvino if pythondiv is false , to call out[i] = arg0[i] / arg1[i]

openvino/src/core/reference/include/ngraph/runtime/reference/divide.hpp

Line 48 in 7bbd428

out[i] = arg0[i] / arg1[i];

the logic is same.

@ceciliapeng2011 ceciliapeng2011 self-assigned this Sep 30, 2022
@ilyachur ilyachur enabled auto-merge (squash) September 30, 2022 06:21
auto-merge was automatically disabled October 13, 2022 03:03

Head branch was pushed to by a user without write access

@ilyachur ilyachur enabled auto-merge (squash) October 14, 2022 03:10
@ilyachur ilyachur merged commit 2d8cf90 into openvinotoolkit:master Oct 20, 2022
@ilya-lavrenov ilya-lavrenov added the ExternalPR External contributor label Mar 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@openvino-dev-samples openvino-dev-samples openvino-dev-samples left review comments

@meiyang-intel meiyang-intel meiyang-intel approved these changes

@ceciliapeng2011 ceciliapeng2011 ceciliapeng2011 approved these changes

@ilyachur ilyachur ilyachur approved these changes

Assignees

@ceciliapeng2011 ceciliapeng2011

Labels
category: PDPD FE OpenVINO PaddlePaddle FrontEnd ExternalPR External contributor PaddlePaddle Hackathon a Intel and Baidu joint Hackathon event
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@taixiurong @openvino-dev-samples @ilyachur @ceciliapeng2011 @meiyang-intel @ilya-lavrenov