Skip to content

dnn onnx graph simplifier: handle optional inputs of Slice#24655

Merged
asmorkalov merged 2 commits intoopencv:4.xfrom
fengyuentau:graph_simplifier_optional_input
Dec 6, 2023
Merged

dnn onnx graph simplifier: handle optional inputs of Slice#24655
asmorkalov merged 2 commits intoopencv:4.xfrom
fengyuentau:graph_simplifier_optional_input

Conversation

@fengyuentau
Copy link
Copy Markdown
Member

Resolves #24609

Pull Request Readiness Checklist

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

  • I agree to contribute to the project under Apache 2 License.
  • To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV
  • The PR is proposed to the proper branch
  • There is a reference to the original bug report and related work
  • There is accuracy test, performance test and test data in opencv_extra repository, if applicable
    Patch to opencv_extra has the same branch name.
  • The feature is well documented and sample code can be built with the project CMake

@fengyuentau fengyuentau added category: dnn category: dnn (onnx) ONNX suport issues in DNN module labels Dec 6, 2023
@fengyuentau fengyuentau requested a review from dkurt December 6, 2023 03:12
@fengyuentau
Copy link
Copy Markdown
Member Author

@dkurt What do you think about this solution?

@dkurt dkurt self-assigned this Dec 6, 2023

int shape2 = addNodeToMatch("Shape", input);
int slice = addNodeToMatch("Slice", shape2, addNodeToMatch("Constant"), addNodeToMatch("Constant"), addNodeToMatch("Constant"));
int slice = addNodeToMatch("Slice", shape2, addNodeToMatch("Constant"), addNodeToMatch("Constant"));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should ResizeSubgraph1 be modified in the same way?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. This subgraph is added to simplify model resize_nearest_unfused_opset11_torch1.4.onnx in opencv_extra. This graph does not have shape pre-inferred, so there is no value info proto for the input of the Slice operator. The actual axes value is 0, which is not -1 and cannot be recognized as the last axis because there is no shape inference information.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. So as I tried propose here can we align all the Slice layers matches to accept 5 mandatory inputs but add a pattern which appends optional inputs if missed?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, current proposal also looks good to me and it make sense to merge to unblock Attention layer.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

5 inputs

I would say it can be more robust as it also handles cases where value info proto is not available. However, it needs to create ONNX Constant nodes for axes and steps in finalize() which I am not sure how to do.

}

/* Slice operator has two optional inputs "axes" and "steps". Some models may be set to have
Slice with optional inputs of default values, some of them don't. This Subgraph removes
Copy link
Copy Markdown
Member

@dkurt dkurt Dec 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some of them don't

Means two things - they still have such inputs but with different values or they don't have inputs at all.

Can we do matching-fusion in the opposite way - align all the Slice layers (with less than 5 inputs) to some canonical version with 5 inputs? Filling missed values with default ones.

@asmorkalov asmorkalov added this to the 4.9.0 milestone Dec 6, 2023
@asmorkalov asmorkalov merged commit f5ec92e into opencv:4.x Dec 6, 2023
@fengyuentau fengyuentau deleted the graph_simplifier_optional_input branch December 6, 2023 11:42
@dkurt dkurt mentioned this pull request Dec 7, 2023
13 tasks
@asmorkalov asmorkalov mentioned this pull request Jan 19, 2024
thewoz pushed a commit to thewoz/opencv that referenced this pull request May 29, 2024
…ional_input

dnn onnx graph simplifier: handle optional inputs of Slice opencv#24655

Resolves opencv#24609

### Pull Request Readiness Checklist

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

- [x] I agree to contribute to the project under Apache 2 License.
- [x] To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV
- [x] The PR is proposed to the proper branch
- [x] There is a reference to the original bug report and related work
- [x] There is accuracy test, performance test and test data in opencv_extra repository, if applicable
      Patch to opencv_extra has the same branch name.
- [x] The feature is well documented and sample code can be built with the project CMake
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category: dnn (onnx) ONNX suport issues in DNN module category: dnn

Projects

None yet

Development

Successfully merging this pull request may close these issues.

dnn graph simplifier: support optional constant inputs when match

3 participants