Align behavior of ONNX Frontend function ReduceL2-11, 13, 18 with original framework

[LucaTamSapienza]

Details:

• I've aligned the ReduceL2 operation with opset 11, 13, and 18. I have some doubts about how to implement support for the tensor bfloat16 for opset 13 and also some doubts about opset 18. I've registered the function inside the ops_bridge.cpp, created test models, and added them inside onnx_import.in.cpp.

Tickets:

• Closes #20560

[p-wysocki]

Hello @LucaTamSapienza, is this PR ready for review?

[LucaTamSapienza]

Hello @LucaTamSapienza, is this PR ready for review?

Hi @p-wysocki. The code compiles, but I have some pending issues. I have described my problems in the PR specification. I was waiting for someone to provide me with some clarifications on the doubts I encountered. I took a look at softmax.cpp file and to this conversation to see how to implement the features of the different versions. Unfortunately, I still have some doubts about opset13, on how to support the bfloat input. And I'm not too sure about my implementation of opset11. If you could take a look at my code and give me some feedback, it would be greatly appreciated. Thanks!

[LucaTamSapienza]

Please, rebase your solution to a fresh master, because I've merged a change #22462 with fix for all Reduce* operations which may affect your test cases.

Sure, i'll sync && pull immediatly.

Also, could you enable tests here

Do you mean for v 11, 13 ,18?

[gkrivor]

Hi, I refactored reduce.cpp and reduce.hpp. I think it should be more simple to add a new functionality.

Please, rebase after merge.

https://github.com/openvinotoolkit/openvino/pull/23429

[LucaTamSapienza]

Hi, I refactored reduce.cpp and reduce.hpp. I think it should be more simple to add a new functionality.

Please, rebase after merge.

#23429

Thanks for it, I see that it has been merged. I'll work on it.

[gkrivor]

Hi @LucaTamSapienza ,

Please take a look on https://github.com/openvinotoolkit/openvino/pull/23475 as a reference.

Especially take a look on test cases. For example, as I can see you've missed in opset-18 prototxt mention of an "axes" input. It is very important to verify this way.

Also check changes in onnx/tests/init.py and onnx/tests/tests_python/test_backend.py

[LucaTamSapienza]

Please take a look on #23475 as a reference.

@gkrivor ,

Thank you very much for your help and linking me to this PR, I have properly aligned versions 13 and 18. I noticed that you left a comment under version 11. Perhaps I should remove the call from reduce.cpp file and simply use the same implementation used in v1 by calling it in reduce.hpp within v11? Something like

namespace set_11 {
using set_1::reduce_l2;
} // namespace set_11

Especially take a look on test cases. For example, as I can see you've missed in opset-18 prototxt mention of an "axes" input. It is very important to verify this way.

For this correction, I took inspiration from the prototxt of the reduceMax operation in set_18. Now should be good.

Also check changes in onnx/tests/init.py and onnx/tests/tests_python/test_backend.py

I've deleted the ReduceL2 references from ticket 99962 && 99968 inside the init.py and removed for 99968 declaration about reduce_l2. I just have some doubts about this "OnnxBackendNodeModelTest.test_reduce_l2_empty_set_cpu", should I leave this?

[gkrivor]

Hi @LucaTamSapienza, thanks for the update!

Regarding opset-11. Actually, I think it is a good idea, I also thought about something like this. But currently, we have an internal discussion about "following the standards". I suggest to keep it as is. When we will decide how strictly we want to support standards - I could easily extend/reduce code base.

Also, I don't recommend to touch this place because several developers are working on reduce* operations right now and you may introduce additional merge-conflicts between you and them.

About "OnnxBackendNodeModelTest.test_reduce_l2_empty_set_cpu" - depends on a test result. If it become passing (you will see in test results), and leave it if it is still xfailed.

[LucaTamSapienza]

Also, I don't recommend to touch this place because several developers are working on reduce* operations right now and you may introduce additional merge-conflicts between you and them.

You're right @gkrivor , I've just resolved some merge conflicts I had, now everything should be fine.

About "OnnxBackendNodeModelTest.test_reduce_l2_empty_set_cpu" - depends on a test result. If it become passing (you will see in test results), and leave it if it is still xfailed.

I ran /bin/arm64/Release/ov_onnx_frontend_tests --gtest_filter=\*reduce_l2\* and all the tests passed successfully.

[gkrivor]

build_jenkins

[gkrivor]

build_jenkins

[gkrivor]

build_jenkins

[github-actions[bot]]

This PR will be closed in a week because of 2 weeks of no activity.