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

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

Merged
merged 10 commits into from Sep 27, 2023

Conversation

fengyuentau
Copy link
Member

@fengyuentau fengyuentau commented Sep 19, 2023

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 .

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
@asmorkalov
Copy link
Contributor

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

@fengyuentau
Copy link
Member Author

@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.

…ues; add early copy for unchanged total elements
@fengyuentau
Copy link
Member Author

fengyuentau commented Sep 21, 2023

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):

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
Copy link
Member Author

CI is not passing again because of #24300

@fengyuentau
Copy link
Member Author

All tests are green now.

@vpisarev vpisarev self-requested a review September 27, 2023 06:27
@vpisarev vpisarev merged commit bb171a0 into opencv:4.x Sep 27, 2023
23 checks passed
@fengyuentau fengyuentau deleted the add_onnx_expand branch September 27, 2023 07:02
@asmorkalov asmorkalov mentioned this pull request Sep 28, 2023
hanliutong pushed a commit to hanliutong/opencv that referenced this pull request Oct 7, 2023
* 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
* 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
48 tasks
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
4 participants