feat: Add KAK Commit Factory #954
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dan-commit-factory-base #954 +/- ##
==========================================================
Coverage ? 79.46%
==========================================================
Files ? 90
Lines ? 10892
Branches ? 10618
==========================================================
Hits ? 8655
Misses ? 1648
Partials ? 589
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Hey Dan!
The logic is great and I like the names (SearchingWalker, frontier etc). Most of my comments are about Rust or some inconsistencies.
General Rust code standards:
- make sure you build this and run your tests:
cargo test(will run all tket2 tests) orcargo test kak(will run the tests in this file); - I highly recommend you use
cargo fmtto format your file in the standard way. Some formatting conventions in rust get a second to get used to (esp. around iterators imo), but the consistency is very nice to have; - less important but good to know:
cargo clippyis the Rust linter. If you runcargo clippy --fixit will adjust your code to follow all best practices. Note that--fixwill not work if you have any files not commited in your repo. Either clean up your directory or usecargo clippy --fix --allow-dirty.
Let me know if you have any issues with making your code compile :) For clippy and fmt, you might have to install them first: rustup component add clippy and rustup component add rustfmt (assuming you installed Rust with rustup?).
You should be pretty close to a working solution and I look forward to testing it!
| walker: Walker <'w>, // Initial state of walker. | ||
| frontier_wires: Vec<PersistentWire>, // Wires to be expanded. | ||
| pattern: KAKResynthesis, // Current pattern. | ||
| two_qubit_node: Option<PatchNode>, // If a 2q gate has been found along one wire, | ||
| // but not yet the other, then it is stored here. |
There was a problem hiding this comment.
Put the documentation for the fields on a separate line above the fields, starting with ///. That way rust will automatically pick it up as docstrings (and include it in automatically generated documentation) :)
| walker: Walker <'w>, // Initial state of walker. | |
| frontier_wires: Vec<PersistentWire>, // Wires to be expanded. | |
| pattern: KAKResynthesis, // Current pattern. | |
| two_qubit_node: Option<PatchNode>, // If a 2q gate has been found along one wire, | |
| // but not yet the other, then it is stored here. | |
| /// Initial state of walker. | |
| walker: Walker <'w>, | |
| /// Wires to be expanded. | |
| frontier_wires: Vec<PersistentWire>, | |
| /// Current pattern. | |
| pattern: KAKResynthesis, | |
| /// If a 2q gate has been found along one wire, | |
| /// but not yet the other, then it is stored here. | |
| two_qubit_node: Option<PatchNode>, |
| for node in pers_hugr.nodes() { | ||
| let walker = Walker::from_pinned_node(node, pers_hugr.as_state_space()); | ||
| // walker.get_wire(node, OutgoingPort::from(0)); | ||
| factory.find_pattern_matches(node, walker); | ||
| } |
There was a problem hiding this comment.
Add an assertion to check this does what you expect.
The simplest would be to add a variable let mut n_matches = 0 before the loop, and then count the number of matches you find:
n_matches += factory.find_pattern_matches(node, walker).count();After the loop, you would write assert_eq(n_matches, 1).
A stronger test would involve looking into the matches obtained and checking it contains the gates you expect.
| #[test] | ||
| fn test_my_test() { | ||
| println!("hello world") | ||
| } |
| #[test] | ||
| fn test_kak_resynthesis_init() { |
There was a problem hiding this comment.
not important but useful to know:
if you replace #[test] with #[rstest] (you need to import it with use rstest::rstest) and you add #[fixture] (also imported from rstest) in front of your helper function one_cz_2qb_hugr, then you can write the test as
#[rstest]
fn test_kak_resynthesis_init(one_cz_2qb_hugr: Hugr) { ... }It's just syntactic sugar to move the construction of the Hugr from a function call in your function body as you have it now, to something that looks like an argument passed to the test.
| impl IterMatchedWires for KAKResynthesis { | ||
| fn matched_wires(&self) -> impl Iterator<Item = &PersistentWire> + '_ { | ||
| self.wires[0].iter().chain(self.wires[1].iter()) | ||
| } | ||
| } |
There was a problem hiding this comment.
There's something weird here: the way this is written worked for wires: [Vec<PersistentWire>; 2], but it looks like you've changed it to a simple Vec<_>. In this case, self.wires.iter() should be all you need here.
| // Get the new expanded version of the wire to search along. | ||
| let expanded_wire = expanded_walker.get_wire( | ||
| frontier_node, | ||
| IncomingPort::from(0), |
There was a problem hiding this comment.
I don't think this will always be port 0? Also, if you change the above direction, you will need to change it here too.
|
|
||
| // Get the type of operation this node implements | ||
| let ext_op = expanded_walker.as_hugr_view().get_optype(expanded_node).as_extension_op().expect("Not extension op"); | ||
| let tket2_op = Tk2Op::from_extension_op(ext_op).expect("Not TKET2 OP"); |
There was a problem hiding this comment.
I wouldn't error if it is not a TKET2 (it could be an input/output node). Instead you could write
| let tket2_op = Tk2Op::from_extension_op(ext_op).expect("Not TKET2 OP"); | |
| let Ok(tket2_op) = Tk2Op::from_extension_op(ext_op) else { | |
| continue; | |
| }; |
There was a problem hiding this comment.
See also comment below about adding searching_walker back to the queue in this case.
| if !tket2_op.is_quantum() { | ||
| continue; | ||
|
|
There was a problem hiding this comment.
Actually, both here and just above in the suggestion I made about ignoring non-tket2 ops: you need to make sure that in this case you still add the existing match to the list of completed matches to be returned.
Probably the simplest way to do this would be to add searching_walker back to the queue.
|
|
||
| searching_walker.pattern.wires.push(expanded_wire); | ||
|
|
||
| matches.push((searching_walker.pattern, searching_walker.walker)); |
There was a problem hiding this comment.
I suspect the compiler must be complaining here about using searching_walker.pattern and searching_walker.walker in more than one place (more specifically, it will say "use of moved value" further below). You must clone the data:
| matches.push((searching_walker.pattern, searching_walker.walker)); | |
| matches.push((searching_walker.pattern.clone(), searching_walker.walker.clone())); |
That being said, this will add all possible subcircuits (not just the maximal ones). Is this what you want? Might indeed be the right way to go provided we don't get far too many matches.
There was a problem hiding this comment.
Thanks!
Adding all matches was indeed what I was going for as a first try.
| let [q0, q1] = cz1.outputs_arr(); | ||
| builder.finish_hugr_with_outputs(vec![q0, q1]).unwrap() | ||
| } | ||
|
|
There was a problem hiding this comment.
Add a Hugr with more CZ and more than one match to test on. E.g.:
fn three_cz_hugr() -> Hugr {
let mut builder = DFGBuilder::new(endo_sig(vec![qb_t(); 3])).unwrap();
let [q0, q1, q2] = builder.input_wires_arr();
let cz1 = builder.add_dataflow_op(Tk2Op::CZ, vec![q0, q1]).unwrap();
let [q0, q1] = cz1.outputs_arr();
let cz2 = builder.add_dataflow_op(Tk2Op::CZ, vec![q0, q1]).unwrap();
let [q0, q1] = cz2.outputs_arr();
let cz3 = builder.add_dataflow_op(Tk2Op::CZ, vec![q0, q2]).unwrap();
let [q0, q2] = cz3.outputs_arr();
builder.finish_hugr_with_outputs(vec![q0, q1, q2]).unwrap()
}
No description provided.