Skip to content

dnn: expand refactor with cv::broadcast for onnx models#24295

Merged
vpisarev merged 10 commits intoopencv:4.xfrom
fengyuentau:add_onnx_expand
Sep 27, 2023
Merged

dnn: expand refactor with cv::broadcast for onnx models#24295
vpisarev merged 10 commits intoopencv:4.xfrom
fengyuentau:add_onnx_expand

Conversation

@fengyuentau
Copy link
Copy Markdown
Member

@fengyuentau fengyuentau commented Sep 19, 2023 •
edited
Loading

Resolves #24300
Resolves #24308

Merge with opencv/opencv_extra#1098.

Motivation:

Current implementation of ONNX Expand is very limited and hard to maintain. It can go very unefficient, e.g. input of shape [1, 5, 1] and shape of value [1, 5, 256], it leads to 256 x [1, 5, 1] constant nodes along with a concat node. See down below for more details .

opencv/modules/dnn/src/onnx/onnx_importer.cpp

Lines 2485 to 2494 in b870ad4

else if (broadcast_axes.size() == 1)
{
// FIXME: this will end up creating massive amount of Identity nodes for broadcasting,
// for example, broadcast 1 to 256 needs 256 Identity nodes and 1 Concat node.
// Possible improvement is to use "Scale".
expandMid(layerParams.name, node_proto, srcName, targetShape[broadcast_axes[0]]);
layerParams.set("axis", broadcast_axes[0]);
layerParams.type = "Concat";
node_proto.set_output(0, output_name);

Since now we have cv::broadcast already, it is time to do a refactor on expand.

Checklist:

  • handle all-input-constant case
  • fix expand_neg_batch.onnx
  • take care of 0d/1d tensor

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 Sep 19, 2023
@fengyuentau fengyuentau added this to the 4.9.0 milestone Sep 19, 2023
@fengyuentau fengyuentau marked this pull request as ready for review September 21, 2023 02:49
@fengyuentau fengyuentau requested review from dkurt and rogday September 21, 2023 02:49
@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented Sep 21, 2023

@fengyuentau Could the patch unlock some "conformance" tests for ONNX?

@fengyuentau
Copy link
Copy Markdown
Member Author

fengyuentau commented Sep 21, 2023

@asmorkalov Unfortunately, all Expand conformance tests have input shape as non-constant, which cannot be supported in the current dnn engine. However, I can still convert all of them with constant shape. Let me do it.

@fengyuentau fengyuentau mentioned this pull request Sep 21, 2023
Closed
4 tasks
@fengyuentau
add expand conformance tests; add checks to disallow shape of neg val…
6c9d366 
…ues; add early copy for unchanged total elements
@fengyuentau
Copy link
Copy Markdown
Member Author

fengyuentau commented Sep 21, 2023 •
edited
Loading

CI will not pass due to #24308. Need to fix the problem in another PR first.

Decided to fix the issue here in this pull request. Basically the wrong result is given because of shape is used as input shape for Expand, which has value [2, -1, -1, -1] (See line 838):

opencv/modules/dnn/src/onnx/onnx_graph_simplifier.cpp

Lines 824 to 840 in 3889dcf

class ExpandSubgraph : public Subgraph
{
public:
ExpandSubgraph()
{
int input = addNodeToMatch("");
int values = addNodeToMatch("");
int init = addNodeToMatch("ConstantOfShape", values);
int coeff = addNodeToMatch("Constant");
int mul = addNodeToMatch("Mul", init, coeff);
int shape = addNodeToMatch("Constant");
int condition = addNodeToMatch("Equal", shape, mul);
int where = addNodeToMatch("Where", condition, init, addNodeToMatch("Constant"));
addNodeToMatch("Expand", input, where);
setFusedNode("Expand", input, shape);
}
};

@fengyuentau fengyuentau mentioned this pull request Sep 23, 2023
Merged
@fengyuentau
Copy link
Copy Markdown
Member Author

fengyuentau commented Sep 23, 2023

CI is not passing again because of #24300

@fengyuentau
Copy link
Copy Markdown
Member Author

fengyuentau commented Sep 23, 2023

All tests are green now.

opencv-alalek
opencv-alalek reviewed Sep 23, 2023
View reviewed changes
@vpisarev vpisarev self-requested a review September 27, 2023 06:27
@vpisarev vpisarev merged commit bb171a0 into opencv:4.x Sep 27, 2023
@fengyuentau fengyuentau mentioned this pull request Sep 27, 2023
Merged
8 tasks
@fengyuentau fengyuentau deleted the add_onnx_expand branch September 27, 2023 07:02
@asmorkalov asmorkalov mentioned this pull request Sep 28, 2023
Merged
hanliutong pushed a commit to hanliutong/opencv that referenced this pull request Oct 7, 2023
@fengyuentau @hanliutong
dnn: expand refactor with cv::broadcast for onnx models (opencv#24295)
28fefc3 
* add expand impl with cv::broadcast

* remove expandMid

* deduce shape from -1

* add constant folding

* handle input constant; handle input constant 1d

* add expand conformance tests; add checks to disallow shape of neg values; add early copy for unchanged total elements

* fix ExpandSubgraph

* dummy commit to trigger build

* dummy commit to trigger build 1

* remove conformance from test names
thewoz pushed a commit to thewoz/opencv that referenced this pull request Jan 4, 2024
@fengyuentau @thewoz
dnn: expand refactor with cv::broadcast for onnx models (opencv#24295)
e8b39a2 
* add expand impl with cv::broadcast

* remove expandMid

* deduce shape from -1

* add constant folding

* handle input constant; handle input constant 1d

* add expand conformance tests; add checks to disallow shape of neg values; add early copy for unchanged total elements

* fix ExpandSubgraph

* dummy commit to trigger build

* dummy commit to trigger build 1

* remove conformance from test names
@fengyuentau fengyuentau mentioned this pull request Feb 21, 2024
Open
48 tasks
thewoz pushed a commit to thewoz/opencv that referenced this pull request May 29, 2024
@fengyuentau @thewoz
dnn: expand refactor with cv::broadcast for onnx models (opencv#24295)
9dde77d 
* add expand impl with cv::broadcast

* remove expandMid

* deduce shape from -1

* add constant folding

* handle input constant; handle input constant 1d

* add expand conformance tests; add checks to disallow shape of neg values; add early copy for unchanged total elements

* fix ExpandSubgraph

* dummy commit to trigger build

* dummy commit to trigger build 1

* remove conformance from test names
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@opencv-alalek opencv-alalek opencv-alalek left review comments

@vpisarev vpisarev vpisarev approved these changes

@dkurt dkurt Awaiting requested review from dkurt

@rogday rogday Awaiting requested review from rogday

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

4.9.0

4 participants

@fengyuentau @asmorkalov @vpisarev @opencv-alalek