NotificationsYou must be signed in to change notification settings
Fork56.4k
Star85.3k

ONNX diagnostic tool corrections#19693

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

alalek merged 5 commits intoopencv:masterfromLupusSanctus:onnx_diagnostic

Mar 29, 2021

Merged

ONNX diagnostic tool corrections#19693

alalek merged 5 commits intoopencv:masterfromLupusSanctus:onnx_diagnostic

Mar 29, 2021

Conversation

Copy link

LupusSanctus commentedMar 8, 2021•
edited
Loading

Relates to: PR#19420

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 other license that is incompatible with OpenCV
The PR is proposed to proper branch
There is reference to 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

asmorkalov mentioned this pull request

Mar 9, 2021

WIP: Diagnostic run for ONNX DNN loader (Prototype)#19420

Closed

LupusSanctus changed the title~~WIP: ONNX diagnostic tool corrections~~ONNX diagnostic tool corrections

Mar 9, 2021

LupusSanctus requested review fromalalek andasmorkalov

March 11, 2021 10:05

vpisarev reviewed

Mar 12, 2021

View reviewed changes

apps/model-diagnostics/model_diagnostics.cpp Outdated


		CV_Assert(!model.empty());

		setDiagnosticsRun(true);

Copy link

Contributor

vpisarevMar 12, 2021

Choose a reason for hiding this comment

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

it's not good to alter DNN behaviour by setting a global variable. It would be better to have an overloaded variant of readNet() that will pass the corresponding flags to the importers w/o affecting global state.

Copy link

Member

alalekMar 12, 2021•
edited
Loading

Choose a reason for hiding this comment

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

I prefer adding global variable instead of exploding of public API.
API parameters should cover regular usage and don't confuse users.

It is some kind of logging utilities. Even in functional world nobody brings logger instance parameter into each function.

This approach allows to keep this patch small.

vpisarev reviewed

Mar 12, 2021

View reviewed changes

apps/model-diagnostics/model_diagnostics.cppShow resolvedHide resolved

alalek reviewed

Mar 12, 2021

View reviewed changes

modules/dnn/include/opencv2/dnn/dnn.hpp OutdatedShow resolvedHide resolved

modules/dnn/include/opencv2/dnn/layer.hpp Outdated

		//! Unregisters registered layer with specified type name. Thread-safe.
		staticvoidunregisterLayer(const String &type);

		staticconst std::map<String,int>&getRegisteredLayers() {return registeredTypes; }

Copy link

Member

alalekMar 12, 2021

Choose a reason for hiding this comment

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

std::map<String, int>

Why do we needstd::map here?
Can we use simplestd::vector<std::string> here?

Copy link

Author

LupusSanctusMar 23, 2021

Choose a reason for hiding this comment

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

Removed this map, reused already existingLayerFactory_Impl map (moved it into private header), it has the same content.

modules/dnn/include/opencv2/dnn/layer.hpp OutdatedShow resolvedHide resolved

modules/dnn/src/onnx/onnx_importer.cpp OutdatedShow resolvedHide resolved

LupusSanctus force-pushed theonnx_diagnostic branch fromccddedb to07d7fc8Compare

March 17, 2021 20:11

alalek reviewed

Mar 20, 2021

View reviewed changes

modules/dnn/include/opencv2/dnn/layer.hpp Outdated

		staticvoidunregisterLayer(const String &type);

		//! Get the types of layers which were registered.
		staticconst std::map<String,int>&getRegisteredLayers();

Copy link

Member

alalekMar 20, 2021

Choose a reason for hiding this comment

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

std::map<String, int>

Why do we needstd::map in public API? Usestd::vector instead.

String

Usestd::string in new API.

If you need to optimize access withO(log(N)) insteadO(N), then buildstd::set / hash_map / etc internally. Do not expose that to public API.

std::set does not work with OpenCV bindings, so it should be avoided in public API.

Copy link

Author

LupusSanctusMar 23, 2021•
edited
Loading

Choose a reason for hiding this comment

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

@alalek, corrected, reused already existingLayerFactory_Impl.

modules/dnn/include/opencv2/dnn/layer.hpp Outdated


		private:
		LayerFactory();
		static std::map<String,int>&_getRegisteredLayers();

Copy link

Member

alalekMar 20, 2021

Choose a reason for hiding this comment

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

Remove that from public headers.
All "private" members should be eliminated from public header, seehttps://github.com/opencv/opencv/wiki/Coding_Style_Guide

Copy link

Author

LupusSanctusMar 23, 2021

Choose a reason for hiding this comment

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

Corrected, addedlayer_reg.private.hpp.

LupusSanctus force-pushed theonnx_diagnostic branch 5 times, most recently fromd81f111 to4144e70Compare

March 26, 2021 17:11

Copy link

Author

LupusSanctus commentedMar 26, 2021

@alalek, corrected code, required tests passed.

asmorkalov reviewed

Mar 29, 2021

View reviewed changes

modules/dnn/include/opencv2/dnn/dnn.hppShow resolvedHide resolved

apps/model-diagnostics/model_diagnostics.cpp OutdatedShow resolvedHide resolved

sl-sergeiand others added5 commits

March 29, 2021 17:19

Final

7000708

Add forgotten Normalize layer to the set of supported types

d1c0e99

ONNX diagnostic tool corrections

0c99c94

Fixed CI test warnings

1226b2e

Added code minor corrections

b8f7828

LupusSanctus force-pushed theonnx_diagnostic branch from4144e70 tob8f7828Compare

March 29, 2021 14:20

asmorkalov self-requested a review

March 29, 2021 14:44

asmorkalov approved these changes

Mar 29, 2021

View reviewed changes

alalek assignedasmorkalov

Mar 29, 2021

Copy link

Member

alalek commentedMar 29, 2021

Please update changelog highlights too:https://github.com/opencv/opencv/wiki/ChangeLog#version452

alalek merged commite08de11 intoopencv:master

Mar 29, 2021

alalek mentioned this pull request

Apr 1, 2021

apps(model_diagnostics): fix invalid callback#19829

Merged

alalek mentioned this pull request

Apr 9, 2021

(5.x) Merge 4.x#19885

Merged

a-sajjad72 pushed a commit to a-sajjad72/opencv that referenced this pull request

Mar 30, 2023

Merge pull requestopencv#19693from LupusSanctus:onnx_diagnostic

cf91d0b

ONNX diagnostic tool* Final* Add forgotten Normalize layer to the set of supported types* ONNX diagnostic tool corrections* Fixed CI test warnings* Added code minor correctionsCo-authored-by: Sergey Slashchinin <sergei.slashchinin@xperience.ai>

Labels

category: apps

OpenCV builtin tools

category: dnn feature

Movatterモバイル変換

Uh oh!

ONNX diagnostic tool corrections#19693

ONNX diagnostic tool corrections#19693

Uh oh!

Conversation

LupusSanctus commentedMar 8, 2021• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

vpisarevMar 12, 2021

Choose a reason for hiding this comment

Uh oh!

alalekMar 12, 2021• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

alalekMar 12, 2021

Choose a reason for hiding this comment

Uh oh!

LupusSanctusMar 23, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

alalekMar 20, 2021

Choose a reason for hiding this comment

Uh oh!

LupusSanctusMar 23, 2021• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alalekMar 20, 2021

Choose a reason for hiding this comment

Uh oh!

LupusSanctusMar 23, 2021

Choose a reason for hiding this comment

Uh oh!

LupusSanctus commentedMar 26, 2021

Uh oh!

Uh oh!

Uh oh!

alalek commentedMar 29, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

LupusSanctus commentedMar 8, 2021•
edited
Loading

alalekMar 12, 2021•
edited
Loading

LupusSanctusMar 23, 2021•
edited
Loading