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

feat: Split methods between HugrMut and HugrMutInternals #441

Merged
merged 12 commits into from
Aug 29, 2023
2 changes: 1 addition & 1 deletion src/builder/build_traits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ use super::{

use crate::Hugr;

use crate::hugr::HugrInternalsMut;
use crate::hugr::HugrMut;

/// Trait for HUGR container builders.
/// Containers are nodes that are parents of sibling graphs.
Expand Down
2 changes: 1 addition & 1 deletion src/builder/cfg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use crate::{ops::handle::NodeHandle, types::Type};

use crate::Node;
use crate::{
hugr::{HugrInternalsMut, NodeType},
hugr::{HugrMut, NodeType},
type_row, Hugr,
};

Expand Down
2 changes: 1 addition & 1 deletion src/builder/conditional.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ use super::{

use crate::Node;
use crate::{
hugr::{HugrInternalsMut, NodeType},
hugr::{HugrMut, NodeType},
Hugr,
};

Expand Down
2 changes: 1 addition & 1 deletion src/builder/dataflow.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use crate::types::{FunctionType, Signature};

use crate::extension::ExtensionSet;
use crate::Node;
use crate::{hugr::HugrInternalsMut, Hugr};
use crate::{hugr::HugrMut, Hugr};

/// Builder for a [`ops::DFG`] node.
#[derive(Debug, Clone, PartialEq)]
Expand Down
6 changes: 2 additions & 4 deletions src/builder/module.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use super::{
BuildError, Container,
};

use crate::hugr::hugrmut::sealed::HugrMutInternals;
use crate::{
hugr::{views::HugrView, ValidationError},
ops,
Expand All @@ -18,10 +19,7 @@ use crate::types::Signature;
use crate::Node;
use smol_str::SmolStr;

use crate::{
hugr::{HugrInternalsMut, NodeType},
Hugr,
};
use crate::{hugr::NodeType, Hugr};

/// Builder for a HUGR module.
#[derive(Debug, Clone, PartialEq)]
Expand Down
2 changes: 1 addition & 1 deletion src/extension/infer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -626,7 +626,7 @@ mod test {
use super::*;
use crate::builder::{BuildError, DFGBuilder, Dataflow, DataflowHugr};
use crate::extension::ExtensionSet;
use crate::hugr::HugrInternalsMut;
use crate::hugr::HugrMut;
use crate::hugr::{validate::ValidationError, Hugr, HugrView, NodeType};
use crate::ops::{self, dataflow::IOTrait, handle::NodeHandle};
use crate::type_row;
Expand Down
74 changes: 68 additions & 6 deletions src/hugr.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
//! The Hugr data structure, and its basic component handles.

mod hugrmut;
pub mod hugrmut;

pub mod rewrite;
pub mod serialize;
Expand All @@ -10,14 +10,14 @@ pub mod views;
use std::collections::{HashMap, VecDeque};
use std::iter;

pub(crate) use self::hugrmut::HugrInternalsMut;
pub(crate) use self::hugrmut::HugrMut;
pub use self::validate::ValidationError;

use derive_more::From;
pub use rewrite::{Rewrite, SimpleReplacement, SimpleReplacementError};

use portgraph::multiportgraph::MultiPortGraph;
use portgraph::{Hierarchy, PortMut, UnmanagedDenseMap};
use portgraph::{Hierarchy, NodeIndex, PortMut, UnmanagedDenseMap};
use thiserror::Error;

#[cfg(feature = "pyo3")]
Expand Down Expand Up @@ -238,12 +238,31 @@ impl Hugr {
}
}

/// Produce a canonical ordering of the nodes.
/// Add a node to the graph, with the default conversion from OpType to NodeType
pub(crate) fn add_op(&mut self, op: impl Into<OpType>) -> Node {
// TODO: Default to `NodeType::open_extensions` once we can infer extensions
self.add_node(NodeType::pure(op))
}

/// Add a node to the graph.
pub(crate) fn add_node(&mut self, nodetype: NodeType) -> Node {
let node = self
.graph
.add_node(nodetype.input_count(), nodetype.output_count());
self.op_types[node] = nodetype;
node.into()
}

/// Produce a canonical ordering of the descendant nodes of a root,
/// following the graph hierarchy.
///
/// This starts with the root, and then proceeds in BFS order through the
/// contained regions.
///
/// Used by [`HugrMut::canonicalize_nodes`] and the serialization code.
fn canonical_order(&self) -> impl Iterator<Item = Node> + '_ {
fn canonical_order(&self, root: Node) -> impl Iterator<Item = Node> + '_ {
// Generate a BFS-ordered list of nodes based on the hierarchy
let mut queue = VecDeque::from([self.root.into()]);
let mut queue = VecDeque::from([root]);
iter::from_fn(move || {
let node = queue.pop_front()?;
for child in self.children(node) {
Expand All @@ -252,6 +271,46 @@ impl Hugr {
Some(node)
})
}

/// Compact the nodes indices of the hugr to be contiguous, and order them as a breadth-first
/// traversal of the hierarchy.
///
/// The rekey function is called for each moved node with the old and new indices.
///
/// After this operation, a serialization and deserialization of the Hugr is guaranteed to
/// preserve the indices.
pub fn canonicalize_nodes(&mut self, mut rekey: impl FnMut(Node, Node)) {
aborgna-q marked this conversation as resolved.
Show resolved Hide resolved
// Generate the ordered list of nodes
let mut ordered = Vec::with_capacity(self.node_count());
let root = self.root();
ordered.extend(self.as_mut().canonical_order(root));

// Permute the nodes in the graph to match the order.
//
// Invariant: All the elements before `position` are in the correct place.
for position in 0..ordered.len() {
// Find the element's location. If it originally came from a previous position
// then it has been swapped somewhere else, so we follow the permutation chain.
let mut source: Node = ordered[position];
while position > source.index.index() {
source = ordered[source.index.index()];
}

let target: Node = NodeIndex::new(position).into();
if target != source {
self.graph.swap_nodes(target.index, source.index);
self.op_types.swap(target.index, source.index);
self.hierarchy.swap_nodes(target.index, source.index);
rekey(source, target);
}
}
self.root = NodeIndex::new(0);

// Finish by compacting the copy nodes.
// The operation nodes will be left in place.
// This step is not strictly necessary.
self.graph.compact_nodes(|_, _| {});
}
}

impl Port {
Expand Down Expand Up @@ -354,6 +413,9 @@ pub enum HugrError {
/// An error occurred while manipulating the hierarchy.
#[error("An error occurred while manipulating the hierarchy.")]
HierarchyError(#[from] portgraph::hierarchy::AttachError),
/// The node doesn't exist.
Copy link
Contributor

@acl-cqc acl-cqc Aug 23, 2023

Choose a reason for hiding this comment

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

I wonder about the utility of naming the single field within, but not too worried

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I prefer to use named fields when the order is not relevant (what does x.0 mean?). But I have no strong opinion on this.

Copy link
Contributor

Choose a reason for hiding this comment

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

If there are multiple fields of the same type (e.g. expected and actual) then very much agree that named fields is the way to go. But here I don't think the name adds anything beyond what the type says already, and I really can't think of much else (of any type) you could put in an InvalidNode... (a validity-reason, perhaps. So ok if you prefer it this way!)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I changed it into a tuple struct for consistency with the other errors.

#[error("Invalid node {0:?}.")]
InvalidNode(Node),
}

#[cfg(feature = "pyo3")]
Expand Down
Loading