[ONNX] Extend ONNX Frontend with BlackmanWindow, HammingWindow and HannWindow operators #19428
Conversation
also added basic tests for each
|
Hey @mbencer @p-wysocki could you help me out with this? This is my first time contributing here. |
|
Hey @siddhant-0707, thanks for the PR! @gkrivor could you please take a look? |
|
@siddhant-0707 there was an issue with CI on master, it has been fixed yesterday. Please update your branch, the checks failing on CMake step (all of them from what I see) will now start building and run actual tests. Sorry for the inconvenience. :) |
|
@siddhant-0707 do you have access to Azure pipelines logs? There's apparently some issue with building the new cpp files. |
|
@p-wysocki Yep I can see the Azure pipelines logs. Will get back to you tomorrow. |
|
@siddhant-0707 , btw, which code editor do you use? If it is MS VS - use "Ctrl + K, Ctrl + D" to fix coding style issues. Or thru menu Edit > Advanced > Format Document |
|
Hey guys I made the requested changes but need some help with the coding style issues. I ran Format Document from the command palette (VSCode) and it seemed to work. Format looks same as other files in the repo but tests still failing. |
used output_datatype directly, returned y_values directly
siddhant-0707
changed the title
|
Hi, @siddhant-0707! Great to see you are continue improving the PR! I'll try to build your change locally today or tomorrow, I think you should just use some additional macro to prevent deprecation warnings. But I need to clarify some points before. |
There was a problem hiding this comment.
Hi, I tried to verify your implementation, could you take a look on my comments and align your code? It should fix most issues.
I saw an issue on GPU, if you will see similar (or our CI will see it) - we may need to disable tests for the device (let's see it later).
src/frontends/onnx/tests/models/blackmanwindow_symmetric.prototxt
Outdated
Show resolved
Hide resolved
src/frontends/onnx/tests/models/blackmanwindow_periodic.prototxt
Outdated
Show resolved
Hide resolved
src/frontends/onnx/tests/models/blackmanwindow_periodic.prototxt
Outdated
Show resolved
Hide resolved
|
Great job! Let's wait for CI results! |
|
Congratulations with green CI! I'm waiting when our internal works will be finished and I'll run last pre-commit checks before merge. |
|
build_jenkins |
|
Thank you so much for the help @gkrivor 😃 |
|
That's an enormous piece of work. Thank you and congratulations @siddhant-0707! |
…nnWindow operators (openvinotoolkit#19428) * ONNX BlackManWindow enabled * added a test periodic * Add the license statement * ONNX HammingWindow, HannWindow enabled also added basic tests for each * minor tests added * made reviewed changes * made reviewed changes used output_datatype directly, returned y_values directly * fixed clang-format * add OPENVINO_SUPPRESS_DEPRECATED_START * include math.h * float fix * fix * fix namespace to set_1 * test fixes * fix cast to output_datatype * fix, replace cast with ov::convert * fix, use element::f32 * major fixes * fixes * Update onnx_import.in.cpp * Update onnx_import.in.cpp --------- Co-authored-by: Przemyslaw Wysocki <przemyslaw.wysocki@intel.com>
…nnWindow operators (openvinotoolkit#19428) * ONNX BlackManWindow enabled * added a test periodic * Add the license statement * ONNX HammingWindow, HannWindow enabled also added basic tests for each * minor tests added * made reviewed changes * made reviewed changes used output_datatype directly, returned y_values directly * fixed clang-format * add OPENVINO_SUPPRESS_DEPRECATED_START * include math.h * float fix * fix * fix namespace to set_1 * test fixes * fix cast to output_datatype * fix, replace cast with ov::convert * fix, use element::f32 * major fixes * fixes * Update onnx_import.in.cpp * Update onnx_import.in.cpp --------- Co-authored-by: Przemyslaw Wysocki <przemyslaw.wysocki@intel.com>
Details:
Tickets:
BlackmanWindow,HammingWindowandHannWindowoperators #18483