[tools/onnx-subgraph] add onnx proto wrapper for graph parsing#14992
[tools/onnx-subgraph] add onnx proto wrapper for graph parsing#14992seanshpark merged 4 commits intoSamsung:masterfrom
Conversation
add onnx proto wrapper for graph parsing ONE-DCO-1.0-Signed-off-by: Youxin Chen <[email protected]>
chenyx113
force-pushed
the
onnx-subgraph-0328-2
branch
from
9aef03b to
a0825de
Compare
| { | ||
| template <> struct hash<NodeTensor> | ||
| { |
There was a problem hiding this comment.
| { | |
| template <> struct hash<NodeTensor> | |
| { | |
| { | |
| template <> struct hash<NodeTensor> | |
| { |
tools/onnx_subgraph/include/graph.h
Outdated
| }; | ||
| #endif |
There was a problem hiding this comment.
| }; | |
| #endif | |
| }; | |
| #endif // GRAPH_H |
| // | ||
| // WARNING: This file is automatically generated! Please edit onnx.in.proto. | ||
| // |
There was a problem hiding this comment.
1/ it would be better to generate at configure or compile time instead from onnx.in.proto instead of adding this generated file in source tree.
2/ I don't see onnx.in.proto file in this PR. how are you going to mange change for this file?
There was a problem hiding this comment.
I use the onnx.proto file from the official github directly, and I will add auto downloading file in future code, thanks for your suggestion.
| */ | ||
| #include <iostream> | ||
| #include <string> | ||
| #include "graph.h" |
There was a problem hiding this comment.
plz move this addition above #include <iostream> line like
+ #include "graph.h"
+
#include <iostream>
tools/onnx_subgraph/CMakeLists.txt
Outdated
| include_directories(${CMAKE_CURRENT_SOURCE_DIR}/include) | ||
| include_directories(${Python3_INCLUDE_DIRS}) | ||
|
|
||
| file(GLOB SOURCES "src/lib/*.cpp" "src/lib/*.cpp" ) |
There was a problem hiding this comment.
src/lib/*.cpp is added twice. is there particular reason?
tools/onnx_subgraph/CMakeLists.txt
Outdated
|
|
||
| add_executable(onnx-subgraph src/main.cpp) | ||
| target_link_libraries(onnx-subgraph ${Python3_LIBRARIES}) | ||
| target_link_libraries(onnx-subgraph onnx-subgraph-parser ${Python3_LIBRARIES}) |
There was a problem hiding this comment.
optional)
| target_link_libraries(onnx-subgraph onnx-subgraph-parser ${Python3_LIBRARIES}) | |
| target_link_libraries(onnx-subgraph onnx-subgraph-parser) | |
| target_link_libraries(onnx-subgraph ${Python3_LIBRARIES}) |
tools/onnx_subgraph/include/graph.h
Outdated
|
|
||
| } // namespace std | ||
|
|
||
| class Graph |
There was a problem hiding this comment.
Q) is there particular reason to use class? only single member exist and it's public, so it can be used with struct. what do you think?
There was a problem hiding this comment.
yes, it indeed doesn't need a class, just an API is ok, I have changed, thank you
tools/onnx_subgraph/include/graph.h
Outdated
| onnx::GraphProto GetGraphFromOnnx(std::string &path); | ||
| }; | ||
|
|
||
| struct graph_adjacency_node |
There was a problem hiding this comment.
Other struct uses pascal naming convention. is there particular reason to use snake case?
There was a problem hiding this comment.
I change naming convention for this struct, thank you
tools/onnx_subgraph/include/graph.h
Outdated
| int index; | ||
| }; | ||
|
|
||
| #endif |
There was a problem hiding this comment.
| #endif | |
| #endif // GRAPH_H |
2 participants
related issue: #14534
historical full changes: #14613
add onnx proto wrapper for graph parsing
ONE-DCO-1.0-Signed-off-by: Youxin Chen [email protected]