-
Notifications
You must be signed in to change notification settings - Fork 127
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Pluggable Clifford synthesis for RB circuits #1288
Pluggable Clifford synthesis for RB circuits #1288
Conversation
7662b10
to
b04681d
Compare
No performance regression by this commit:
|
QiskitError: If basis_gates is not supplied. | ||
""" | ||
# synthesize cliffords | ||
circ = synth_clifford_full(high_level_object) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ve examined synth_clifford_depth_lnn
(as suggested by @alexanderivrii) using random RB circuits on 3 linear qubits (i.e. no need for SabreSwap routing) and found that it is a bit faster as expected but surprisingly it produces circuits with more CNOT counts and deeper depths (about 2x/1.5x) than synth_clifford_full
+ SabreSwap. So I’ll stick to synth_clifford_full
in the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
synth_clifford_depth_lnn:
total cx counts = 2961, depth = 4418, max_length=10, time_transpiled_circuits: 1.233635034004692
total cx counts = 6286, depth = 9193, max_length=20, time_transpiled_circuits: 1.9450378390029073
total cx counts = 8659, depth = 12850, max_length=30, time_transpiled_circuits: 2.565633656005957
total cx counts = 11074, depth = 16313, max_length=40, time_transpiled_circuits: 3.270593570006895
total cx counts = 13727, depth = 20190, max_length=50, time_transpiled_circuits: 3.964288625997142
synth_clifford_full:
total cx counts = 1589, depth = 3092, max_length=10, time_transpiled_circuits: 1.8188774910086067
total cx counts = 2915, depth = 6009, max_length=20, time_transpiled_circuits: 2.883968324007583
total cx counts = 4120, depth = 8318, max_length=30, time_transpiled_circuits: 3.982568758990965
total cx counts = 5439, depth = 10996, max_length=40, time_transpiled_circuits: 4.483078286997625
total cx counts = 6493, depth = 13110, max_length=50, time_transpiled_circuits: 5.633502009004587
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
synth_clifford_depth_lnn
is asymptotically better as it guarantees a total cx-depth bounded by 7n+2 (when n is large), but for n=3 qubits perhaps it's less useful.
Note that for the final inverse Clifford, you can reduce the depth and CX count by using |
@ShellyGarion Thanks, good to know, however, I think it's too much for RB to have one more Clifford synthesis option only for the last inversion Clifford. |
d707f5c
to
fd9c575
Compare
1d822f4
to
cdb9483
Compare
cdb9483
to
0923c0e
Compare
0923c0e
to
23277cb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have looked at all of the code and everything looks good. Personally I have especially liked the idea of creating coupling-map and basis-gates aware synthesis plugins by running any synthesis routine, followed by SabreSwap
, undoing layout change, and running BasisTranslation
.
I would be also interested in hearing your feedback on the high level synthesis interface - do you think it could be simplified or extended?
qc = QuantumCircuit(clifford.num_qubits, name=str(clifford)) | ||
qc.append(clifford, qc.qubits) | ||
return _synthesize_clifford_circuit( | ||
qc, | ||
basis_gates=basis_gates, | ||
coupling_tuple=coupling_tuple, | ||
synthesis_method=synthesis_method, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really minor. For a clifford over 3+ qubits, the code would first construct a quantum circuit with this clifford, then convert it back to Clifford (as done on line 191) and then against construct a quantum circuit with this new clifford (as also done on line 191). Possibly these extra translations may be avoided.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's a slight misunderstanding. The extra translations you mentioned happen only when synthesizing a 1- or 2-qubit Clifford using a custom synthesis method. I think it's rare and users who has a Clifford circuit can directly call _synthesize_clifford_circuit
as I do so here. Also _synthesize_clifford
is used only in the constructor of InterleavedRB and not repeatedly called so much. I think there is no clear need for its performance improvement at least for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Somewhat related to this, if HighLevelSynthesis
had a method such that it takes a Clifford and returns a QuantumCircuit (namely synthesize
?), it would be very helpful. Actually, if it was, I didn't need to wrap a Clifford with a QuantumCircuit here just because HighLevelSynthesis.run
requires a DAGCircuit and support only the QuantumCircuit-to-QuantumCircuit conversion through PassManager.run
.
# for 3q+ or custom synthesis method, synthesizes clifford circuit | ||
hls_config = HLSConfig(clifford=[(synthesis_method, {"basis_gates": basis_gates})]) | ||
pm = PassManager([HighLevelSynthesis(hls_config=hls_config, coupling_map=coupling_map)]) | ||
circuit = pm.run(circuit) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great! You are using all of the functionality offered by the high level synthesis interface, including passing additional options (like "basis_gates") for the plugin. Not related to the PR, but I would like to hear your feedback on this: do you think the interface could be simplified? extended?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for providing this useful HighLevelSynthesisPlugin interface. I like it and it's sufficient for this PR.
Let me share a minor point that could be simplified. I was a bit confused with the fact that HighLevelSynthesisPlugin.run
takes over coupling_map
option set up via HighLevelSynthesis.__init__
, but it does not take over basis_gates
option set up via HighLevelSynthesis.__init__
, so I had to set it via hls_config
here. Is this (run
does not have basis_gates
option) intentional?
It was not clear to me which options should be given through hls_config
(HLSConfig
) and which options should be given directly (like coupling_map
) in the current API. Probably, more explanation on that point in the API reference doc would be helpful for developers like me.
As for an extension, it may be worth considering an introduction of HighLevelSynthesis.synthesize
for the ease of custom Clifford-to-QuantumCircuit conversion as I mentioned above.
pm = PassManager( | ||
[ | ||
SabreSwap(coupling_map), | ||
undo_layout_change, | ||
BasisTranslator(sel, basis_gates), | ||
CheckGateDirection(coupling_map), | ||
] | ||
) | ||
pm.append([GateDirection(coupling_map)], condition=_direction_condition) | ||
pm.append([Optimize1qGatesDecomposition(basis=basis_gates)]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is clever. This allows to turn any synthesis plugin into one that adheres to the coupling map and to the basis gates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm good with this, just some minor comments.
releasenotes/notes/plugable-rb-clifford-synthesis-0e66c62fa3088fba.yaml
Outdated
Show resolved
Hide resolved
@@ -1,6 +1,6 @@ | |||
numpy>=1.17 | |||
scipy>=1.4 | |||
qiskit>=0.44 | |||
qiskit>=0.45 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the version bump here, the qiskit>=0.45.0
pin in requirements-dev.txt
can be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed at 3361a6a
releasenotes/notes/plugable-rb-clifford-synthesis-0e66c62fa3088fba.yaml
Outdated
Show resolved
Hide resolved
"""Check if clifford synthesis does not change Clifford""" | ||
basis_gates = tuple(["rz", "h", "cz"]) | ||
coupling_tuple = ( | ||
None if num_qubits == 1 else tuple((i, i + 1) for i in range(num_qubits - 1)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps it's worth to test also a non-linear connectivity (for 4 qubits).
What happens if the qubits are not near each other? do you get an error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added such test cases at 3efc58a and confirmed that it works well with non-linear (star and cycle) connectivities.
I don't understand what you mean by "the qubits are not near each other". Does it mean "disconnected" (e.g. ((0, 1), (2, 3))) or "not full" (e.g. (0, 2), (2, 3), (3, 4))? In both cases, _synthesize_clifford
fails with TranspilerError: The circuit has an invalid layout as two qubits need to interact in disconnected components of the coupling map...
or Fewer qubits in the circuit (4) than the coupling map (5)...
. I think these are expected results for users.
### Summary Add a new experiment class to measure layer fidelity, which is a holistic benchmark to characterize the full quality of the devices at scale (https://arxiv.org/abs/2311.05933) Example notebook: [run_lf_qiskit_experiments_large.ipynb (Gist)](https://gist.github.com/itoko/28c7cc117614c67e2a1899a3757d4ad1) ### Experimental features: - Exceptionally `circuits()` method returns circuits on physical qubits (not virtual qubits as usual) - Add `reason` as an extra analysis result entry to tell users why the `quality` of the analysis was "bad" ### Follow-up items: - Add API for customizing DD (e.g. register DD sequence generator by name and specify the name in experiment_options) ``` def dd_func1(delay_length, backend) -> list[Instruction]; LayerFidelity.dd_functions = { "dd1": dd_func1, "dd2": dd_func2, } ``` ### Features decided not to include: - `full_sampling` option (`StandardRB` has). `LayerFidelity` behaves as if setting always `full_sampling==True` to avoid correlation between sequences, which RB theory does not consider. - `replicate_in_parallel` option that allows to use a common direct RB sequence for all qubit pairs. It turned out that `replicate_in_parallel==True` may underestimate some types of errors, suggesting it should always be set to `False`. ### Issues to be addressed separately - Poor interface for querying figures: No pointer to a relevant figure (or data used for fitting) is stored in `AnalysisResult` (i.e. users who find a bad fitting result in `AnalysisResult` cannot easily look at a figure relevant to the result) ==> For now, you can query a figure by its name; e.g. `exp_data.figure("DirectRB_Q83_Q82")` ### Dependencies: - [x] #1288 for Clifford synthesis with unidirectional 2q gates (e.g. IBM Eagle processors)
Summary
Make the Clifford synthesis algorithm for RB circuits pluggable (implementing it as a
HighLevelSynthesisPlugin
).Fixes #1279 and #1023.
Change to accept Clifford elements consisting only of instructions supported by the backend for
interleaved_element
option inInterleavedRB
.Speed up 2Q RB/IRB for backends with unidirectional 2q gates, e.g. IBM's 127Q Eagle processors.
Details and comments
Previously, for 3Q+ RB circuits, entire circuit is transpiled at once and hence for each of the resulting Cliffords, the initial and the final layout may differ, that means sampled Cliffords are changed during the transpilation. Also in the worst case, the resulting circuit may use physical qubits not in the supplied
physical_qubits
. To avoid that, this commit changes to transpile an RB sequence Clifford by Clifford. The Clifford synthesis algorithm (rb_default
) is implemented as aHighLevelSynthesisPlugin
(seeRBDefaultCliffordSynthesis
inclifford_synthesis.py
), which forces the initial layout (i.e. guarantees the initial layout = the final layout) and physical qubits to use. As a byproduct, the performance of 2Q RB/IRB for backends with directed 2q gates (e.g. IBM's 127Q Eagle processors) is drastically improved. For those cases, previously we had to rely ontranspile
function to make generated circuits comply with the coupling map, however, after this commit, we can synthesize Cliffords with considering the 2q-gate direction and go through the fast path introduced in #982.Depends on Qiskit/qiskit#10477 (qiskit 0.45)