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

docs: cleanups, clarify (+test) connected components #180

Merged
merged 11 commits into from
Feb 24, 2025
30 changes: 17 additions & 13 deletions src/view.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ pub trait PortView {
#[must_use]
fn port_offset(&self, port: impl Into<PortIndex>) -> Option<PortOffset>;

/// Returns the port index for a given node, direction, and offset.
/// Returns the port index for a given node and offset.
#[must_use]
fn port_index(&self, node: NodeIndex, offset: PortOffset) -> Option<PortIndex>;

Expand All @@ -51,7 +51,7 @@ pub trait PortView {

/// Iterates over all the input ports of the `node`.
///
/// Shorthand for [`PortView::ports`].
/// Shorthand for [`PortView::ports`]`(`[`Direction::Incoming`]`)`.
#[must_use]
#[inline]
fn inputs(&self, node: NodeIndex) -> impl Iterator<Item = PortIndex> + Clone {
Expand All @@ -60,7 +60,7 @@ pub trait PortView {

/// Iterates over all the output ports of the `node`.
///
/// Shorthand for [`PortView::ports`].
/// Shorthand for [`PortView::ports`]`(`[`Direction::Outgoing`]`)`.
#[must_use]
#[inline]
fn outputs(&self, node: NodeIndex) -> impl Iterator<Item = PortIndex> + Clone {
Expand All @@ -81,7 +81,7 @@ pub trait PortView {

/// Returns the number of input ports of the `node`.
///
/// Shorthand for [`PortView::num_ports`].
/// Shorthand for [`PortView::num_ports`]`(`[`Direction::Incoming`]`)`..
#[must_use]
#[inline]
fn num_inputs(&self, node: NodeIndex) -> usize {
Expand All @@ -90,7 +90,7 @@ pub trait PortView {

/// Returns the number of output ports of the `node`.
///
/// Shorthand for [`PortView::num_ports`].
/// Shorthand for [`PortView::num_ports`]`(`[`Direction::Outgoing`]`)`..
#[must_use]
#[inline]
fn num_outputs(&self, node: NodeIndex) -> usize {
Expand Down Expand Up @@ -126,7 +126,7 @@ pub trait PortView {

/// Iterates over all the input port offsets of the `node`.
///
/// Shorthand for [`PortView::port_offsets`].
/// Shorthand for [`PortView::port_offsets`]`(`[`Direction::Incoming`]`)`.
#[must_use]
#[inline]
fn input_offsets(&self, node: NodeIndex) -> impl Iterator<Item = PortOffset> + Clone {
Expand All @@ -135,7 +135,7 @@ pub trait PortView {

/// Iterates over all the output port offsets of the `node`.
///
/// Shorthand for [`PortView::port_offsets`].
/// Shorthand for [`PortView::port_offsets`]`(`[`Direction::Outgoing`]`)`.
#[must_use]
#[inline]
fn output_offsets(&self, node: NodeIndex) -> impl Iterator<Item = PortOffset> + Clone {
Expand Down Expand Up @@ -283,7 +283,7 @@ pub trait LinkView: PortView {
type LinkEndpoint: Into<PortIndex> + Copy;

/// Returns an iterator over every pair of matching ports connecting `from`
/// with `to`.
/// to `to`, i.e. in that direction.
///
/// # Example
/// ```
Expand Down Expand Up @@ -350,7 +350,7 @@ pub trait LinkView: PortView {
self.get_connection(from, to).is_some()
}

/// Returns the port that the given `port` is linked to.
/// Returns the port (or ports if this is a [`MultiView`]) that the given `port` is linked to.
#[must_use]
fn port_links(
&self,
Expand Down Expand Up @@ -401,7 +401,7 @@ pub trait LinkView: PortView {
) -> impl Iterator<Item = (Self::LinkEndpoint, Self::LinkEndpoint)> + Clone;

/// Iterates over the connected input links of the `node`. Shorthand for
/// [`LinkView::links`].
/// [`LinkView::links`]`(`[`Direction::Incoming`]`)`.
#[must_use]
#[inline]
fn input_links(
Expand All @@ -412,7 +412,7 @@ pub trait LinkView: PortView {
}

/// Iterates over the connected output links of the `node`. Shorthand for
/// [`LinkView::links`].
/// [`LinkView::links`]`(`[`Direction::Outgoing`]`)`..
#[must_use]
#[inline]
fn output_links(
Expand Down Expand Up @@ -452,14 +452,18 @@ pub trait LinkView: PortView {
#[must_use]
fn all_neighbours(&self, node: NodeIndex) -> impl Iterator<Item = NodeIndex> + Clone;

/// Iterates over the input neighbours of the `node`. Shorthand for [`LinkView::neighbours`].
/// Iterates over the input neighbours of the `node`.
///
/// Shorthand for [`LinkView::neighbours`]`(`[`Direction::Incoming`]`)`.
#[must_use]
#[inline]
fn input_neighbours(&self, node: NodeIndex) -> impl Iterator<Item = NodeIndex> + Clone {
self.neighbours(node, Direction::Incoming)
}

/// Iterates over the output neighbours of the `node`. Shorthand for [`LinkView::neighbours`].
/// Iterates over the output neighbours of the `node`.
///
/// Shorthand for [`LinkView::neighbours`]`(`[`Direction::Outgoing`]`)`.
#[must_use]
#[inline]
fn output_neighbours(&self, node: NodeIndex) -> impl Iterator<Item = NodeIndex> + Clone {
Expand Down
64 changes: 60 additions & 4 deletions src/view/subgraph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,11 @@ use crate::{
/// wall. The [Direction] of edges (incoming/outgoing) defines which side of
/// the wall is inside, and which is outside.
///
/// If both incoming and outgoing boundary edges are empty, the subgraph is
/// taken to be the entire graph.
/// Note that if the graph contains multiple connected components, there may be
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reading this definition, it seems to be describing the behaviour of Subgraph::new_subgraph rather than the structure itself.
(It's missing the arbitrary extra nodes).

Should we move this there, or add the optional closed connected components here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, good point. I kept the first bit here (introduction to the concept, boundary does not distinguish between two possibilities) and moved the rest to new_subgraph

/// components none of whose edges are in the boundary. The definition above is
/// consistent with such a component being either in or outside the subgraph.
/// The [Self::new_subgraph] constructor offers only limited flexibility in
/// defining such subgraphs and [Self::with_nodes] may need to be used instead.
///
/// If an invalid subgraph is defined, then behaviour is undefined.
///
Expand All @@ -67,6 +70,13 @@ impl<G: LinkView> Subgraph<G> {
/// and outgoing ports are outgoing boundary edges.
///
/// This initialisation is linear in the size of the subgraph.
///
/// If both incoming and outgoing boundary edges are empty, the subgraph is taken
/// to be the entire graph (i.e. all components); otherwise, the subgraph
/// contains only (parts of) those components mentioned in the boundary.
/// (Specifically, where the boundary includes at least one edge, or disconnected
/// port. To create a subgraph containing the whole of a component without any
/// disconnected ports, use [`Self::with_nodes`].)
pub fn new_subgraph(graph: G, boundary: Boundary) -> Self {
let nodes = boundary.internal_nodes(&graph).collect();
Self {
Expand All @@ -78,8 +88,8 @@ impl<G: LinkView> Subgraph<G> {

/// Create a new subgraph of `graph` containing only the given nodes.
///
/// The boundary of the subgraph is defined by all ports of the given nodes,
/// in the order they are given.
/// The boundary of the subgraph is defined by all ports of the given nodes
/// that have edges to other (i.e. external) nodes, in the order `nodes` are given.
///
/// See [`Subgraph::new_subgraph`] for a method that takes an explicit port boundary.
pub fn with_nodes(graph: G, nodes: impl IntoIterator<Item = NodeIndex>) -> Self {
Expand Down Expand Up @@ -707,6 +717,52 @@ mod tests {
assert!(!subg.is_convex());
}

#[test]
fn test_disconnected_components() {
let mut graph = PortGraph::new();
let n0 = graph.add_node(0, 1);
let n1 = graph.add_node(1, 0);
graph.link_nodes(n0, 0, n1, 0).unwrap();
let n2 = graph.add_node(0, 1);
let n3 = graph.add_node(1, 1);
graph.link_nodes(n2, 0, n3, 0).unwrap();

let subg_nodes = |b| Subgraph::new_subgraph(&graph, b).nodes_iter().collect_vec();

// No edges -> all components
let b = Boundary::from_ports(&graph, []);
let nodes = subg_nodes(b.clone());
assert_eq!(nodes, [n0, n1, n2, n3]);
assert_eq!(Subgraph::with_nodes(&graph, nodes).boundary, b);

// Edge in only one component -> just (part of) that component
let b = Boundary::from_ports(&graph, [graph.output(n0, 0).unwrap()]);
let nodes = subg_nodes(b.clone());
assert_eq!(nodes, [n0]);
assert_eq!(Subgraph::with_nodes(&graph, nodes).boundary, b);

// Edges in two components -> relevant parts of each component
let b = Boundary::from_ports(
&graph,
[graph.output(n0, 0).unwrap(), graph.input(n3, 0).unwrap()],
);
let nodes = subg_nodes(b.clone());
assert_eq!(nodes, [n0, n3]);
assert_eq!(Subgraph::with_nodes(&graph, nodes).boundary, b);

// Disconnected port -> includes whole component:
let b = Boundary::from_ports(&graph, [graph.output(n3, 0).unwrap()]);
assert_eq!(subg_nodes(b), [n2, n3]);
// However the graph with those nodes includes the unconnected port as interior,
// so has an empty boundary:
assert_eq!(
Subgraph::with_nodes(&graph, [n2, n3]).boundary,
Boundary::from_ports(&graph, [])
);
// (The subgraph cannot be recreated from said boundary,
// see https://github.com/CQCL/portgraph/issues/179.)
}

#[test]
fn test_copy_in_parent() {
let mut graph = PortGraph::new();
Expand Down
Loading