Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use the same syntax to replace an node input (2) + optimize replace_all_inputs #1060
Use the same syntax to replace an node input (2) + optimize replace_all_inputs #1060
Changes from all commits
469b4c4
bae92a6
74321f3
d972ace
d9ea76a
7b244e0
f5c9406
3db62e4
03e2bb7
085d6fa
cd0e711
c491bc9
b4a73be
eb68bcd
0adae73
71b0ddb
874e5c5
1250847
c9aa7ab
d130591
e1eccab
1daa780
0b469c0
6a54003
bb4d9a6
5115124
094ab85
89ad32a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this isn't technically correct if a node has two copies of the same input (node.input = [inp1, inp1]) and you replace just one copy (replace_input(node, inp1, inp2, 0) -> node.input = [inp2, inp1]. node is still a consumer of inp1. This bug should only occur if input_index is not None since otherwise we replace everything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also why not use the _unregister helper function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right. I did not do it because sometimes replace_input is followed by a call to remove_input, sometimes not. So I did not do it. That means
_input_to_node_name keeps[input_name]
contains nodes not using the input anymore. This is not an issue as_input_to_node_name keeps[input_name]
to retrieve all nodes usinginput_name
. It was previously done with get_nodes(), even if the new set is bigger than necessary, it is still smaller than get_nodes(). I need to review all calls to replace_input and remove_input to be thorough to call _unregister.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you use unregister here as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same reason as above, a change here means other changes to be consistent.