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

Align behavior of ONNX Frontend function ReduceSumSquare-11, 13, 18 with original framework #22993

Conversation

inbasperu
Copy link
Contributor

@inbasperu inbasperu commented Feb 21, 2024

Hello maintainers,

I have implemented the first three steps of the solution proposed in issue #20563 for the suggested table. To maintain DRY code, I created helper functions within an anonymous namespace. I also verified that the current implementation accounts for the negative range by reviewing the core helper functions.

For Opset 18, when axes are not specified as an attribute, the helper function defaults to using noop_with_empty_axes with a default value of 0. This is implemented in src/frontends/onnx/frontend/src/op/reduce.cpp.

In addressing reduction over an empty set, I opted for a case statement that returns 0 early.

However, I have encountered uncertainties regarding type constraints. The ONNX changelog indicates that Opset 1 supports f16, f32, i32, i64, u32, u64, and double. Nevertheless, the current implementation, as referenced here, seems not to support the double data type, and I am unsure if this oversight is correct.

Moreover, for Opsets 13 and 18, there is a need to enforce the bfloat16 type constraint, which the current implementation does not address. I believe changes are required in the core operator modules at reduce_sum.cpp. It would be great if someone could guide me on how to proceed with enforcing this type constraint.

If the implementation approach is deemed acceptable, I will begin working on the test models.

Closes #20563 (comment)

@p-wysocki
Copy link
Contributor

cc @gkrivor

@p-wysocki
Copy link
Contributor

build_jenkins

@gkrivor
Copy link
Contributor

gkrivor commented Feb 28, 2024

Hi @inbasperu, thank you for your contribution!

I've just merged (#22462) a fix for Reduce* shape inference which should allow you to do not overcomplicate your solution. Also, your change for handling of zero-rank may solve a different issue, probably for other Reduce* operations too, I didn't check it yet. I suggest to move it to separate PR.

Also, could you remove reduce_sum_square test here

"OnnxBackendNodeModelTest.test_reduce_sum_square_do_not_keepdims_example_cpu",
? After adding opset-18 they should be passed.

So, my thoughts are:

  1. Leave implementations for opset-11,13,18 as you did
  2. Add additional checks for opset-1,11,13 (verify we accept correct amount of inputs and type of them)
  3. Enable tests I mentioned before, verify them locally if possible, it might not be easy, don't spend much time
  4. Yep, you find right, you need to add additional check for double and fail with another message - it isn't supported by OpenVINO

I'll check with a team about support for a missing types. Because we also may work with "unsupported types" as with regular float (f32), but with loosing some accuracy.

@gkrivor
Copy link
Contributor

gkrivor commented Mar 13, 2024

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

@gkrivor
Copy link
Contributor

gkrivor commented Mar 15, 2024

Hi @inbasperu ,

Please take a look on #23475 as a reference.

Your implementation is missing for tests at the moment and you have a some extra changes which I think we may need to apply as a separate PR (I'm talking about zero-rank). I'll take a look on it, please, do not add it in this change right now.

Copy link
Contributor

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

@inbasperu
Copy link
Contributor Author

Hey @gkrivor,
Since there have been many changes made to the master branch and my current code includes some additional changes (specifically regarding zero rank), I thought it would be best to create two new PRs. Please take a look at the PR#23798 I've submitted to align the behavior of the ONNX Frontend function ReduceSumSquare-11, 13, 18 with the original framework.

Also, would it be possible to create an issue addressing the zero rank task and assign me to that task? That way, I can work on it once I complete this current task. Thank you!

github-merge-queue bot pushed a commit that referenced this pull request May 23, 2024
…ith original framework (#23798)

Hello maintainers,

Since my previous PR
(#22993 (comment))
had diverged significantly from the master branch, and also included
additional changes, I thought it would be best to create a new PR.
@gkrivor, could you please take a look?

This closes
#20563 (comment).

---------

Co-authored-by: Georgy Krivoruchko <georgy.krivoruchko@intel.com>
@inbasperu inbasperu deleted the dev_align_reduce_sum_square_behavior_20563 branch June 26, 2024 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: ONNX FE OpenVINO ONNX FrontEnd ExternalIntelPR External contributor from Intel Stale
Projects
None yet
4 participants