Skip to content

bug fixed of GEMM node in ONNX_importer#22122

Merged
opencv-pushbot merged 1 commit intoopencv:4.xfrom
zihaomu:gemm_onnx_bug_fix
Jun 22, 2022
Merged

bug fixed of GEMM node in ONNX_importer#22122
opencv-pushbot merged 1 commit intoopencv:4.xfrom
zihaomu:gemm_onnx_bug_fix

Conversation

@zihaomu
Copy link
Copy Markdown
Member

@zihaomu zihaomu commented Jun 17, 2022 •
edited by alalek
Loading

Merge with extra: opencv/opencv_extra#982

Related Issue 22096

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

@zihaomu zihaomu mentioned this pull request Jun 17, 2022
Closed
@zihaomu zihaomu requested a review from rogday June 17, 2022 03:52
TE-ShiyuYang
TE-ShiyuYang reviewed Jun 17, 2022
View reviewed changes
modules/dnn/src/onnx/onnx_importer.cpp Outdated
Mat weights = getBlob(node_proto, 1);
int ind_num_out = 0;

if (layerParams.has("transB") && !layerParams.get<int>("transB")) {
Copy link
Copy Markdown

@TE-ShiyuYang TE-ShiyuYang Jun 17, 2022

Choose a reason for hiding this comment

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

Sometimes onnx operator will not set attr, for example:

onnx.helper.make_node(
            'Gemm',
            ['a', 'b', 'c'],
            ['y'],
            #transB=0)

Both transB and transA are not specified, in case like this, onnx would treat attrs as default value(which is 0),

Copy link
Copy Markdown
Member Author

@zihaomu zihaomu Jun 17, 2022

Choose a reason for hiding this comment

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

Thanks for mentioning this, it has been fixed.

@zihaomu zihaomu force-pushed the gemm_onnx_bug_fix branch from 84b3f55 to 581d7dc Compare June 17, 2022 05:05
@zihaomu zihaomu mentioned this pull request Jun 21, 2022
Merged
rogday
rogday reviewed Jun 21, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@rogday rogday left a comment

Choose a reason for hiding this comment

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

Thank you!

modules/dnn/src/onnx/onnx_importer.cpp Outdated
int ind_num_out = 0;
if (layerParams.has("transB") && !layerParams.get<int>("transB")) {

if (!layerParams.has("transB") || (layerParams.has("transB") && !layerParams.get<int>("transB")))
Copy link
Copy Markdown
Member

@rogday rogday Jun 21, 2022

Choose a reason for hiding this comment

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

You can supply default value to get: if (!layerParams.get<int>("transB", 0))

Copy link
Copy Markdown
Member Author

@zihaomu zihaomu Jun 22, 2022

Choose a reason for hiding this comment

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

Agree.

@zihaomu zihaomu force-pushed the gemm_onnx_bug_fix branch from 7a093b0 to 2411b82 Compare June 22, 2022 07:01
rogday
rogday approved these changes Jun 22, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@rogday rogday left a comment

Choose a reason for hiding this comment

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

LGTM!

@opencv-pushbot opencv-pushbot merged commit 24a66a4 into opencv:4.x Jun 22, 2022
@rogday
Copy link
Copy Markdown
Member

rogday commented Jun 22, 2022

@zihaomu, sorry, I missed the branch name. The same bug is present in 3.4 branch. Please create another PR backporting the fix. As for the future PRs: as a rule of thumb, we fix everything in 3.4 and merge the fixes into 4.x. New features go straight to 4.x.

@zihaomu
Copy link
Copy Markdown
Member Author

zihaomu commented Jun 22, 2022

Hi @rogday. Does this mean that if 3.4 and 4.x have the same bug, we only push code to 3.4 branch?

@rogday
Copy link
Copy Markdown
Member

rogday commented Jun 22, 2022

Hi @rogday. Does this mean that if 3.4 and 4.x have the same bug, we only push code to 3.4 branch?

Yup.

@zihaomu zihaomu mentioned this pull request Jun 22, 2022
Merged
6 tasks
@alalek alalek mentioned this pull request Aug 21, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@TE-ShiyuYang TE-ShiyuYang TE-ShiyuYang left review comments

@rogday rogday rogday approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@rogday rogday

Labels

bug category: dnn (onnx) ONNX suport issues in DNN module category: dnn port/backport done Label for maintainers. Authors of PR can ignore this test

Projects

None yet

Milestone

4.7.0

Development

Successfully merging this pull request may close these issues.

5 participants

@zihaomu @rogday @TE-ShiyuYang @opencv-pushbot @alalek