fix(pytket decoder): Panic on repeated bit registers in pytket decoded output#1445
Merged
fix(pytket decoder): Panic on repeated bit registers in pytket decoded output#1445
Conversation
aborgna-q
commented
Mar 13, 2026
aborgna-q
commented
Mar 13, 2026
| node, | ||
| hugr, | ||
| // Output bits use new registers, so don't reuse any input bits. | ||
| EmitCommandOptions::new().reuse_bits(|_| vec![]), |
Collaborator
Author
There was a problem hiding this comment.
Drive-by: The classical expression emitter was re-using the input bits to the node instead of using a separate output bit register.
Contributor
There was a problem hiding this comment.
Yeah this looks like it might even have been the bigger fix?? Do we have a test that covers just this?
Collaborator
Author
There was a problem hiding this comment.
It's not used much, hence why we never saw the bug.
I added a test.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1445 +/- ##
==========================================
+ Coverage 79.56% 79.96% +0.40%
==========================================
Files 155 155
Lines 20245 20237 -8
Branches 19254 19246 -8
==========================================
+ Hits 16107 16182 +75
+ Misses 3186 3099 -87
- Partials 952 956 +4
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:
|
aborgna-q
force-pushed
the
ab/decoder-fixes
branch
from
204bbf7 to
c8ea134
Compare
acl-cqc
approved these changes
Mar 17, 2026
Contributor
acl-cqc
left a comment
acl-cqc
left a comment
There was a problem hiding this comment.
Thanks, @aborgna-q - I think there are some fairly significant improvements/fixes in, so keen to get this in :). I think a couple of comments are significant (PR title/panicking) but mostly just neatness/comprehension so I'll approve rightaway :-)
| encoder.emit_node_command( | ||
| node, | ||
| hugr, | ||
| // Output bits use new registers, so don't reuse any input bits. |
Contributor
There was a problem hiding this comment.
super-nit: consider assigning this to a variable first (called output_bits, thus shortening the comment?)
| for qubit in encoded_info.output_qubits.iter() { | ||
| let id = self.wire_tracker.tracked_qubit_for_register(qubit)?; | ||
| qubits.insert(id.clone()); | ||
| qubits.push(id.clone()); |
Contributor
There was a problem hiding this comment.
The PR title is "Panic on repeated registers". I don't see anything that would panic if the same qubit was pushed more than once - TBH it seems to me that "repeated registers" for qubits is a fairly serious error (linearity violation), so panicking sounds like a good thing to do?
(Not sure whether "registers" means "qubits", "bits" or both - panicking on repeated bits sounds an odd thing to do if you are from any background other than pytket 😉 😁 but you might want to)
If the PR title is wrong and we don't panic, please update it ;-), maybe something about bit registers if it doesn't apply to qubits.
Collaborator
Author
There was a problem hiding this comment.
Indeed, it only applies to bit registers.
Qubit outputs should never be repeated if the metadata originates from a valid hugr.
I swapped both IndexSet for Vecs for symmetry, but for qubit it doesn't really change anything.
| node, | ||
| hugr, | ||
| // Output bits use new registers, so don't reuse any input bits. | ||
| EmitCommandOptions::new().reuse_bits(|_| vec![]), |
Contributor
There was a problem hiding this comment.
Yeah this looks like it might even have been the bigger fix?? Do we have a test that covers just this?
| // Convert the wire type, if needed. | ||
| let wire_data = &self.wires[&wire]; | ||
| let new_wire = config.transform_typed_value(wire, wire_data.ty(), ty, builder)?; | ||
| let found_wire_type = wire_data.ty(); |
Contributor
There was a problem hiding this comment.
not clear why you've changed this, you're not using found_wire_type anywhere you weren't before AFAICS. If you're intending the name to be an improvement then you should probably rename wire_data to found_wire instead?
Collaborator
Author
There was a problem hiding this comment.
There was a bug hidden in here, we were tracking the new converted wire with the type of the pre-conversion wire.
I inlined found_wire_type and changed wire_data to found_wire_data to make things clear.
aborgna-q
changed the title
Collaborator
|
This PR contains breaking changes to the public Rust API. cargo-semver-checks summary |
Draft
cgh-qtnm
pushed a commit
that referenced
this pull request
Mar 20, 2026
…d output (#1445) Fix an edge case where we extracted a hugr from a pytket circuit, and declared multiple output ports to have the same bit register. Also cleans up a bit of the logic that handled consumed bit wires, to avoid marking them as outdated if they can still be used by other operations.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Fix an edge case where we extracted a hugr from a pytket circuit, and declared multiple output ports to have the same bit register.
Also cleans up a bit of the logic that handled consumed bit wires, to avoid marking them as outdated if they can still be used by other operations.