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 flip op #15828

Merged
merged 18 commits into from
May 19, 2023

Conversation

Asthestarsfalll
Copy link
Contributor

@Asthestarsfalll Asthestarsfalll commented Feb 20, 2023

@Asthestarsfalll Asthestarsfalll requested a review from a team as a code owner February 20, 2023 20:04
@github-actions github-actions bot added the category: PDPD FE OpenVINO PaddlePaddle FrontEnd label Feb 20, 2023
@Asthestarsfalll Asthestarsfalll changed the title add flip 【PaddlePaddle Hackathon 4】add flip op Feb 21, 2023
@Asthestarsfalll Asthestarsfalll changed the title 【PaddlePaddle Hackathon 4】add flip op 【PaddlePaddle Hackathon 4】add paddle flip op Feb 21, 2023
@Asthestarsfalll Asthestarsfalll requested review from a team as code owners February 21, 2023 06:39
@Asthestarsfalll Asthestarsfalll requested review from bstankix and eaidova and removed request for a team February 21, 2023 06:39
@github-actions github-actions bot added category: build OpenVINO cmake script / infra category: CI OpenVINO public CI category: Core OpenVINO Core (aka ngraph) category: CPP API OpenVINO CPP API bindings labels Feb 21, 2023
Copy link
Contributor

@rkazants rkazants left a comment

Choose a reason for hiding this comment

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

One comment left. Other LGTM.

src/frontends/paddle/src/op/flip.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@mvafin mvafin left a comment

Choose a reason for hiding this comment

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

Looks good from my side, but would like to see approve from @ceciliapeng2011

@Asthestarsfalll
Copy link
Contributor Author

@ceciliapeng2011 PTAL

@Asthestarsfalll
Copy link
Contributor Author

@ceciliapeng2011 PTAL

@Asthestarsfalll
Copy link
Contributor Author

@OpenVINO-dev-contest PTAL

@yuxu42
Copy link
Contributor

yuxu42 commented May 9, 2023

@ceciliapeng2011 could you please review?

@openvino-comment-bot
Copy link

Can one of the admins verify this patch?

@itikhono
Copy link
Contributor

@Asthestarsfalll I have a question here. flip is an alias of reverse, right? If so, why not map it using Reverse IR. refer to #17525

As I can see, Reverse op was deleted in opset2. We don't have this operator in opset2+
It's better to fix reverse op convertor (translator) to use ReverseSequence op or similar approach like in this PR if possible

@xczhai
Copy link
Contributor

xczhai commented May 18, 2023

@Asthestarsfalll I have a question here. flip is an alias of reverse, right? If so, why not map it using Reverse IR. refer to #17525

As I can see, Reverse op was deleted in opset2. We don't have this operator in opset2+ It's better to fix reverse op convertor (translator) to use ReverseSequence op or similar approach like in this PR if possible

PD reverse op executes the same operation. It is better to unify the two mappings instead of two separate implement.

@itikhono
Copy link
Contributor

@Asthestarsfalll I have a question here. flip is an alias of reverse, right? If so, why not map it using Reverse IR. refer to #17525

As I can see, Reverse op was deleted in opset2. We don't have this operator in opset2+ It's better to fix reverse op convertor (translator) to use ReverseSequence op or similar approach like in this PR if possible

PD reverse op executes the same operation. It is better to unify the two mappings instead of two separate implement.

Yes, I agree. But the current pd reverse op translator uses OV Reverse op that was deleted from officially supported opsets (from opset2 and later). We need to fix reverse op translator in this case

@xczhai
Copy link
Contributor

xczhai commented May 18, 2023

@Asthestarsfalll I have a question here. flip is an alias of reverse, right? If so, why not map it using Reverse IR. refer to #17525

As I can see, Reverse op was deleted in opset2. We don't have this operator in opset2+ It's better to fix reverse op convertor (translator) to use ReverseSequence op or similar approach like in this PR if possible

PD reverse op executes the same operation. It is better to unify the two mappings instead of two separate implement.

Yes, I agree. But the current pd reverse op translator uses OV Reverse op that was deleted from officially supported opsets (from opset2 and later). We need to fix reverse op translator in this case

@Asthestarsfalll Hi, it is much appreciated if you can help unify reverse and flip implements together.

@Asthestarsfalll
Copy link
Contributor Author

@Asthestarsfalll Hi, it is much appreciated if you can help unify reverse and flip implements together.

I will update code latter.

@ceciliapeng2011 ceciliapeng2011 self-assigned this May 18, 2023
@ceciliapeng2011 ceciliapeng2011 enabled auto-merge (squash) May 18, 2023 11:23
auto-merge was automatically disabled May 19, 2023 04:41

Head branch was pushed to by a user without write access

@ceciliapeng2011 ceciliapeng2011 enabled auto-merge (squash) May 19, 2023 06:02
@ceciliapeng2011 ceciliapeng2011 merged commit 36dbe95 into openvinotoolkit:master May 19, 2023
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.

9 participants