diff --git a/.github/pre-commit b/.github/pre-commit index d769356..dc7eb19 100755 --- a/.github/pre-commit +++ b/.github/pre-commit @@ -9,22 +9,35 @@ if [[ "${IGNORE_RUSTHOOKS:=0}" -ne 0 ]]; then exit 0 fi -if ! cargo fmt -- --check +if ! cargo fmt --all -- --check then echo "There are some code style issues." echo "Run cargo fmt first." exit 1 fi +if ! cargo check --all --all-features --workspace +then + echo "There are some compilation warnings." + exit 1 +fi + +if ! cargo test --all-features --workspace +then + echo "There are some test issues." + exit 1 +fi + if ! cargo clippy --all-targets --all-features --workspace -- -D warnings then echo "There are some clippy issues." exit 1 fi -if ! cargo test --all-features +RUSTDOCFLAGS="-Dwarnings" +if ! cargo doc --no-deps --all-features --workspace then - echo "There are some test issues." + echo "There are some clippy issues." exit 1 fi diff --git a/justfile b/justfile index bc395e9..1c2b9f8 100644 --- a/justfile +++ b/justfile @@ -8,7 +8,7 @@ test: # Auto-fix all clippy warnings fix: - cargo clippy --all-targets --all-features --workspace --fix --allow-staged + cargo clippy --all-targets --all-features --workspace --fix --allow-staged --allow-dirty # Run the pre-commit checks check: diff --git a/src/multiportgraph.rs b/src/multiportgraph.rs index ced5916..17e7c67 100644 --- a/src/multiportgraph.rs +++ b/src/multiportgraph.rs @@ -344,22 +344,23 @@ impl LinkView for MultiPortGraph { #[inline] fn links(&self, node: NodeIndex, direction: Direction) -> Self::NodeLinks<'_> { - NodeLinks::new(self, self.ports(node, direction)) + NodeLinks::new(self, self.ports(node, direction), 0..0) } #[inline] fn all_links(&self, node: NodeIndex) -> Self::NodeLinks<'_> { - NodeLinks::new(self, self.all_ports(node)) + let output_ports = self.graph.node_outgoing_ports(node); + NodeLinks::new(self, self.all_ports(node), output_ports) } #[inline] fn neighbours(&self, node: NodeIndex, direction: Direction) -> Self::Neighbours<'_> { - Neighbours::new(self, self.subports(node, direction)) + Neighbours::new(self, self.subports(node, direction), node, false) } #[inline] fn all_neighbours(&self, node: NodeIndex) -> Self::Neighbours<'_> { - Neighbours::new(self, self.all_subports(node)) + Neighbours::new(self, self.all_subports(node), node, true) } #[inline] @@ -811,6 +812,7 @@ pub mod test { let mut g = MultiPortGraph::new(); let node0 = g.add_node(1, 2); let node1 = g.add_node(2, 1); + let node0_input0 = g.input(node0, 0).unwrap(); let (node0_output0, node0_output1) = g.outputs(node0).collect_tuple().unwrap(); let (node1_input0, node1_input1) = g.inputs(node1).collect_tuple().unwrap(); @@ -866,5 +868,100 @@ pub mod test { ); assert_eq!(g.all_neighbours(node0).collect_vec(), [node1, node1, node1]); assert_eq!(g.port_links(node0_output0).collect_vec(), links[0..2]); + + // Self-link + // The `all_links` / `all_neighbours` iterators should only return these once. + g.link_nodes(node0, 0, node0, 0).unwrap(); + assert_eq!( + g.subport_outputs(node0).collect_vec(), + [ + SubportIndex::new_multi(node0_output0, 0), + SubportIndex::new_multi(node0_output0, 1), + SubportIndex::new_multi(node0_output0, 2), + SubportIndex::new_unique(node0_output1), + ] + ); + assert_eq!( + g.subport_inputs(node0).collect_vec(), + [SubportIndex::new_unique(node0_input0)] + ); + + let links = [ + ( + SubportIndex::new_multi(node0_output0, 0), + SubportIndex::new_unique(node1_input0), + ), + ( + SubportIndex::new_multi(node0_output0, 1), + SubportIndex::new_multi(node1_input1, 0), + ), + ( + SubportIndex::new_multi(node0_output0, 2), + SubportIndex::new_unique(node0_input0), + ), + ( + SubportIndex::new_unique(node0_output1), + SubportIndex::new_multi(node1_input1, 1), + ), + ]; + assert_eq!( + g.input_links(node0).collect_vec(), + [( + SubportIndex::new_unique(node0_input0), + SubportIndex::new_multi(node0_output0, 2), + )] + ); + assert_eq!(g.output_links(node0).collect_vec(), links); + assert_eq!(g.all_links(node0).collect_vec(), links); + assert_eq!(g.input_neighbours(node0).collect_vec(), [node0]); + assert_eq!( + g.output_neighbours(node0).collect_vec(), + [node1, node1, node0, node1] + ); + assert_eq!( + g.all_neighbours(node0).collect_vec(), + [node1, node1, node0, node1] + ); + assert_eq!(g.port_links(node0_output0).collect_vec(), links[0..3]); + } + + #[test] + fn insert_graph() -> Result<(), Box> { + 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); + let node0g = g.add_node(1, 1); + let node1g = g.add_node(1, 1); + g.link_nodes(node0g, 0, node1g, 0)?; + + let mut h = PortGraph::new(); + 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)?; + + let map = g.insert_graph(&h)?; + assert_eq!(map.len(), 2); + + assert_eq!(g.node_count(), 6); + assert_eq!(g.link_count(), 4); + assert!(g.contains_node(map[&node0h])); + assert!(g.contains_node(map[&node1h])); + assert_eq!( + g.input_neighbours(map[&node0h]).collect_vec(), + vec![map[&node0h], map[&node1h]] + ); + assert_eq!( + g.output_neighbours(map[&node0h]).collect_vec(), + vec![map[&node1h], map[&node0h]] + ); + assert_eq!( + g.all_neighbours(map[&node0h]).collect_vec(), + vec![map[&node1h], map[&node1h], map[&node0h]] + ); + + Ok(()) } } diff --git a/src/multiportgraph/iter.rs b/src/multiportgraph/iter.rs index a8b5cc5..5ec5212 100644 --- a/src/multiportgraph/iter.rs +++ b/src/multiportgraph/iter.rs @@ -6,7 +6,7 @@ use std::ops::Range; use super::{MultiPortGraph, SubportIndex}; use crate::portgraph::{self, NodePorts}; -use crate::{LinkView, NodeIndex, PortIndex, PortOffset, PortView}; +use crate::{Direction, LinkView, NodeIndex, PortIndex, PortOffset, PortView}; /// Iterator over the nodes of a graph. #[derive(Clone)] @@ -127,14 +127,27 @@ pub struct Neighbours<'a> { multigraph: &'a MultiPortGraph, subports: NodeSubports<'a>, current_copy_node: Option, + /// The node for which the neighbours are being iterated. + node: NodeIndex, + /// Whether to ignore self-loops in the input -> output direction. + /// This is used to avoid counting self-loops twice when iterating both + /// input and output neighbours. + ignore_dupped_self_loops: bool, } impl<'a> Neighbours<'a> { - pub(super) fn new(multigraph: &'a MultiPortGraph, subports: NodeSubports<'a>) -> Self { + pub(super) fn new( + multigraph: &'a MultiPortGraph, + subports: NodeSubports<'a>, + node: NodeIndex, + ignore_dupped_self_loops: bool, + ) -> Self { Self { multigraph, subports, current_copy_node: None, + node, + ignore_dupped_self_loops, } } } @@ -143,26 +156,42 @@ impl<'a> Iterator for Neighbours<'a> { type Item = NodeIndex; fn next(&mut self) -> Option { - let link = self.subports.find_map(|subport| { - let port_index = subport.port(); - if !self.multigraph.is_multiport(port_index) { - self.multigraph.graph.port_link(port_index) - } else { - // There is a copy node - if subport.offset() == 0 { - self.current_copy_node = self.multigraph.get_copy_node(port_index); + loop { + let link = self.subports.find_map(|subport| { + let port_index = subport.port(); + if !self.multigraph.is_multiport(port_index) { + self.multigraph.graph.port_link(port_index) + } else { + // There is a copy node + if subport.offset() == 0 { + self.current_copy_node = self.multigraph.get_copy_node(port_index); + } + let copy_node = self + .current_copy_node + .expect("Copy node not connected to a multiport."); + let dir = self.multigraph.graph.port_direction(port_index).unwrap(); + let offset = PortOffset::new(dir, subport.offset()); + let subport_index = + self.multigraph.graph.port_index(copy_node, offset).unwrap(); + self.multigraph.graph.port_link(subport_index) } - let copy_node = self - .current_copy_node - .expect("Copy node not connected to a multiport."); - let dir = self.multigraph.graph.port_direction(port_index).unwrap(); - let offset = PortOffset::new(dir, subport.offset()); - let subport_index = self.multigraph.graph.port_index(copy_node, offset).unwrap(); - self.multigraph.graph.port_link(subport_index) + })?; + let link_subport = self.multigraph.get_subport_from_index(link).unwrap(); + let node = self + .multigraph + .graph + .port_node(link_subport.port()) + .unwrap(); + // Ignore self-loops in the input -> output direction. + if self.ignore_dupped_self_loops + && node == self.node + && self.multigraph.port_direction(link_subport.port()).unwrap() + == Direction::Outgoing + { + continue; } - })?; - let link_subport = self.multigraph.get_subport_from_index(link).unwrap(); - self.multigraph.graph.port_node(link_subport.port()) + return Some(node); + } } } @@ -178,14 +207,22 @@ pub struct NodeLinks<'a> { multigraph: &'a MultiPortGraph, ports: NodePorts, current_links: Option>, + /// Ignore links to the given target ports. + /// This is used to avoid counting self-loops twice. + ignore_target_ports: Range, } impl<'a> NodeLinks<'a> { - pub(super) fn new(multigraph: &'a MultiPortGraph, ports: NodePorts) -> Self { + pub(super) fn new( + multigraph: &'a MultiPortGraph, + ports: NodePorts, + ignore_target_ports: Range, + ) -> Self { Self { multigraph, ports, current_links: None, + ignore_target_ports, } } } @@ -196,14 +233,20 @@ impl<'a> Iterator for NodeLinks<'a> { fn next(&mut self) -> Option { loop { - if let Some(links) = &mut self.current_links { - if let Some(link) = links.next() { - return Some(link); - } + let Some(links) = &mut self.current_links else { + let port = self.ports.next()?; + self.current_links = Some(PortLinks::new(self.multigraph, port)); + continue; + }; + let Some((from, to)) = links.next() else { self.current_links = None; + continue; + }; + // Ignore self-loops in the input -> output direction. + if self.ignore_target_ports.contains(&to.port().index()) { + continue; } - let port = self.ports.next()?; - self.current_links = Some(PortLinks::new(self.multigraph, port)); + return Some((from, to)); } } } diff --git a/src/portgraph.rs b/src/portgraph.rs index 468d0d2..6836747 100644 --- a/src/portgraph.rs +++ b/src/portgraph.rs @@ -304,6 +304,14 @@ impl PortGraph { self.port_count -= node_meta.incoming() as usize + node_meta.outgoing() as usize; self.port_count += incoming + outgoing; } + + /// Returns the range of outgoing port indices for a given node. + pub(crate) fn node_outgoing_ports(&self, node: NodeIndex) -> Range { + match self.node_meta_valid(node) { + Some(node_meta) => node_meta.outgoing_ports(), + None => 0..0, + } + } } impl PortView for PortGraph { @@ -791,18 +799,24 @@ impl LinkView for PortGraph { fn links(&self, node: NodeIndex, direction: Direction) -> Self::NodeLinks<'_> { let Some(node_meta) = self.node_meta_valid(node) else { - return NodeLinks::new(self.ports(node, direction), &[]); + return NodeLinks::new(self.ports(node, direction), &[], 0..0); }; let indices = node_meta.ports(direction); - NodeLinks::new(self.ports(node, direction), &self.port_link[indices]) + NodeLinks::new(self.ports(node, direction), &self.port_link[indices], 0..0) } fn all_links(&self, node: NodeIndex) -> Self::NodeLinks<'_> { let Some(node_meta) = self.node_meta_valid(node) else { - return NodeLinks::new(self.all_ports(node), &[]); + return NodeLinks::new(self.all_ports(node), &[], 0..0); }; let indices = node_meta.all_ports(); - NodeLinks::new(self.all_ports(node), &self.port_link[indices]) + // Ignore links where the target is one of the node's output ports. + // This way we only count self-links once. + NodeLinks::new( + self.all_ports(node), + &self.port_link[indices], + node_meta.outgoing_ports(), + ) } #[inline] @@ -1240,6 +1254,7 @@ pub mod test { #[cfg(feature = "serde")] #[cfg(feature = "proptest")] use crate::proptest::gen_portgraph; + use itertools::Itertools; #[cfg(feature = "proptest")] use proptest::prelude::*; @@ -1397,10 +1412,12 @@ pub mod test { #[test] fn link_iterators() { let mut g = PortGraph::new(); - let node0 = g.add_node(1, 2); + let node0 = g.add_node(1, 3); let node1 = g.add_node(2, 1); + let node0_input0 = g.input(node0, 0).unwrap(); let node0_output0 = g.output(node0, 0).unwrap(); let node0_output1 = g.output(node0, 1).unwrap(); + let node0_output2 = g.output(node0, 2).unwrap(); let node1_input0 = g.input(node1, 0).unwrap(); let node1_input1 = g.input(node1, 1).unwrap(); @@ -1432,6 +1449,25 @@ pub mod test { assert!(g.input_neighbours(node0).eq([])); assert!(g.output_neighbours(node0).eq([node1, node1])); assert!(g.all_neighbours(node0).eq([node1, node1])); + + // Self-link + g.link_nodes(node0, 2, node0, 0).unwrap(); + + assert!(g.input_links(node0).eq([(node0_input0, node0_output2)])); + assert!(g.output_links(node0).eq([ + (node0_output0, node1_input0), + (node0_output1, node1_input1), + (node0_output2, node0_input0) + ])); + // The self-link should only appear once in the all_links iterator + assert!(g.all_links(node0).eq([ + (node0_output0, node1_input0), + (node0_output1, node1_input1), + (node0_output2, node0_input0) + ])); + assert!(g.input_neighbours(node0).eq([node0])); + assert!(g.output_neighbours(node0).eq([node1, node1, node0])); + assert!(g.all_neighbours(node0).eq([node1, node1, node0])); } #[test] @@ -1616,11 +1652,11 @@ 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)?; let map = g.insert_graph(&h)?; assert_eq!(map.len(), 2); @@ -1629,10 +1665,18 @@ pub mod test { 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]])); + assert_eq!( + g.input_neighbours(map[&node0h]).collect_vec(), + [map[&node0h], map[&node1h]] + ); + assert_eq!( + g.output_neighbours(map[&node0h]).collect_vec(), + [map[&node1h], map[&node0h]] + ); + assert_eq!( + g.all_neighbours(map[&node0h]).collect_vec(), + [map[&node1h], map[&node1h], map[&node0h]] + ); Ok(()) } diff --git a/src/portgraph/iter.rs b/src/portgraph/iter.rs index 696de7d..663e780 100644 --- a/src/portgraph/iter.rs +++ b/src/portgraph/iter.rs @@ -240,13 +240,21 @@ impl FusedIterator for NodePortOffsets {} #[derive(Clone, Debug)] pub struct NodeLinks<'a> { links: Zip>>, + /// Ignore links with target ports in the given range. + /// This is used to filter out duplicated self-links. + ignore_target_ports: Range, } impl<'a> NodeLinks<'a> { /// Returns a new iterator - pub(super) fn new(ports: NodePorts, links: &'a [Option]) -> Self { + pub(super) fn new( + ports: NodePorts, + links: &'a [Option], + ignore_target_ports: Range, + ) -> Self { Self { links: ports.zip(links.iter()), + ignore_target_ports, } } } @@ -258,9 +266,13 @@ impl<'a> Iterator for NodeLinks<'a> { fn next(&mut self) -> Option { loop { let (port, link) = self.links.next()?; - if let Some(link) = link { - return Some((port, *link)); + let Some(link) = link else { + continue; + }; + if self.ignore_target_ports.contains(&link.index()) { + continue; } + return Some((port, *link)); } }