Add target gate set for cirq benchmarking #224
Conversation
|
I manually triggered a benchmark run again so reviewers can see that compilation still works. I'll plan to delete that commit before merging I have a comment in #146 that could simplify this workflow in the future. |
benchmarks/scripts/common.py
Outdated
| """Target gateset for compiling circuits for benchmarking. | ||
|
|
||
| This is modeled off cirq's CZCompilationTargetGateset, but instead: | ||
| * Decomposes single-qubit gates into Rz, Ry, and Rz gates versus XZPowGate. |
There was a problem hiding this comment.
| * Decomposes single-qubit gates into Rz, Ry, and Rz gates versus XZPowGate. | |
| * Decomposes single-qubit gates into Rz, Ry, Rz, and H gates versus XZPowGate. |
There was a problem hiding this comment.
Good catch. I will tweak slightly toDecomposes non target gateset single-qubit gates into Rz, Ry gates versus XZPowGate. so that it's clearer we are decomposing the arbitrary single-qubit gates to just the RzRyRz, but leave X and H gates alone since the are already in the target gateset.
There was a problem hiding this comment.
Ahhh okay yeah I misunderstood that. Thanks for clarifying.
benchmarks/scripts/common.py
Outdated
| ) | ||
|
|
||
| def _decompose_single_qubit_operation( | ||
| self, op: cirq.Operation, _ |
There was a problem hiding this comment.
| self, op: cirq.Operation, _ | |
| self, op: cirq.Operation |
Not suggesting this argument is removed, but just trying to highlight it. What is this argument for if it's not used? I guess required for some ABC?
There was a problem hiding this comment.
Somewhat -- it was to override the same method in the base class (https://github.com/quantumlib/Cirq/blob/dd3df78c045a03b2de70b2d54d8582abbfc1f6c2/cirq-core/cirq/transformers/target_gatesets/compilation_target_gateset.py#L257). Its not strictly abstract since the base class provides a definition.
But I can actually name this to moment. I'm not sure why I switched to unnamed as I don't think the linting rules required it. I'll put it back as its clearer!
benchmarks/tests/__init__.py
Outdated
There was a problem hiding this comment.
Do test directories need init files?
There was a problem hiding this comment.
No, I don't think they do as pytest should still discover them. Its just habit for me, as there's the off chance there's some importing or other tooling that might want to treat it as a package.
| expected_gates = cirq.Gateset(cirq.CNOT, cirq.Rx, cirq.Ry, cirq.Rz, cirq.H) | ||
| assert expected_gates.validate(c_new), ( | ||
| "Cirq compilation had unsupported gatges" | ||
| ) |
There was a problem hiding this comment.
I'm learning about so many cirq functions through this PR! Good stuff.
cirq's optimize_for_target_gateset function requires providing a class that compiles to a desired target gateset. None of the cirq provided target gatesets use the set of gates we consider for benchmarking, so this change adds an appropriate one,
91816f1 to
7e7ffbb
Compare
|
Thanks for the feedback @natestemen! I went ahead and dropped the prior benchmark run from the git lineage. |
cirq's optimize_for_target_gateset function requires providing a class that compiles to a desired target gateset. None of the cirq provided target gatesets use the set of gates we consider for benchmarking, so this change adds an appropriate one with our desired basis.Fixes #201 .