[add] merge two onnx and related #131
Conversation
|
Thanks for your effort! I am reviewing the codes. This may take some time as this PR contains many modifications. |
There was a problem hiding this comment.
Thanks for your contribution. I have reviewed the codes and left some suggestions/confusing points. Please check. In addition, this patch is named with "merge two onnx and related" but ships with features including including but not limited to: 1) merge models; 2) copy nodes; 3) other minor modifications. It is suggested to break them into separate patches. Thanks for your effort again!
static/onnx.js
Outdated
| if (!map.has(item.name)) { | ||
| map.set(item.name, []); | ||
| } | ||
| map.get(item.name).push(item); | ||
| } | ||
|
|
||
| // reorder the map |
There was a problem hiding this comment.
When adding a node, convolutional nodes and several other types are more commonly used. It is more convenient to have them displayed at the beginning of the node list.
onnx_modifier.py
Outdated
| logging.info("load done!") | ||
| return cls(name, model_proto) | ||
|
|
||
| @classmethod |
There was a problem hiding this comment.
To merge two models, it is expected to connect some of the outputs from the first model with inputs from the second model. However, this logic is not implemented in the merge() function.
reference: https://github.com/onnx/onnx/blob/main/docs/PythonAPIOverview.md#onnx-compose
There was a problem hiding this comment.
Well, I have considered this issue, but the problem encountered by the person who proposed this issue is just one of many requirements involving merging two models. We may encounter different needs such as placing the main structures of two models side by side with shared inputs and outputs; or we might need to replace a part of an existing model with a portion of another model, and so on. Therefore, considering comprehensively, I merely put the two models together. As for how to proceed next, adjustments will still need to be made based on specific scenarios and cannot be achieved all at once. This is a trade-off I made based on minimizing code changes.
In fact, a more elegant solution would be to support cross-file module copy and paste functionality, but that might require a significant amount of time to implement, so it has not been realized.
There was a problem hiding this comment.
Yes, merging models can involve many cases. I think it can be a user-driven feature, and we support the cases that users call for at a higher priority, such as connecting models with one's tail and another's head.
README_zh-CN.md
Outdated
| @@ -182,6 +182,19 @@ | |||
|
|
|||
| `onnx-modifer`正在活跃地更新中:hammer_and_wrench:。 欢迎使用，提issue，如果有帮助的话，感谢给个:star:~ | |||
|
|
|||
| ## 合并两个onnx模型 | |||
There was a problem hiding this comment.
Please move these sections ahead of "onnx-modifer正在活跃地更新中...".
README_zh-CN.md
Outdated
| > 1. 只能复制已知节点，未知节点需要先修改`metadata.json`,即先变为已知节点。 | ||
| > 2. 调试模式下按下Alt，进入断点并松开Alt，退出调试后，Alt会被认为一直是按下状态。需要重新按一次解除。 | ||
|
|
||
| ## 复制或删除多个节点 |
There was a problem hiding this comment.
Deleting nodes is already supported. What is the motivation of "deleting nodes" here?
There was a problem hiding this comment.
Umm, my description here might not be very precise. What I have implemented is the ability to select two nodes as the start and end points, thereby selecting all consecutive nodes between them, and supporting deletion and duplication of these selected nodes. This feature facilitates adjustments to the model structure. Previously, the deletion function was limited to removing a single node or all subsequent nodes, so there is a little difference in functionality.
There was a problem hiding this comment.
Got it. It is suggested to split this feature into a separate patch. To be frank, I am not sure whether it is a commonly used feature.
static/view-grapher.js
Outdated
|
|
||
| getPathNodeNames(start, end) | ||
| { | ||
| var twoDimensionalArray = new Set(); |
There was a problem hiding this comment.
A more meaningful variable name is expected.
There was a problem hiding this comment.
You are absolutely right. This is the most challenging task, and I need to think it over carefully.
|
Thank you for your reply. I have responded to your code review and explained some of my design thoughts. I am not sure if my approach is appropriate. I hope to receive your feedback so that I can make the necessary adjustments or modifications. |
xiaominghan121
force-pushed
the
master
branch
from
18ed1bb to
067010f
Compare
|
按照 #131 (review) 建议将各个功能分离成不同提交，新的PR已提交 #135 |
提供合并两个ONNX模型的功能，同时支持多个节点复制和删除功能。
可以满足
https://github.com/ZhangGe6/onnx-modifier/issues/63需求。