Skip to content
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

insert_graph fails with self-edges #130

Closed
acl-cqc opened this issue May 31, 2024 · 0 comments · Fixed by #132
Closed

insert_graph fails with self-edges #130

acl-cqc opened this issue May 31, 2024 · 0 comments · Fixed by #132
Assignees
Labels
bug Something isn't working

Comments

@acl-cqc
Copy link
Contributor

acl-cqc commented May 31, 2024

I applied the following diff to the insert_graph test in src/portgraph.rs (commit 71453e8 on branch main):

@@ -1616,12 +1616,13 @@ pub mod test {
         g.link_nodes(node0g, 0, node1g, 0)?;
 
         let mut h = PortGraph::new();
-        let node0h = h.add_node(1, 2);
-        let node1h = h.add_node(2, 1);
-        h.link_nodes(node0h, 0, node1h, 1)?;
-        h.link_nodes(node0h, 1, node1h, 0)?;
-        h.link_nodes(node1h, 0, node0h, 0)?;
+        let node0h = h.add_node(2, 2);
+        let node1h = h.add_node(1, 1);
+        h.link_nodes(node0h, 0, node1h, 0)?;
+        h.link_nodes(node0h, 1, node0h, 0)?;
+        h.link_nodes(node1h, 0, node0h, 1)?;
 
+        // The following fails with an AlreadyLinked:
         let map = g.insert_graph(&h)?;
         assert_eq!(map.len(), 2);

The edges are now a cycle from node0h back to itself as well as a cycle from node0h via node1h back to node0h. The error happens because in insert_graph all_links for node0h returns two views of the same edge (one from the outgoing port and one from the incoming port), both of which lead to trying to add the edge.

I also tried with MultiPortGraph using this diff on the above, and insert_graph succeeds but is adding two copies of the edge:

@@ -1607,7 +1607,7 @@ pub mod test {
 
     #[test]
     fn insert_graph() -> Result<(), Box<dyn std::error::Error>> {
-        let mut g = PortGraph::new();
+        let mut g = crate::MultiPortGraph::new();
         // Add dummy nodes to produce different node ids than in the other graph.
         g.add_node(0, 0);
         g.add_node(0, 0);
@@ -1622,18 +1622,26 @@ pub mod test {
         h.link_nodes(node0h, 1, node0h, 0)?;
         h.link_nodes(node1h, 0, node0h, 1)?;
 
-        // The following fails with an AlreadyLinked:
+        // This now succeeds as one would expect:
         let map = g.insert_graph(&h)?;
         assert_eq!(map.len(), 2);
 
         assert_eq!(g.node_count(), 6);
-        assert_eq!(g.link_count(), 4);
+        //This fails (link_count is 5):
+        //assert_eq!(g.link_count(), 4);
         assert!(g.contains_node(map[&node0h]));
         assert!(g.contains_node(map[&node1h]));
-        assert!(g.input_neighbours(map[&node0h]).eq([map[&node1h]]));
-        assert!(g
-            .output_neighbours(map[&node0h])
-            .eq([map[&node1h], map[&node1h]]));
+        // Modified the assert to give better debug output.
+        // The LHS now has three elements: map[&node0h] *twice* and map[&node1h]
+        assert_eq!(
+            g.input_neighbours(map[&node0h]).collect::<Vec<_>>(),
+            vec![map[&node0h], map[&node1h]]
+        );
+        // Similarly, here the LHS now has map[&node1h], map[&node0h], map[&node0h]
+        assert_eq!(
+            g.output_neighbours(map[&node0h]).collect::<Vec<_>>(),
+            vec![map[&node1h], map[&node0h]]
+        );
 
         Ok(())
     }
@acl-cqc acl-cqc added the bug Something isn't working label May 31, 2024
@aborgna-q aborgna-q self-assigned this May 31, 2024
github-merge-queue bot pushed a commit that referenced this issue Jun 3, 2024
The `all_links` iterator for both `Portgraph` and `Multiportgraph`, and
`all_neighbours` for the latter yielded self-loops twice.

This in turn caused `insert_graph` to try inserting repeated links, as
reported in #130.

This PR fixes the error and adds tests for that.

Fixes #130.

drive-by: Add missing checks in the pre-commit hook.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants