-
Notifications
You must be signed in to change notification settings - Fork 6
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: Drop linear bits, improve pytket encoding/decoding #420
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #420 +/- ##
==========================================
- Coverage 81.41% 81.26% -0.16%
==========================================
Files 59 59
Lines 5423 5838 +415
Branches 4938 5349 +411
==========================================
+ Hits 4415 4744 +329
- Misses 790 854 +64
- Partials 218 240 +22
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
The reduced coverage is mostly related to error paths and functionality that's still unused (e.g. input bools in operations; there's no classically controlled ops in tket2 yet so we cannot test that). |
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.
much more flexible than i anticipated
main concerns are with use of default "q" and "c" registers
tket2/src/serialize/pytket/tests.rs
Outdated
|p: &Vec<circuit_json::Permutation>| -> HashMap<circuit_json::Register, circuit_json::Register> {p.iter().map(|p| (p.0.clone(), p.1.clone())).collect()}; | ||
let _permutation_a = get_permutation(&a.implicit_permutation); | ||
let _permutation_b = get_permutation(&b.implicit_permutation); | ||
|
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 there supposed to be an assert here that the permutations are equal?
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.
We cannot unsure that anymore, as circuits with different permutations may still be equal.
I removed this bit and added a simple validation instead.
.map(|p| (p.1.clone(), p.0.clone())) | ||
.collect(); | ||
for qubit in &serialcirc.qubits { | ||
output_qubits.push(output_to_input.get(qubit).unwrap_or(qubit).clone()); |
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 find this confusing because you're pushing the result of an "output->input" mapping to "output_qubits", I don't think its wrong but can you add some clarifying comments or renames?
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 added better comments, with an example.
We are storing the input name for each output register, in the order that outputs appear in a circuit.
let wire = self.register_wires.remove(®ister).unwrap(); | ||
outputs.push(wire); | ||
} | ||
for wire in self.register_wires.into_values() { |
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.
how does this case come about? I note it is also not covered by test
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.
It shouldn't, since ordered_wires
always matches the qubit_registers
keys.
I replaced the loop with an assert.
} | ||
|
||
// Assign the new output wires to some register, if needed. | ||
for (register, wire) in output_registers.into_iter().zip_eq(wires) { |
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.
so this reuses existing registers rather than creating a scratch/temp one?
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.
We're just following the registers indicated by the serial circuit, we don't need temp registers here since they all have been declared in SerialCircuit::{qu,}bit
already.
|
||
/// Add a new register unit for a bit wire. | ||
pub fn add_qubit_register(&mut self, unit_id: usize) -> &RegisterUnit { | ||
let reg = RegisterUnit("q".to_string(), vec![self.qubit_to_reg.len() as i64]); |
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.
if "q" register isn't already being used this could suddenly create a very large "q" register with mostly unused qubits right? Not too much of a problem for tket but if then lowered to something that creates whole registers would be annoying. Maybe worth maintaining a "next free qubit"?
} | ||
} | ||
|
||
// TODO: Look at the circuit outputs to determine the final permutation. |
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.
now or later?
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.
Added a comment. It needs some extra plumbing, so I'll add it as an issue.
None => RegisterUnit("c".to_string(), vec![self.bit_to_reg.len() as i64]), | ||
}; |
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.
not covered, also might be a problem for targets that enforce bit register max sizes for the problems mentioned in the qubit version
.zip(circuit_output_order) | ||
.map(|(out, circ_out)| (RegisterHash::from(out), circ_out)) | ||
.collect(); | ||
// The final permutation is the composition of these two mappings. |
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.
the comments are very helpful
Co-authored-by: Seyon Sivarajah <[email protected]>
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.
down to just two questions!
let mut last_unit: Option<u16> = None; | ||
for reg in existing { | ||
if reg.0 != register { | ||
continue; |
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.
suggests there should be some tests with multiple register names?
if reg.0 != register { | ||
continue; | ||
} | ||
last_unit = Some(reg.1[0] as u16); |
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.
does this assume the existing
are always returned in order (i.e. q[0] is checked before q[1])?
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.
🤦Yep
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.
## 🤖 New release * `tket2`: 0.1.0-alpha.2 -> 0.1.0 * `tket2-hseries`: 0.1.0 <details><summary><i><b>Changelog</b></i></summary><p> ## `tket2` <blockquote> ## [0.1.0](tket2-v0.1.0-alpha.2...tket2-v0.1.0) - 2024-08-01 ### Bug Fixes - Single source of truth for circuit names, and better circuit errors ([#390](#390)) - Support non-DFG circuits ([#391](#391)) - Portmatching not matching const edges ([#444](#444)) - Pattern matcher discriminating on opaqueOp description ([#441](#441)) - `extract_dfg` inserting the output node with an invalid child order ([#442](#442)) - Recompile ecc sets after [#441](#441) ([#484](#484)) ### Documentation - Update tket2-py readme ([#431](#431)) - Better error reporting in portmatching ([#437](#437)) - Improved multi-threading docs for Badger ([#495](#495)) ### New Features - `Circuit::operations` ([#395](#395)) - tuple unpack rewrite ([#406](#406)) - guppy → pytket conversion ([#407](#407)) - Drop linear bits, improve pytket encoding/decoding ([#420](#420)) - *(py)* Allow using `Tk2Op`s in the builder ([#436](#436)) - Initial support for `TailLoop` as circuit parent ([#417](#417)) - Support tuple unpacking with multiple unpacks ([#470](#470)) - Partial tuple unpack ([#475](#475)) - [**breaking**] Compress binary ECCs using zlib ([#498](#498)) - Add timeout options and stats to Badger ([#496](#496)) - Expose advanced Badger timeout options to tket2-py ([#506](#506)) ### Refactor - [**breaking**] Simplify tket1 conversion errors ([#408](#408)) - Cleanup tket1 serialized op structures ([#419](#419)) ### Testing - Add coverage for Badger split circuit multi-threading ([#505](#505)) </blockquote> ## `tket2-hseries` <blockquote> ## [0.1.0](https://github.com/CQCL/tket2/releases/tag/tket2-hseries-v0.1.0) - 2024-08-01 ### New Features - [**breaking**] init tket2-hseries ([#368](#368)) - *(tket2-hseries)* Add `tket2.futures` Hugr extension ([#471](#471)) - Add lazify-measure pass ([#482](#482)) - add results extensions ([#494](#494)) - *(tket2-hseries)* [**breaking**] Add `HSeriesPass` ([#487](#487)) </blockquote> </p></details> --- This PR was generated with [release-plz](https://github.com/MarcoIeni/release-plz/). --------- Co-authored-by: Douglas Wilson <[email protected]>
## 🤖 New release * `tket2`: 0.1.0 -> 0.1.1 * `tket2-hseries`: 0.1.0 -> 0.1.1 <details><summary><i><b>Changelog</b></i></summary><p> ## `tket2` <blockquote> ## [0.1.0](tket2-v0.1.0-alpha.2...tket2-v0.1.0) - 2024-08-01 ### Bug Fixes - Single source of truth for circuit names, and better circuit errors ([#390](#390)) - Support non-DFG circuits ([#391](#391)) - Portmatching not matching const edges ([#444](#444)) - Pattern matcher discriminating on opaqueOp description ([#441](#441)) - `extract_dfg` inserting the output node with an invalid child order ([#442](#442)) - Recompile ecc sets after [#441](#441) ([#484](#484)) ### Documentation - Update tket2-py readme ([#431](#431)) - Better error reporting in portmatching ([#437](#437)) - Improved multi-threading docs for Badger ([#495](#495)) ### New Features - `Circuit::operations` ([#395](#395)) - tuple unpack rewrite ([#406](#406)) - guppy → pytket conversion ([#407](#407)) - Drop linear bits, improve pytket encoding/decoding ([#420](#420)) - *(py)* Allow using `Tk2Op`s in the builder ([#436](#436)) - Initial support for `TailLoop` as circuit parent ([#417](#417)) - Support tuple unpacking with multiple unpacks ([#470](#470)) - Partial tuple unpack ([#475](#475)) - [**breaking**] Compress binary ECCs using zlib ([#498](#498)) - Add timeout options and stats to Badger ([#496](#496)) - Expose advanced Badger timeout options to tket2-py ([#506](#506)) ### Refactor - [**breaking**] Simplify tket1 conversion errors ([#408](#408)) - Cleanup tket1 serialized op structures ([#419](#419)) ### Testing - Add coverage for Badger split circuit multi-threading ([#505](#505)) </blockquote> ## `tket2-hseries` <blockquote> ## [0.1.1](tket2-hseries-v0.1.0...tket2-hseries-v0.1.1) - 2024-08-15 ### New Features - *(tket2-hseries)* make result operation internals public ([#542](#542)) </blockquote> </p></details> --- This PR was generated with [release-plz](https://github.com/MarcoIeni/release-plz/). --------- Co-authored-by: Seyon Sivarajah <[email protected]>
Removes the ad-hoc
LINEAR_BIT
used for decoding pytket circuits, and uses non-linearBOOL_T
s instead.This now lets us encode guppy circuits with measurements;
Mermaid diagram (rooted on the
DataflowBlock
):tket1 circuit:
![circuit](https://private-user-images.githubusercontent.com/121866228/341782547-62877218-dd24-4e5e-a8f7-ce738e05662c.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3Mzk2MDcxMDEsIm5iZiI6MTczOTYwNjgwMSwicGF0aCI6Ii8xMjE4NjYyMjgvMzQxNzgyNTQ3LTYyODc3MjE4LWRkMjQtNGU1ZS1hOGY3LWNlNzM4ZTA1NjYyYy5wbmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjUwMjE1JTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI1MDIxNVQwODA2NDFaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT04MzlmYzRmNmRmNzVkZjQyMmUxM2U1ZDE1Yjc3MTE3NDI3MGE0NjA0NmVhNWFkODgxMTBiNTU1M2Q0NGZkYWQzJlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCJ9.m4fbX-lyC1U1b9gs5C5obtUkwmbWnJlZUY43vPgmmtc)
Re-extracted circuit:
This required multiple improvements to the encoder/decoder logic, including
Tk1Op::Native
operations (backed by aTk2Op
) can now have different number of input/output qubit/bits.QAlloc
/QFree
operations (generated by guppy) by adding extra input/outputs to the circuit.opgroup
value from decoded pytket operations.Closes #379