-
Notifications
You must be signed in to change notification settings - Fork 8
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
Implement SimpleReplace
#114
Conversation
Not to be merged since it assumes multiports, which are not yet reflected in the spec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but the portgraph manipulating code is not as easy as I'd hoped! Would be nice if we can make that more readable at all - perhaps by super-consistent naming.
src/hugr.rs
Outdated
r: SimpleReplacement, | ||
) -> Result<(), SimpleReplacementError> { | ||
// 1. Check the parent node exists. | ||
if self.nodes().all(|node| node != r.p) || self.get_optype(r.p).tag() != OpTag::Dfg { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is accessing the nodes using self as a HugrView, right? Nice as that might be, this makes the rewrite O(n) in the size of the whole Hugr, not the fragment being replaced, which seems bad. I think from here (inside impl Hugr
) we can access self.graph
directly (or perhaps self.hierarchy
, as you do below), which could reduce this to O(1).
src/hugr.rs
Outdated
let n_node = Node { index: *n_index }; | ||
let op: &OpType = r.n.get_optype(n_node); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let n_node = Node { index: *n_index }; | |
let op: &OpType = r.n.get_optype(n_node); | |
let op: &OpType = r.n.get_optype((*n_index).into()); |
let op: &OpType = r.n.get_optype(n_node); | ||
let sig = op.signature(); | ||
if !sig.const_input.is_empty() { | ||
return Err(SimpleReplacementError::InvalidReplacementNode()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does mean the HUGR has been mutated i.e. we are midway through the replacement. So, while I admit it would probably involve recomputing op
for each node, how about doing this check in advance, e.g., using n_non_io_indices.all(|n_index|....)
src/hugr.rs
Outdated
let n_non_io_indices = &n_indices[1..n_sz - 1]; // omit Input and Output | ||
let self_input_node_index: NodeIndex = self.hierarchy.first(r.p.index).unwrap(); | ||
let n_output_node_index: NodeIndex = n_indices[n_sz - 1]; | ||
for n_index in n_non_io_indices { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmmm, I'm not 100% sure of my Rust here, but can you do
for &n_index in n_non_io_indices {
here and then avoid the *
before each n_index
in the loop?
src/hugr.rs
Outdated
let n_indices = | ||
r.n.hierarchy | ||
.children(n_dfg_node.index) | ||
.collect::<Vec<NodeIndex>>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, still looking at the conversion to Node done (at least twice) below, another way would be to do that here - I think .map_into
(from itertools
) would do it, or .into
or just collect::<Vec<Node>>
. That would mean you'd need to .index
in a few more places but might be neater overall.
src/replacement/simple_replace.rs
Outdated
|
||
const QB: SimpleType = SimpleType::Linear(LinearType::Qubit); | ||
|
||
/// Creates a hugr with a DFG. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we have rooted-at-any-op Hugrs, we might as well skip much of this as well, right? (Just to simplify the test)
src/replacement/simple_replace.rs
Outdated
let [qb0, qb1] = func_builder.input_wires_arr(); | ||
|
||
let mut inner_builder = | ||
func_builder.dfg_builder(vec![(QB, qb0), (QB, qb1)], type_row![QB, QB])?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and ASCII art again
src/replacement/simple_replace.rs
Outdated
.unwrap(); | ||
let (n_node_h0, n_node_h1) = n.input_neighbours(n_node_cx).collect_tuple().unwrap(); | ||
// 4.2. Locate the ports we need to specify as "glue" in n | ||
let n_port_0 = n.node_ports(n_node_h0, Direction::Incoming).next().unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note as well as .next()
there is .exactly_one()
if you use itertools::Itertools
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I strongly suggest using more meaningful names in the implementation. Having to go to an internal definition in another module to parse what r.s
adds a lot of friction.
I think this now handles empty wires correctly. Note the updated spec. |
Some fixes needed following merge from latest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Result::ok
just casts the value to an Option
, discarding any error information. In most cases calling that before unwrap
will give less debug info.
- a map $\nu\_\textrm{inp}: \textrm{inp}\_H(T \setminus \\{\texttt{Input}\\}) \to \textrm{inp}\_{\Gamma}(S)$; | ||
- a map $\nu_\textrm{out}: \textrm{out}_{\Gamma}(S) \to \textrm{out}_H(T \setminus \\{\texttt{Output}\\})$. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the subindices are wrong here?
- a map $\nu\_\textrm{inp}: \textrm{inp}\_H(T \setminus \\{\texttt{Input}\\}) \to \textrm{inp}\_{\Gamma}(S)$; | |
- a map $\nu_\textrm{out}: \textrm{out}_{\Gamma}(S) \to \textrm{out}_H(T \setminus \\{\texttt{Output}\\})$. | |
- a map $\nu\_\textrm{inp}: \textrm{inp}\_{\Gamma}(T \setminus \\{\texttt{Input}\\}) \to \textrm{inp}\_H(S)$; | |
- a map $\nu_\textrm{out}: \textrm{out}_H(S) \to \textrm{out}_{\Gamma}(T \setminus \\{\texttt{Output}\\})$. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think they are correct? T is a set in H, and S is a set in Gamma.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just above it says:
Given a set
$S$ of nodes in a hugr$H$ , let:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see the confusion. But that is a different H (scoped to the definition). I will change the definition to use a different letter!
nu_inp, | ||
nu_out, | ||
}; | ||
h.apply_simple_replacement(r).ok(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
h.apply_simple_replacement(r).ok(); | |
h.apply_simple_replacement(r).unwrap(); |
nu_inp, | ||
nu_out, | ||
}; | ||
h.apply_simple_replacement(r).ok(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
h.apply_simple_replacement(r).ok(); | |
h.apply_simple_replacement(r).unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the idea to move apply_simple_replacement
to simple_replace.rs
once #119 is done?
The code could be moved over there now, so it's less noisy to change that afterwards.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I was going to wait until that was done, since there isn't really a framework for it yet.
Thanks, yes. It seems I can't change all instances of
Will change what I can. |
## 🤖 New release * `hugr-llvm`: 0.5.1 -> 0.6.0 (⚠️ API breaking changes) ###⚠️ `hugr-llvm` breaking changes ``` --- failure auto_trait_impl_removed: auto trait no longer implemented --- Description: A public type has stopped implementing one or more auto traits. This can break downstream code that depends on the traits being implemented. ref: https://doc.rust-lang.org/reference/special-types-and-traits.html#auto-traits impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.36.0/src/lints/auto_trait_impl_removed.ron Failed in: type TypeConverter is no longer UnwindSafe, in /tmp/.tmpGjujM0/hugr-llvm/src/types.rs:59 type TypeConverter is no longer RefUnwindSafe, in /tmp/.tmpGjujM0/hugr-llvm/src/types.rs:59 --- failure derive_trait_impl_removed: built-in derived trait no longer implemented --- Description: A public type has stopped deriving one or more traits. This can break downstream code that depends on those types implementing those traits. ref: https://doc.rust-lang.org/reference/attributes/derive.html#derive impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.36.0/src/lints/derive_trait_impl_removed.ron Failed in: type TypeConverter no longer derives Copy, in /tmp/.tmpGjujM0/hugr-llvm/src/types.rs:59 type TypeConverter no longer derives Clone, in /tmp/.tmpGjujM0/hugr-llvm/src/types.rs:59 type TypeConverter no longer derives Debug, in /tmp/.tmpGjujM0/hugr-llvm/src/types.rs:59 --- failure function_missing: pub fn removed or renamed --- Description: A publicly-visible function cannot be imported by its prior path. A `pub use` may have been removed, or the function itself may have been renamed or removed entirely. ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.36.0/src/lints/function_missing.ron Failed in: function hugr_llvm::custom::prelude::array::emit_array_op, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom/prelude/array.rs:51 function hugr_llvm::custom::int::add_int_extensions, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom/int.rs:226 function hugr_llvm::custom::rotation::add_rotation_extensions, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom/rotation.rs:196 function hugr_llvm::custom::prelude::add_prelude_extensions, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom/prelude.rs:295 function hugr_llvm::custom::float::add_float_extensions, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom/float.rs:186 function hugr_llvm::custom::conversions::add_conversions_extension, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom/conversions.rs:270 function hugr_llvm::custom::logic::add_logic_extensions, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom/logic.rs:106 function hugr_llvm::custom::prelude::add_default_prelude_extensions, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom/prelude.rs:304 --- failure inherent_method_missing: pub method removed or renamed --- Description: A publicly-visible method or associated fn is no longer available under its prior name. It may have been renamed or removed entirely. ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.36.0/src/lints/inherent_method_missing.ron Failed in: TypeConverter::new, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/types.rs:107 TypeConverter::iw_context, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/types.rs:112 CodegenExtsMap::add_float_extensions, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom/float.rs:192 CodegenExtsMap::add_int_extensions, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom/int.rs:234 CodegenExtsMap::add_logic_extensions, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom/logic.rs:113 CodegenExtsMap::add_default_prelude_extensions, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom/prelude.rs:311 CodegenExtsMap::add_prelude_extensions, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom/prelude.rs:317 CodegenExtsMap::add_rotation_extensions, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom/rotation.rs:201 CodegenExtsMap::new, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom.rs:88 CodegenExtsMap::add_cge, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom.rs:97 CodegenExtsMap::get, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom.rs:110 CodegenExtsMap::llvm_type, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom.rs:120 CodegenExtsMap::emit, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom.rs:130 CodegenExtsMap::load_constant, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom.rs:145 --- failure module_missing: pub module removed or renamed --- Description: A publicly-visible module cannot be imported by its prior path. A `pub use` may have been removed, or the module may have been renamed, removed, or made non-public. ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.36.0/src/lints/module_missing.ron Failed in: mod hugr_llvm::custom::conversions, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom/conversions.rs:1 mod hugr_llvm::custom::logic, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom/logic.rs:1 mod hugr_llvm::custom::int, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom/int.rs:1 mod hugr_llvm::custom::prelude, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom/prelude.rs:1 mod hugr_llvm::fat, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/fat.rs:1 mod hugr_llvm::custom::float, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom/float.rs:1 mod hugr_llvm::custom::rotation, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom/rotation.rs:1 mod hugr_llvm::custom::prelude::array, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom/prelude/array.rs:1 --- failure struct_missing: pub struct removed or renamed --- Description: A publicly-visible struct cannot be imported by its prior path. A `pub use` may have been removed, or the struct itself may have been renamed or removed entirely. ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.36.0/src/lints/struct_missing.ron Failed in: struct hugr_llvm::custom::logic::LogicCodegenExtension, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom/logic.rs:78 struct hugr_llvm::fat::FatNode, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/fat.rs:28 struct hugr_llvm::custom::int::IntOpsCodegenExtension, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom/int.rs:120 struct hugr_llvm::custom::prelude::PreludeCodegenExtension, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom/prelude.rs:174 struct hugr_llvm::custom::float::FloatTypesCodegenExtension, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom/float.rs:27 struct hugr_llvm::emit::NullEmitLlvm, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/emit.rs:52 struct hugr_llvm::custom::conversions::ConversionsCodegenExtension, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom/conversions.rs:244 struct hugr_llvm::custom::prelude::DefaultPreludeCodegen, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom/prelude.rs:169 struct hugr_llvm::custom::int::IntTypesCodegenExtension, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom/int.rs:150 struct hugr_llvm::custom::rotation::RotationCodegenExtension, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom/rotation.rs:30 --- failure trait_method_added: pub trait method added --- Description: A non-sealed public trait added a new method without a default implementation, which breaks downstream implementations of the trait ref: https://doc.rust-lang.org/cargo/reference/semver.html#trait-new-item-no-default impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.36.0/src/lints/trait_method_added.ron Failed in: trait method hugr_llvm::custom::CodegenExtension::add_extension in file /tmp/.tmpGjujM0/hugr-llvm/src/custom.rs:41 --- failure trait_method_missing: pub trait method removed or renamed --- Description: A trait method is no longer callable, and may have been renamed or removed entirely. ref: https://doc.rust-lang.org/cargo/reference/semver.html#major-any-change-to-trait-item-signatures impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.36.0/src/lints/trait_method_missing.ron Failed in: method extension of trait CodegenExtension, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom.rs:38 method supported_consts of trait CodegenExtension, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom.rs:44 method llvm_type of trait CodegenExtension, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom.rs:50 method emitter of trait CodegenExtension, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom.rs:58 method load_constant of trait CodegenExtension, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom.rs:68 --- failure trait_missing: pub trait removed or renamed --- Description: A publicly-visible trait cannot be imported by its prior path. A `pub use` may have been removed, or the trait itself may have been renamed or removed entirely. ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.36.0/src/lints/trait_missing.ron Failed in: trait hugr_llvm::emit::EmitOp, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/emit.rs:45 trait hugr_llvm::custom::prelude::PreludeCodegen, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/custom/prelude.rs:52 trait hugr_llvm::fat::FatExt, previously in file /tmp/.tmpu0X9jI/hugr-llvm/src/fat.rs:313 --- failure trait_no_longer_object_safe: trait no longer object safe --- Description: Trait is no longer object safe, which breaks `dyn Trait` usage. ref: https://doc.rust-lang.org/stable/reference/items/traits.html#object-safety impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.36.0/src/lints/trait_no_longer_object_safe.ron Failed in: trait CodegenExtension in file /tmp/.tmpGjujM0/hugr-llvm/src/custom.rs:38 ``` <details><summary><i><b>Changelog</b></i></summary><p> <blockquote> ## [0.6.0](CQCL/hugr-llvm@v0.5.1...v0.6.0) - 2024-10-21 ### Bug Fixes - Conversion operations having poison results ([#131](CQCL/hugr-llvm#131)) ### New Features - [**breaking**] Allow extension callbacks to have non-`'static` lifetimes ([#128](CQCL/hugr-llvm#128)) - [**breaking**] Support `tket2.rotation.from_halfturns_unchecked` ([#133](CQCL/hugr-llvm#133)) ### Refactor - [**breaking**] remove trait emit op ([#104](CQCL/hugr-llvm#104)) - [**breaking**] rework extensions interface ([#119](CQCL/hugr-llvm#119)) - [**breaking**] move packaged extensions from `crate::custom` to `crate::extension` ([#126](CQCL/hugr-llvm#126)) </blockquote> </p></details> --- This PR was generated with [release-plz](https://github.com/MarcoIeni/release-plz/).
This PR includes and implements #85 .
Since we don't yet have DFG-rooted HUGRs we use a workaround where the replacement graph is specified as a HUGR with a unique DFG node. Once #112 is merged we can remove this workaround.