[ONNX] Extended ReduceMax by opsets 13,18,20 #23475
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,6 +29,15 @@ ov::OutputVector reduce_l2(const ov::frontend::onnx::Node& node); | |
| namespace set_1 { | ||
| ov::OutputVector reduce_max(const ov::frontend::onnx::Node& node); | ||
| } // namespace set_1 | ||
| namespace set_13 { | ||
|
There was a problem hiding this comment. This operator is also present in opset version 11. Is there any reason we're skipping version 11 ? In my case for ReduceLogSumExp there is no change between version 1 and 11, however maybe it would be good to include it for compatibility reasons ? There was a problem hiding this comment. Hi @mwilczy , I put a common motivation to do not introduce opset-11 for all Reduce* here |
||
| ov::OutputVector reduce_max(const ov::frontend::onnx::Node& node); | ||
| } // namespace set_13 | ||
| namespace set_18 { | ||
| ov::OutputVector reduce_max(const ov::frontend::onnx::Node& node); | ||
| } // namespace set_18 | ||
| namespace set_20 { | ||
| ov::OutputVector reduce_max(const ov::frontend::onnx::Node& node); | ||
| } // namespace set_20 | ||
|
|
||
| namespace set_1 { | ||
| ov::OutputVector reduce_mean(const ov::frontend::onnx::Node& node); | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,68 @@ | ||
| ir_version: 3 | ||
| producer_name: "OpenVINO ONNX Frontend" | ||
| graph { | ||
| node { | ||
| input: "A" | ||
| input: "axes" | ||
| output: "B" | ||
| op_type: "ReduceMax" | ||
| } | ||
| name: "compute_graph" | ||
| initializer { | ||
| data_type: 6 | ||
| dims: 1 | ||
| name: "axes" | ||
| raw_data: "\002\000\000\000" | ||
| } | ||
| input { | ||
| name: "A" | ||
| type { | ||
| tensor_type { | ||
| elem_type: 2 | ||
| shape { | ||
| dim { | ||
| dim_value: 1 | ||
| } | ||
| dim { | ||
| dim_value: 1 | ||
| } | ||
| dim { | ||
| dim_value: 4 | ||
| } | ||
| dim { | ||
| dim_value: 4 | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| input { | ||
| name: "axes" | ||
| type { | ||
| tensor_type { | ||
| elem_type: 6 | ||
| shape { | ||
| dim { | ||
| dim_value: 1 | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| output { | ||
| name: "B" | ||
| type { | ||
| tensor_type { | ||
| elem_type: 2 | ||
| shape { | ||
| dim { | ||
| dim_value: 1 | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| opset_import { | ||
| version: 18 | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| ir_version: 3 | ||
| producer_name: "OpenVINO ONNX Frontend" | ||
| graph { | ||
| node { | ||
| input: "A" | ||
| output: "B" | ||
| op_type: "ReduceMax" | ||
| } | ||
| name: "compute_graph" | ||
| input { | ||
| name: "A" | ||
| type { | ||
| tensor_type { | ||
| elem_type: 9 | ||
| shape { | ||
| dim { | ||
| dim_value: 1 | ||
| } | ||
| dim { | ||
| dim_value: 1 | ||
| } | ||
| dim { | ||
| dim_value: 4 | ||
| } | ||
| dim { | ||
| dim_value: 4 | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| output { | ||
| name: "B" | ||
| type { | ||
| tensor_type { | ||
| elem_type: 9 | ||
| shape { | ||
| dim { | ||
| dim_value: 1 | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| opset_import { | ||
| version: 13 | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| ir_version: 3 | ||
| producer_name: "OpenVINO ONNX Frontend" | ||
| graph { | ||
| node { | ||
| input: "A" | ||
| output: "B" | ||
| op_type: "ReduceMax" | ||
| } | ||
| name: "compute_graph" | ||
| input { | ||
| name: "A" | ||
| type { | ||
| tensor_type { | ||
| elem_type: 9 | ||
| shape { | ||
| dim { | ||
| dim_value: 1 | ||
| } | ||
| dim { | ||
| dim_value: 1 | ||
| } | ||
| dim { | ||
| dim_value: 4 | ||
| } | ||
| dim { | ||
| dim_value: 4 | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| output { | ||
| name: "B" | ||
| type { | ||
| tensor_type { | ||
| elem_type: 9 | ||
| shape { | ||
| dim { | ||
| dim_value: 1 | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| opset_import { | ||
| version: 20 | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Hi,
I'm working on new opsets for ReduceLogSumExp, and would like to take this PR as example.
I'm wondering whether inventing new versioning scheme for types is a good idea ? Wouldn't it be better to just put the constant supported_types into a namespace ?
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.
@mwilczy you mean why I didn't align it with an opset version? For example, supported_types_v1, v11, v13, etc. Or op::set_1::supported_types}, op::set_11::supported_types, etc. Did I get correct? I thought about it at first step but...
I motivated to do it as I did because I compared all differences as a table, and I found by text comparison between all versions of all Reduce* operations. A summary:
Types versioning isn't aligned between Reduce* versions. For example, ReduceMax/Min-13 has different list of supported types (added int8 and uint8) than all other Reduce*-13 operations. as I can see - opset-13 simultaneously introduced two different lists of supported types. I really don't like constructions like op::set_13::supported_types::v1 ;)
So, supported_types for ReduceMax/Min will look different. And in such case I may need to introduce more intermediate entities like: define four different lists in a common part and use it thru... "using support_types = supported_types_v3" for each Reduce*? I think it will overcomplicate the source without any benefits for reading. In my opinion it could be reasonable in case we would separate Reduce* operations for a set of files (maybe it will be done when we will have all implementations ready).
Hope, I've answered on your question...