Use createUniqueName() while adding nodes from a Graphview to another.
Context
The
bool GraphView::add(std::shared_ptr<GraphView> otherGraph,
bool includeLearnableParam = true);function does not check for duplicate node names before adding nodes from a given Graphview to another Graphview.
Modified files
-
aidge_core/src/graph/GraphView.cpp, to add a condition that checks for duplicate node names in the target Graphview and usescreateUniqueName()if a duplicate is found;
Merge request reports
Activity
added Fix 🔥🔥 label
assigned to @idealbuq
requested review from @cmoineau
Oops looks like you broke some unit test! You can try to replay them locally see dedicated wiki section: https://gitlab.eclipse.org/groups/eclipse/aidge/-/wikis/C++%20test
added 1 commit
- 673b7a3f - Corrected logic of creating unique names in the GraphView.add() method.
added 1 commit
- b8704b16 - Added actual pointer comparison while creating unique names in the GraphView.add() method.
added 4 commits
Toggle commit list684 685 node->addView(shared_from_this()); 685 686 mNodes.insert(node); 686 if (!(node->name()).empty()) 687 mNodeRegistry.insert(std::make_pair(node->name(), node)); 687 if (!(node->name()).empty()) { 688 // Check if a node with the same name exists in the registry 689 auto it = mNodeRegistry.find(node->name()); 690 if (it != mNodeRegistry.end()) { 691 // Found a node with the same name, check if it's the same node pointer 692 if (it->second.get() == node.get()) { 693 Log::debug("Node \"{}\" is already registered and is the same instance. Skipping insertion.\n", node->name()); 694 } else { 695 // Different instance with the same name – generate an unique name 696 std::string newName = node->createUniqueName(node->name()); 697 698 while (mNodeRegistry.find(newName) != mNodeRegistry.end()) { Is this code useful? At this point,
nodeis already added to the currentGraphView, hencecreateUniqueName()should not create a name conflicting with the node registry. Furthermore, adding iteratively "_1" suffix breaks withcreateUniqueName()naming logic and may lead to very long and inconsistent names!changed this line in version 6 of the diff
I built up on the logic followed in commit 76ccc34, but indeed after further testing I agree that this while-loop is not necessary here.
I see, but with your change I think you should remove what was done in 76ccc34, as code is redundant and it is more logical to deal with this issue in the method your are in, which is called by the other one.
added 13 commits
-
4d3daaff...578c0429 - 10 commits from branch
eclipse/aidge:dev - d8c7ca6c - Employ createUniqueName() in the add() function.
- a751db03 - Corrected logic of creating unique names in the GraphView.add() method.
- c35b4940 - Added actual pointer comparison while creating unique names in the GraphView.add() method.
Toggle commit list-
4d3daaff...578c0429 - 10 commits from branch
added 1 commit
- 16a70b56 - Removed unnecessary while-loop when checking for duplicate node names.
added 13 commits
-
16a70b56...19cd7d0f - 9 commits from branch
eclipse/aidge:dev - 6480c7ba - Employ createUniqueName() in the add() function.
- e547e7bc - Corrected logic of creating unique names in the GraphView.add() method.
- 3ee5e5f4 - Added actual pointer comparison while creating unique names in the GraphView.add() method.
- 35e3ff55 - Removed unnecessary while-loop when checking for duplicate node names.
Toggle commit list-
16a70b56...19cd7d0f - 9 commits from branch
added 1 commit
- 67b9e164 - Removed redudant code in GraphView's add() methods.
-
@idealbuq Is this ready to be merged?
added StatusReview Ready label
added 9 commits
-
67b9e164...a6a93847 - 4 commits from branch
eclipse/aidge:dev - 8e203946 - Employ createUniqueName() in the add() function.
- d38a7d11 - Corrected logic of creating unique names in the GraphView.add() method.
- 6716d3f4 - Added actual pointer comparison while creating unique names in the GraphView.add() method.
- f4f694c6 - Removed unnecessary while-loop when checking for duplicate node names.
- 5916d89e - Removed redudant code in GraphView's add() methods.
Toggle commit list-
67b9e164...a6a93847 - 4 commits from branch
enabled an automatic merge when all merge checks for 5916d89e pass
