feat!: depth optimisation via commutation pass #74
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
to reduce confusion
requires defining a registry of all used extensions to validate against.
Open
aborgna-q
approved these changes
Sep 21, 2023
pyrs/src/lib.rs
Outdated
Comment on lines
12
to
17
| let s_c = SerialCircuit::_from_tket1(py_c.clone()); | ||
| let mut h: Hugr = s_c.decode()?; | ||
| let n_moves = apply_greedy_commutation(&mut h)?; | ||
|
|
||
| let s_c = SerialCircuit::encode(&h)?; | ||
| Ok((s_c.to_tket1()?, n_moves)) |
There was a problem hiding this comment.
Suggested change
| let s_c = SerialCircuit::_from_tket1(py_c.clone()); | |
| let mut h: Hugr = s_c.decode()?; | |
| let n_moves = apply_greedy_commutation(&mut h)?; | |
| let s_c = SerialCircuit::encode(&h)?; | |
| Ok((s_c.to_tket1()?, n_moves)) | |
| try_with_hugr(py_c, |mut h| { | |
| let n_moves = apply_greedy_commutation(&mut h)?; | |
| let py_c = SerialCircuit::encode(&h)?.to_tket1()?; | |
| PyResult::Ok((py_c, n_moves)) | |
| }) |
(and import try_with_hugr).
pyrs/test/test_depth.ipynb
Outdated
There was a problem hiding this comment.
This is using to_hugr_dot, to_hugr and check_soundness, which are still not being exported in pyrs.
Also, you're loading test files from your home. Maybe put some in tket1/test_files instead?
There was a problem hiding this comment.
i think I can just remove this file from version control
src/circuit.rs
Outdated
| @@ -14,7 +14,7 @@ use hugr::types::FunctionType; | |||
| pub use hugr::types::{EdgeKind, Signature, Type, TypeRow}; | |||
| pub use hugr::{Node, Port, Wire}; | |||
|
|
|||
| use self::units::{filter, FilteredUnits, Units}; | |||
| pub(crate) use self::units::{filter, FilteredUnits, Units}; | |||
There was a problem hiding this comment.
circuit::filter may be confusing.
Make mod units public instead (reexporting FilteredUnits and Units is ok).
src/passes/commutation.rs
Outdated
| use thiserror::Error; | ||
|
|
||
| #[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)] | ||
| struct Qb(usize); |
There was a problem hiding this comment.
This is the same as ::circuit::units::LinearUnit. Can you use a type alias instead?
type Qb = LinearUnit;| } | ||
|
|
||
| #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)] | ||
| struct ComCommand { |
There was a problem hiding this comment.
It'd be nice to avoid the extra wrapper, but it's not possible with our current API (see #126).
For the moment I say use this, and we'll simplify it once that issue is solved.
Simplifies the multithreading logic by using a multi-consumer channel. Includes some small improvements to the TASO bin. Now, why is this a draft: I'm testing with `barenco_tof_5_rm` and `Nam_6_3_complete_ECC_set` on my 12-core laptop. Here are some results with a timeout of `10` seconds: | | circuits seen after 10s | best size after 10s | | ----------------- | ------ | ---- | | single-threaded | 7244 | 178 | | -j 2 | 31391 | 204 | | -j 10 | 42690 | 204 | There's something wrong somewhere :P (note that `-j N` means `N` worker threads, plus the master) --------- Co-authored-by: Luca Mondada <[email protected]> Co-authored-by: Luca Mondada <[email protected]>
- Adds some minimal `tracing` instrumentation. The idea is to do some multithreaded perf debugging in the future based on that. - Leverages the same infrastructure to refactor the TASO logger, now events are logged into the global logger, and the executable subscribes to either general events (for stdout), or more granular data (for the logfile).
`Port::index()` now requires importing a trait
aborgna-q
approved these changes
Sep 27, 2023
ss2165
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Forms slices of topologically sorted commands, then greedily moves them forwards if a command can moved to an earlier slice through commutation.
Can be used with pytket through pyo3 binding, does not change the operations and respects device connectivity so can be run as a final pass after all existing pytket optimisations.
I benchmarked on all the circuits in software_benchmarking, metric is CZ depth
so ~1100 circuits total
248 qiskit wins
752 pytket wins + tket-2 does not improve on pytket
106 tket-2 improves on pytket
twice tket-2 + pytket outperforms qiskit even though qiskit was outperforming pytket
for total depth:
111 qiskit wins
834 pytket wins + tket-2 does not improve on pytket
161 tket-2 improves on pytket
BREAKING CHANGE: LinearUnit is now a newtype struct