fix: Don't use opgroup in pytket barrier encoding #1251
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1251 +/- ##
==========================================
+ Coverage 79.06% 79.12% +0.05%
==========================================
Files 160 160
Lines 20421 20412 -9
Branches 19489 19480 -9
==========================================
+ Hits 16145 16150 +5
+ Misses 3289 3279 -10
+ Partials 987 983 -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:
|
acl-cqc
left a comment
acl-cqc
left a comment
There was a problem hiding this comment.
Yup, thanks @aborgna-q . Looks about as nice as I think it's possible to be given the tket1 serialization format...so pretty grotty 😆 but LGTM!
| /// - `opgroup`: A tket1 operation group identifier, if any. | ||
| /// - `opgroup`: A tket1 [operation group | ||
| /// identifier](https://docs.quantinuum.com/tket/user-guide/manual/manual_circuit.html#modifying-operations-within-circuits), | ||
| /// if any. |
There was a problem hiding this comment.
This is pub wrt the whole crate so you can't just drop the param?
There was a problem hiding this comment.
- It's part of the public API (custom encoders may call this function)
- It's a generic call to emit arbitrary pytket commands, no reason not to allow custom
optypesif someone wants to use them.
| bits: &[TrackedBit], | ||
| params: &[LoadedParameter], | ||
| opgroup: Option<&str>, | ||
| _opgroup: Option<&str>, |
There was a problem hiding this comment.
:-( I guess there is no plan to deprecate this? (Some third-party encoder/decoder might still use it?)
There was a problem hiding this comment.
Same here, it's part of the PytketDecoder trait. We don't lose much by giving the info to the user (pytket commands are normally an Operation + arguments + opgroup)
| pub use payload::{EncodedEdgeID, OpaqueSubgraphPayload}; | ||
|
|
||
| #[expect(deprecated)] | ||
| pub use payload::OPGROUP_OPAQUE_HUGR; |
There was a problem hiding this comment.
Do we need to #[deprecate] this too, or does it inherit that from the original definition?
There was a problem hiding this comment.
No deprecation on re-exports, the warning is emitted if the main definition is deprecated.
| /// A reference to a subgraph tracked by an `OpaqueSubgraphs` registry | ||
| /// in an [`EncodedCircuit`][super::super::circuit::EncodedCircuit] | ||
| /// structure. | ||
| #[serde(rename = "HugrExternal")] |
There was a problem hiding this comment.
Does this mean we should declare this as a breaking PR?
There was a problem hiding this comment.
These external payloads are meant to be short-lived, and are only valid when decoded by the same runtime that generated them.
You could make the case that we are breaking Inline payloads but I don't expect anyone to be storing pytket circuits with embedded Hugrs, with the aim to recover them atm
| bits: &[TrackedBit], | ||
| params: &[LoadedParameter], | ||
| opgroup: Option<&str>, | ||
| _opgroup: Option<&str>, |
There was a problem hiding this comment.
It's part of the PytketDecoder trait. We don't lose much by giving the info to the user (pytket commands are normally an Operation + arguments + opgroup)
🤖 I have created a release *beep* *boop* --- ## [0.12.11](tket-py-v0.12.10...tket-py-v0.12.11) (2025-11-13) ### Bug Fixes * **ci:** Make wheels compatible with MacOS 15.0 (#1248) ([49c5996](49c5996)) * Don't use opgroup in pytket barrier encoding (#1251) ([49c5996](49c5996)) * guppy_to_circuit always returns num_operations = 0 (#1200) ([49c5996](49c5996)) * **pytket-decoder:** Avoid QAllocating and immediately freeing qubits ([#1256](#1256)) ([228ff52](228ff52)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: Agustín Borgna <[email protected]>
We were writing
OPAQUE_HUGRto the pytket barrier operation's opgroup when encoding opaque subgraphs, to be able to quickly check if an incoming barrier should be decoded and return errors on invalid payloads.However, this was an invalid use of the
opgroupfield which caused pytket to panic when multiple operations with the same opgroup had different signatures.This PR removes the use of that field, and adds some helpers to check the contents of a barrier payload instead.