Skip to content

Fix optional outputs support#21490

Merged
opencv-pushbot merged 1 commit intoopencv:3.4from
rogday:optional_outputs
Jan 26, 2022
Merged

Fix optional outputs support#21490
opencv-pushbot merged 1 commit intoopencv:3.4from
rogday:optional_outputs

Conversation

@rogday
Copy link
Copy Markdown
Member

@rogday rogday commented Jan 20, 2022 •
edited
Loading

https://github.com/onnx/onnx/blob/main/docs/IR.md#optional-inputs-and-outputs

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

@rogday rogday added bug category: dnn (onnx) ONNX suport issues in DNN module labels Jan 20, 2022
@rogday rogday requested a review from sl-sergei January 20, 2022 14:01
alalek
alalek reviewed Jan 21, 2022
View reviewed changes
@rogday rogday marked this pull request as draft January 21, 2022 08:25
@rogday rogday marked this pull request as ready for review January 21, 2022 09:33
@rogday rogday requested a review from alalek January 21, 2022 09:34
alalek
alalek reviewed Jan 21, 2022
View reviewed changes
modules/dnn/src/onnx/onnx_importer.cpp
{
opencv_onnx::NodeProto node_proto = node_proto_;
const std::string& layer_type = node_proto.op_type();
const std::string output_name = node_proto.output(0);
Copy link
Copy Markdown
Member

@alalek alalek Jan 21, 2022

Choose a reason for hiding this comment

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

Reference?

const std::string&

Copy link
Copy Markdown
Member Author

@rogday rogday Jan 21, 2022

Choose a reason for hiding this comment

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

node_proto's output name is modified below.

alalek
alalek approved these changes Jan 21, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@alalek alalek 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 9188ce6 into opencv:3.4 Jan 26, 2022
@rogday rogday deleted the optional_outputs branch January 26, 2022 15:32
@alalek alalek mentioned this pull request Jan 28, 2022
Merged
@alalek
Copy link
Copy Markdown
Member

alalek commented Jan 29, 2022

It makes sense to test such non-trivial changes with 4.x branch too before merging them.

Currently we have broken output names with failed tests on 4.x branch:

[  FAILED  ] Objdetect_face_detection.regression
[  FAILED  ] Objdetect_face_recognition.regression

Outputs are also broken on 3.4 branch, but we don't have tests for them here.

@alalek alalek mentioned this pull request Jan 29, 2022
Merged
2 tasks
@alalek alalek mentioned this pull request Feb 22, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alalek alalek alalek approved these changes

@sl-sergei sl-sergei Awaiting requested review from sl-sergei

Assignees

@alalek alalek

Labels

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

Projects

None yet

Milestone

3.4.18

Development

Successfully merging this pull request may close these issues.

3 participants

@rogday @alalek @opencv-pushbot