-
Notifications
You must be signed in to change notification settings - Fork 2.8k
【PaddlePaddle Hackathon 3 No.102】support paddle elementwise_floordiv #13059
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
Conversation
75d39a3 to
db4369c
Compare
openvino-dev-samples
left a comment
There was a problem hiding this 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
858f9e9 to
e19dc70
Compare
|
Hi, @meiyang-intel @ceciliapeng2011 could you also help to review this PR, thanks |
| 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": |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 .
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
|
Please fix clang format error. It could fixed with 2 options - Method2: |
d527367 to
ea5a609
Compare
ce58b89 to
17840ec
Compare
| 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]) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
- 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:
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?
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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:

do want to push new code ?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update
There was a problem hiding this comment.
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.
| if (pythondiv) { |
| bool pythondiv, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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]
| out[i] = arg0[i] / arg1[i]; |
the logic is same.
681d47e to
6f54f6f
Compare
Head branch was pushed to by a user without write access


Details:
Reference
https://www.paddlepaddle.org.cn/documentation/docs/zh/1.8/api_cn/layers_cn/elementwise_floordiv_cn.html#elementwise-floordiv
https://www.paddlepaddle.org.cn/documentation/docs/zh/api/paddle/floor_divide_cn.html#floor-divide
Unit-test passed