Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR generalizes the Wormhole Inspired Teleportation (WIT) benchmark to support arbitrary qubit counts and configuration parameters, addressing issue #609. The implementation replaces hard-coded 6- and 7-qubit circuits with a configurable approach while maintaining backward compatibility through legacy support.
Key changes:
- Introduced
WormholeTeleportationConfigdataclass andWormholeTeleporterFactoryfor parameterized circuit generation - Extended JSON schema to include physics parameters (coupling strength, rotation angles, time steps, etc.)
- Added comprehensive test coverage for the new generalized circuit builder and parameter validation
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| metriq_gym/benchmarks/wit.py | Core refactoring: replaced fixed circuits with configurable builder; added config validation and parameter parsing |
| metriq_gym/schemas/wit.schema.json | Extended schema with new physics parameters; made num_qubits optional for backward compatibility |
| tests/unit/benchmarks/test_wit.py | Added tests verifying generalized circuits match legacy behavior and validate config constraints |
| metriq_gym/schemas/examples/wit.example.json | Updated example to demonstrate new parameterization with 7-qubit swap-method circuit |
| tests/conftest.py | Updated fixture params to include all required WIT configuration fields |
| tests/unit/quantinuum/test_run_fetch_result_quantinuum.py | Updated test params to match new schema requirements |
| metriq_gym/suites/uf_frugal_3.json | Migrated suite config to new parameter format |
| docs/source/quickstart.rst | Added note explaining new parameterization approach |
| docs/source/end_to_end_tutorial.ipynb | Updated notebook output reflecting new param structure |
| docs/source/cli_workflows.rst | Updated suite example with expanded WIT parameters |
| README.md | Added brief explanation of new WIT configuration flexibility |
| metriq_gym/benchmarks/mirror_circuits.py | Minor formatting improvements (unrelated to WIT changes) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
metriq_gym/benchmarks/wit.py
Outdated
| z_rotation_angles_raw = cast(Sequence[SupportsFloat], raw_fields["z_rotation_angles"]) | ||
| z_rotation_angles = tuple(float(v) for v in z_rotation_angles_raw) | ||
| time_steps = int(cast(SupportsInt, raw_fields["time_steps"])) | ||
| insert_method_str = str(raw_fields["insert_message_method"]).lower() |
There was a problem hiding this comment.
The .lower() call may fail if insert_message_method is not a string. Consider validating the type before calling .lower() or handling potential AttributeError.
|
Did you run it for n_qubits = 6 and 7, and cross-check the results against the ones we obtained with the old non-general version? |
Yes. The QASM for the circuits obtained for the 6 and 7-qubit circuits are identical to the QASM used with the parameterized circuits for 6 and 7-qubits. Running using the parameterized version on the IBM Kingston device, we have that: 6-qubits: Using the schema parameterization: {
"benchmark_name": "WIT",
"shots": 8192,
"n_qubits_per_side": 3,
"message_size": 1,
"insert_message_method": "reset",
"interaction_coupling_strength": 1.5707963267948966,
"x_rotation_transverse_angle": 0.7853981633974483,
"zz_rotation_angle": 0.7853981633974483,
"z_rotation_angles": [0.0283397, 0.00519953, 0.0316079],
"time_steps": 3,
"num_qubits": 6
}7-qubits: Using the schema parameterization: {
"benchmark_name": "WIT",
"shots": 8192,
"n_qubits_per_side": 3,
"message_size": 1,
"insert_message_method": "swap",
"interaction_coupling_strength": 1.5707963267948966,
"x_rotation_transverse_angle": 0.7853981633974483,
"zz_rotation_angle": 0.7853981633974483,
"z_rotation_angles": [0.0283397, 0.00519953, 0.0316079],
"time_steps": 3,
"num_qubits": 7
} |
| "n_qubits_per_side": 3, | ||
| "message_size": 1, | ||
| "insert_message_method": "swap", | ||
| "interaction_coupling_strength": 1.5707963267948966, | ||
| "x_rotation_transverse_angle": 0.7853981633974483, | ||
| "zz_rotation_angle": 0.7853981633974483, | ||
| "z_rotation_angles": [ | ||
| 0.0283397, | ||
| 0.00519953, | ||
| 0.0316079 | ||
| ], | ||
| "time_steps": 3, |
There was a problem hiding this comment.
Do we really want to give this much freedom in the parameters? Can't we simplify and have defaults for all parameters except the num of qubits?
There was a problem hiding this comment.
Likely not. I think we can keep this level of specification strictly to the benchmark logic itself and not necessarily expose it to the user. Done.
| class MirrorCircuitsResult(BenchmarkResult): | ||
| success_probability: BenchmarkScore = Field(..., json_schema_extra={"direction": MetricDirection.HIGHER}) | ||
| polarization: BenchmarkScore = Field(..., json_schema_extra={"direction": MetricDirection.HIGHER}) | ||
| success_probability: BenchmarkScore = Field( |
There was a problem hiding this comment.
nit: was this file committed before we had a linter and a formatter, or why does it need reformatting now?
metriq_gym/schemas/wit.schema.json
Outdated
| "interaction_coupling_strength": { | ||
| "type": "number", | ||
| "description": "Interaction coupling strength (g) applied during the two-sided coupling step.", | ||
| "default": 1.5707963267948966, |
There was a problem hiding this comment.
I am guessing those values are fractions of pi? It's odd that one can't use symbols in here. One more reason to get rid of those parameters.
There was a problem hiding this comment.
Good point. Those have since been removed (see the above comment/response).
| def __post_init__(self) -> None: | ||
| if self.n_qubits_per_side < 1: | ||
| raise ValueError("n_qubits_per_side must be at least 1.") | ||
| if self.message_size < 1: | ||
| raise ValueError("message_size must be at least 1.") | ||
| if self.message_size >= self.n_qubits_per_side: | ||
| raise ValueError("message_size must be smaller than n_qubits_per_side.") | ||
| if self.time_steps < 1: | ||
| raise ValueError("time_steps must be at least 1.") | ||
| if self.insert_message_method not in VALID_MESSAGE_METHODS: | ||
| raise ValueError( | ||
| f"insert_message_method must be one of {', '.join(VALID_MESSAGE_METHODS)}." | ||
| ) |
There was a problem hiding this comment.
Aren't those constraints validated as part of the json schema validation?
Closes: #609
Additionally, this MR fixes the following warnings: