-
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
[Good First Issue]: Align behavior of ONNX Frontend operator AveragePool-7, 10, 11, 19 with original framework #20553
Comments
Hello I woulld like to work on this issue. |
Thank you for giving me this issue to work on. |
I have created a table regarding the differences and changes in different versions of average pooling, however I am not getting on how to put these changes into the repository as mentioned in the issue. It would be great if I could get some help regarding this. |
Hi, could you put your table here? |
Sure, here is the table of differences I infered and created from the documentation link provided in the issue. I am also attatching my fork so you can see the changes which I have made till now |
@gkrivor sir, I had forgotten to put a small change in op-7 to the initial version of avg pool. Could you furthur assist me regarding the onnx frontend on how and where all to change the code. I have read the frontend docs but still facing some difficulties and issues, would be great if you could assist. |
@p-wysocki @gkrivor Sir, I have quite some changes in the repo to align op-7, 10, 11 and 19, however to make any changes furthur I would need to run test cases for which I would be required to build the repo, but for some reason my CMake command is giving errors while building the repo could you pls help me with this. I am getting the following error. cmake -G "Visual Studio 16 2019" "C:\Users\Sukhvansh Jain\Desktop\Open source\OpenVINO\openvino"
|
Hi, sorry for a delay, I have a time to check github tuesday-wednesday :( Looks like a great solution, but you touched a core part of OpenVINO, I'm not sure it is necessary. I think you've overcomplicated the solution. Let me check. I thought about something like this, it should be enough: Currently, biggest problem which requires are alignment/refactoring is a "some common implementation" of a translation function. What does it mean? It means it looks like as an any of documented opset (for example, 7,10,11,19), but doesn't implement exact opset functionality (allows to use an undocumented input types, use wrong inputs, etc). That's why we want to align behavior with the original documentation. Because otherwise, when you pass a model with a 19 opset it fails due to a common implementation uses wrong input (input instead of an attribute, as example), but you will get any possible error and will spend a lot of time to understand what goes wrong. I'm sure previous approach was very good for a fast implementation. But now it cause a misunderstanding while debugging and enabling new opsets. |
Hello, I've left review comments within the mentioned PR #22075, but let me put the summary here as well. Most of the currently failing ONNX AvgPool compliance tests are related to the "dilations" attribute, and in fact to cover it, new version of the ov AvgPool is needed (as it was done for ov MaxPool op). @gkrivor We should cover such request internally or by a separate, precisely described task for contributors. |
.take |
Thank you for looking into this issue! Please let us know if you have any questions or require any help. |
Hello @awayzjj, are you still working on this? Do you need any help? |
Yes, sorry for the late, I will create a PR this weekend. |
@gkrivor @mitruska Hi I aligned the behavior of AveragePool-7, 10, and 11, and created a PR. The AveragePool-19 added the Here is how the max-pool operator handles the openvino/src/core/src/op/max_pool.cpp Lines 419 to 429 in 999e20a
Its evaluate method ultimately calls reference::max_pool :openvino/src/core/src/op/max_pool.cpp Lines 189 to 196 in 999e20a
However, I found that the AvgPool operator does not override the evaluate method:openvino/src/core/include/openvino/op/avg_pool.hpp Lines 51 to 91 in 999e20a
Could you provide some guidance on where and how I should add the |
@p-wysocki fyi |
Context
Neural networks are graphs consisting of nodes called operators. Each operator corresponds to a mathematical function, usually described in framework's documentation or an AI standard, such as ONNX.
OpenVINO ONNX Frontend is a component responsible for working with ONNX graphs and requires implementation of different ONNX operators in order to use ONNX models.
This task requires alignment between OpenVINO ONNX Frontend and original framework implementations of AveragePool for next list of opsets: opset 7, opset 10, opset 11, opset 19
Necessary help will be provided by ONNX Fronted team.
What needs to be done?
Operator details can be found in ONNX Operators
More details can be found in ONNX Changelog: opset 7, opset 10, opset 11, opset 19
More details in adding operators to ONNX Frontend guide
Example Pull Requests
No response
Resources
Contact points
@gkrivor
Ticket
No response
The text was updated successfully, but these errors were encountered: