refactor: Cleanup tket1 serialized op structures #419

aborgna-q · 2024-06-19T14:18:31Z

This is a noisy internal refactor of ::serialize::pytket::op::JsonOp, extracted from the work towards #379.

JsonOp was a temporary structure used during the encoding/decoding of pytket circuits that represented two different kinds of operation:

pytket operations with a direct tket2 counterpart
other operations that have to be encoded as OpaqueOps

This mixed up the two definitions, and made applying custom logic to one of the variants more annoying.
(E.g. the special handling of bit input/outputs for tket2 ops needed for #379).

This PR splits the structs into a Native and an Opaque variant, so we can keep the implementation clean. The code is functionally the same.

codecov · 2024-06-19T14:25:54Z

Codecov Report

Attention: Patch coverage is 84.34505% with 49 lines in your changes missing coverage. Please review.

Project coverage is 81.33%. Comparing base (8c5a487) to head (58b5966).

Files	Patch %	Lines
tket2/src/serialize/pytket/op/serialised.rs	80.88%	8 Missing and 5 partials ⚠️
tket2/src/serialize/pytket/op/native.rs	92.07%	7 Missing and 1 partial ⚠️
tket2/src/serialize/pytket/decoder.rs	86.00%	4 Missing and 3 partials ⚠️
tket2/src/serialize/pytket/op.rs	77.41%	6 Missing and 1 partial ⚠️
tket2/src/ops.rs	55.55%	4 Missing ⚠️
tket2/src/serialize/pytket/encoder.rs	86.66%	1 Missing and 3 partials ⚠️
tket2/src/serialize/pytket.rs	0.00%	1 Missing and 2 partials ⚠️
tket2/src/passes/commutation.rs	33.33%	0 Missing and 2 partials ⚠️
tket2/src/passes/pytket.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #419      +/-   ##
==========================================
+ Coverage   81.00%   81.33%   +0.33%     
==========================================
  Files          57       59       +2     
  Lines        5411     5423      +12     
  Branches     4926     4938      +12     
==========================================
+ Hits         4383     4411      +28     
+ Misses        819      793      -26     
- Partials      209      219      +10

Flag	Coverage Δ
python	`97.52% <ø> (ø)`
rust	`79.74% <84.34%> (+0.37%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

aborgna-q · 2024-06-19T14:42:34Z

I fixed the CI error.
The one functional change I made was adding .with_extension_delta(TKET1_EXTENSION_ID) to the decoded circuit signature, and that's a bit untested in the rest of the code :P

doug-q

Looks much better. I've not looked at coverage lines yet, that may induce further comments.

doug-q · 2024-06-20T08:13:38Z

tket2/src/ops.rs

+}
+
+// Internal implementation for `TryFrom<Optype> for Tk2Op` that doesn't copy the `OpType` when it errors.
+fn optype_to_tk2op(op: &OpType) -> Option<Tk2Op> {


Both callsites create the same error anyway, I would prefer to do it once here. Not a big deal though.

doug-q · 2024-06-20T08:19:55Z

tket2/src/serialize/pytket/op.rs


-/// A serialized operation, containing the operation type and all its attributes.
+/// An operation originated from pytket, containing the operation type and all its attributes.


Suggested change

/// An operation originated from pytket, containing the operation type and all its attributes.

/// An operation originating from pytket, containing the operation type and all its attributes.

doug-q · 2024-06-20T08:22:31Z

tket2/src/serialize/pytket/op.rs

-        op.compute_param_fields();
-        Some(op)
+
+        tk1_op.ok_or_else(|| OpConvertError::UnsupportedOpSerialization(op))


Suggested change

tk1_op.ok_or_else(|| OpConvertError::UnsupportedOpSerialization(op))

tk1_op.ok_or(OpConvertError::UnsupportedOpSerialization(op))

doug-q · 2024-06-20T08:24:48Z

tket2/src/serialize/pytket/op.rs

+    /// Requires specifying the number of qubits and bits in the operation in
+    /// case the `serial_op` does not define a signature.


Suggested change

/// Requires specifying the number of qubits and bits in the operation in

/// case the `serial_op` does not define a signature.

/// If `serial_op` defines a signature then `num_qubits` and `num_qubits` are ignored. Otherwise, a signature is synthesised from those parameters.

doug-q · 2024-06-20T08:27:52Z

tket2/src/serialize/pytket/op.rs

-            param_inputs,
-            num_params,
+    /// Get the hugr optype for the operation.
+    pub fn optype(&self) -> OpType {


Perhaps this should take self by value, since it unconditionally clones. I am not sure whether this would work having not inspected all methods used below.

The Opaque path doesn't consume the json_op, so we avoid a clone by using &self.

But Native is the other way, returning an Optype consumes it.

I'll define both options, so the caller can choose.

doug-q · 2024-06-20T08:38:47Z

tket2/src/serialize/pytket/op/serialised.rs

+            // TODO: Throw an error? We should never get here if the name matches.
+            return None;
+        };
+        let op = serde_yaml::from_value(arg.value.clone()).ok()?;


Can we return a Result? Swallowing this error is a bit gross.

doug-q · 2024-06-20T08:41:08Z

tket2/src/serialize/pytket/op/serialised.rs

+
+    /// Compute the signature of the operation.
+    ///
+    /// We assume the operation has `num_qubits` qubit inputs and outputs,


Suggested change

/// We assume the operation has `num_qubits` qubit inputs and outputs,

/// The signature returned has `num_qubits` qubit inputs, followed by `num_bits` bit inputs, followed by `num_params` `f64` inputs. It has `num_qubits` qubit outputs follwed by `num_bits` bit outputs.

doug-q · 2024-06-20T08:42:05Z

tket2/src/serialize/pytket/op/serialised.rs

+
+    /// Wraps the op into a [`TKET1_OP_NAME`] opaque operation.
+    pub fn as_custom_op(&self) -> CustomOp {
+        let op = serde_yaml::to_value(self).unwrap();


I have looked at all think these unwraps and I think they're ok.

doug-q · 2024-06-20T08:43:06Z

tket2/src/serialize/pytket/op/serialised.rs

+        //let sig = self.signature();
+        //OpaqueOp::new(
+        //    TKET1_EXTENSION_ID,
+        //    TKET1_OP_NAME,
+        //    "".into(),
+        //    vec![payload],
+        //    sig,
+        //)
+        //.into()


Suggested change

//let sig = self.signature();

//OpaqueOp::new(

// TKET1_EXTENSION_ID,

// TKET1_OP_NAME,

// "".into(),

// vec![payload],

// sig,

//)

//.into()

doug-q · 2024-06-20T08:44:02Z

tket2/src/extension.rs

        "An opaque TKET1 operation.".into(),
-        JsonOpSignature([json_op_payload])
+        JsonOpSignature([tket1_op_payload])


should we rename JsonOpSignature?

doug-q · 2024-06-20T08:47:53Z

I suggest grepping for JsonOp in case there are any other dangling references.

doug-q · 2024-06-20T08:54:28Z

tket2/src/serialize/pytket/op/native.rs

+    }
+
+    /// Consumes the `NativeOp` and returns the underlying `OpType`.
+    pub fn into_op(self) -> OpType {


Suggested change

pub fn into_op(self) -> OpType {

pub fn into_optype(self) -> OpType {

Co-authored-by: doug-q <[email protected]>

doug-q

Just a couple of nits, and one suspicious coverage gap. Happy to approve if you don't think it's a problem.

doug-q · 2024-06-20T11:32:02Z

tket2/src/ops.rs

+    }
+}
+
+// Internal implementation for `TryFrom<Optype> for Tk2Op` that doesn't copy the `OpType` when it errors.


Suggested change

// Internal implementation for `TryFrom<Optype> for Tk2Op` that doesn't copy the `OpType` when it errors.

Ah, the original function that returned an Option was meant as an optimisation to avoid cloning the optype on the error path of TryFrom<OpType>. I guess it's too much of a micro optimisation, so I'll just leave the TryFrom<&OpType impl as we had before.

doug-q · 2024-06-20T11:36:41Z

tket2/src/serialize/pytket.rs

@@ -169,16 +148,44 @@ pub fn save_tk1_json_str(circ: &Circuit) -> Result<String, TK1ConvertError> {

 /// Error type for conversion between `Op` and `OpType`.
 #[derive(Debug, Error)]
+#[non_exhaustive]
+pub enum OpConvertError {
+    /// The serialized operation is not supported.


these comments duplicating the error messages right next to them are very annoying. 😠 . No action required on your part.

doug-q · 2024-06-20T11:39:49Z

tket2/src/serialize/pytket/op/native.rs

+                if ty == &QB_T {
+                    num_qubits += 1
+                } else if *ty == *LINEAR_BIT {
+                    num_bits += 1


this line seems to be missing coverage? somewhat concerning.

We don't support native measurements yet (that's coming in the PR eliminating linear bits), so this path is never taken.
There's a test with a pytket measurement, but that gets encoded as an opaque op instead.

doug-q · 2024-06-20T11:41:07Z

tket2/src/serialize/pytket/op/serialised.rs

+    ///
+    /// # Errors
+    ///
+    /// Returns a


incomplete comment

@aborgna-q I think you missed this?

Nope, I can't work computers. It's perfect

## 🤖 New release * `tket2`: 0.1.0-alpha.2 -> 0.1.0 * `tket2-hseries`: 0.1.0 <details><summary>Changelog</summary> ## `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> </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>Changelog</summary> ## `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> </details> --- This PR was generated with [release-plz](https://github.com/MarcoIeni/release-plz/). --------- Co-authored-by: Seyon Sivarajah <[email protected]>

refactor: Cleanup tket1 serialized op structures

18c48fb

aborgna-q requested a review from doug-q June 19, 2024 14:18

Ignore mismatched extension_reqs in CircuitChunks::update_circuit

385e881

aborgna-q force-pushed the ab/rework-jsonop branch from 29bfe9e to 385e881 Compare June 19, 2024 14:51

doug-q reviewed Jun 20, 2024

View reviewed changes

aborgna-q and others added 4 commits June 20, 2024 10:10

Rename all JsonOp.*

720c275

s/into_op/into_optype/

b38d3e5

Address more review comments

4814cfb

Co-authored-by: doug-q <[email protected]>

Test native tk2op roundtrip

6dd44e8

aborgna-q requested a review from doug-q June 20, 2024 10:32

Remove unused OpaqueTk1Op constructor

3e8f817

doug-q reviewed Jun 20, 2024

View reviewed changes

aborgna-q added 2 commits June 20, 2024 13:03

Remove TryFrom<OpType> for Tk2Op

22de8c8

Fill incomplete comment

58b5966

aborgna-q requested a review from doug-q June 20, 2024 12:08

doug-q approved these changes Jun 20, 2024

View reviewed changes

aborgna-q added this pull request to the merge queue Jun 20, 2024

Merged via the queue into main with commit a46e63f Jun 20, 2024
16 checks passed

aborgna-q deleted the ab/rework-jsonop branch June 20, 2024 12:26

hugrbot mentioned this pull request Jun 18, 2024

chore: release #409

Closed

hugrbot mentioned this pull request Aug 1, 2024

chore: release #519

Merged

hugrbot mentioned this pull request Aug 1, 2024

chore: release #521

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Cleanup tket1 serialized op structures #419

refactor: Cleanup tket1 serialized op structures #419

aborgna-q commented Jun 19, 2024

codecov bot commented Jun 19, 2024 •

edited

Loading

aborgna-q commented Jun 19, 2024

doug-q left a comment

doug-q Jun 20, 2024

doug-q Jun 20, 2024

doug-q Jun 20, 2024

doug-q Jun 20, 2024

doug-q Jun 20, 2024

aborgna-q Jun 20, 2024

doug-q Jun 20, 2024

doug-q Jun 20, 2024

doug-q Jun 20, 2024

doug-q Jun 20, 2024

doug-q Jun 20, 2024

doug-q commented Jun 20, 2024

doug-q Jun 20, 2024

doug-q left a comment

doug-q Jun 20, 2024

aborgna-q Jun 20, 2024

doug-q Jun 20, 2024

doug-q Jun 20, 2024

aborgna-q Jun 20, 2024

doug-q Jun 20, 2024

doug-q Jun 20, 2024

doug-q Jun 20, 2024


		/// A serialized operation, containing the operation type and all its attributes.
		/// An operation originated from pytket, containing the operation type and all its attributes.

	/// An operation originated from pytket, containing the operation type and all its attributes.
	/// An operation originating from pytket, containing the operation type and all its attributes.

	tk1_op.ok_or_else(\|\| OpConvertError::UnsupportedOpSerialization(op))
	tk1_op.ok_or(OpConvertError::UnsupportedOpSerialization(op))

		/// Requires specifying the number of qubits and bits in the operation in
		/// case the `serial_op` does not define a signature.

	/// Requires specifying the number of qubits and bits in the operation in
	/// case the `serial_op` does not define a signature.
	/// If `serial_op` defines a signature then `num_qubits` and `num_qubits` are ignored. Otherwise, a signature is synthesised from those parameters.

	/// We assume the operation has `num_qubits` qubit inputs and outputs,
	/// The signature returned has `num_qubits` qubit inputs, followed by `num_bits` bit inputs, followed by `num_params` `f64` inputs. It has `num_qubits` qubit outputs follwed by `num_bits` bit outputs.

	pub fn into_op(self) -> OpType {
	pub fn into_optype(self) -> OpType {

refactor: Cleanup tket1 serialized op structures #419

refactor: Cleanup tket1 serialized op structures #419

Conversation

aborgna-q commented Jun 19, 2024

codecov bot commented Jun 19, 2024 • edited Loading

Codecov Report

aborgna-q commented Jun 19, 2024

doug-q left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

doug-q commented Jun 20, 2024

Choose a reason for hiding this comment

doug-q left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 19, 2024 •

edited

Loading