-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Adding ( [ONNX] Extend ONNX Frontend with Function Mish-18 ) #22833
Conversation
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.
Hello @PRATHAM-SPS, thank you for this contribution!
In general the import code looks correct, just few comments.
Also please align the code style to pass the CI check,
call make clang_format_fix_all
from build directory to apply auto formatting.
ov::OutputVector mish(const ov::frontend::onnx::Node& node) { | ||
const auto data = node.get_ov_inputs().at(0); | ||
|
||
return std::make_shared<v4::Mish>(data)->outputs(); |
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.
Is there any benefit of usage ->outputs()
here?
If no specific reason, I would suggest to align the return statement with the rest of the onnx fe ops:
return std::make_shared<v4::Mish>(data)->outputs(); | |
return {std::make_shared<v4::Mish>(data)}; |
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.
No there is no specific reason for that, I have commited all the changes that you have suggested in the new commit.Thank you for your guidance and support .
Any suggestion?? How should I fix it? |
you need install clang-format-9 tool |
Is there any other solution for it? I have ubuntu 22.04 it come with clang-format-14 and I am facing lots of issues to install the clang-format-9 externally. |
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.
@PRATHAM-SPS You can also try to format the files with your IDE, there is a chance that the output will be compatible. If not, I've added the code style suggestions in the comments, that should fix the style-check.
Hi @PRATHAM-SPS, we started a discussion on that internally - we'll do our best to provide a solution to that as soon as possible. :) For now please use @mitruska's comments. |
Hi @p-wysocki, @mitruska, @ilya-lavrenov, Thank you for looking into this internally. I appreciate the effort. I've followed @mitruska's comments as instructed and have made the necessary changes. Thank you for your guidance. Best regards, |
Hello @mitruska @p-wysocki @ilya-lavrenov can someone please initiate CI checks ? |
build_jenkins |
Details:
Tickets: