ONNX diagnostic tool corrections#19693
Conversation
|
|
||
| CV_Assert(!model.empty()); | ||
|
|
||
| setDiagnosticsRun(true); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| //! Unregisters registered layer with specified type name. Thread-safe. | ||
| static void unregisterLayer(const String &type); | ||
|
|
||
| static const std::map<String, int>& getRegisteredLayers() { return registeredTypes; } |
There was a problem hiding this comment.
std::map<String, int>
Why do we need std::map here?
Can we use simple std::vector<std::string> here?
There was a problem hiding this comment.
Removed this map, reused already existing LayerFactory_Impl map (moved it into private header), it has the same content.
ccddedb to
07d7fc8
Compare
| static void unregisterLayer(const String &type); | ||
|
|
||
| //! Get the types of layers which were registered. | ||
| static const std::map<String, int>& getRegisteredLayers(); |
There was a problem hiding this comment.
std::map<String, int>
Why do we need std::map in public API? Use std::vector instead.
String
Use std::string in new API.
If you need to optimize access with O(log(N)) instead O(N), then build std::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.
There was a problem hiding this comment.
@alalek, corrected, reused already existing LayerFactory_Impl.
|
|
||
| private: | ||
| LayerFactory(); | ||
| static std::map<String, int>& _getRegisteredLayers(); |
There was a problem hiding this comment.
Remove that from public headers.
All "private" members should be eliminated from public header, see https://github.com/opencv/opencv/wiki/Coding_Style_Guide
d81f111 to
4144e70
Compare
|
@alalek, corrected code, required tests passed. |
4144e70 to
b8f7828
Compare
|
Please update changelog highlights too: https://github.com/opencv/opencv/wiki/ChangeLog#version452 |
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 corrections Co-authored-by: Sergey Slashchinin <sergei.slashchinin@xperience.ai>
Relates to: PR #19420
Patch to opencv_extra has the same branch name.