qiskit-experiments
qiskit-experiments copied to clipboard
Clifford Mirror RB experiment
Summary
Implements mirror RB with single Clifford + CX layers and a sampler class for generating layer samples.
Details and comments
This is the updated version of #842 that has been refactored in the new RB structure. There were problems with the reno history that I couldn't fix without rebasing, and I didn't want to overwrite the branch history so I continued on my fork.
The main changes from that PR are adding a Sampler class based on @ihincks's comment and refactoring mirror RB into the new RB structure, keeping the single qubit Cliffords integers until circuit generation. I tested with a two-qubit experiment and it was roughly ~20% faster than the previous implementation.
Also, for computing the target bitstrings, Clifford()
is used but it doesn't work when the basis gates are specified, so currently two copies of the circuits are made, one being the actual circuits and one without basis gates for calculating the target bitstring. Once https://github.com/Qiskit/qiskit-terra/pull/9475 is merged, the redundant circuit generation step can be removed (thanks to @itoko for helping with this).
@nkanazawa1989 @itoko I realized the previous design of the Sampler class wasn't very friendly for customization, so I've refactored it. Now users can specify their own sampling distributions or change the distribution settings in the experiment class. I'm done with interface changes before review comments.
@itoko Because we don't require the user to provide a backend on experiment instantiation, the sampler class was designed to have a minimal __init__
so that it stayed backend and coupling map-agnostic until samples needed to be generated. Do you think we should just require backend on experiment instantiation then? Or we can override self._set_backend
like we used to and update self._distribution
when it's called.
Ah, I missed the point (backend
can be None and replaceable after instantiation) again... I'm happy if we could require backend on experiment instantiation (I personally think that is good for all experiments) but I'm not sure everyone agree with that.
@itoko I think MirrorRB(sampler_algorithm="my_sampler", sampler_opts={a: a, b: b, c: c}, x=x, y=y, z=z)
is a good interface, but there are some caveats. Currently there's a problem with moving all the _set_distribution_options()
logic into the sampler, because the two-qubit density to set in the sampler depends on whether the pauli layers are on in the main experiment. So there needs to be logic in MirrorRB
that manipulates two_qubit_density
before passing it to the sampler, which is not ideal if we want to decouple the experiment from these options. One solution is to just pass this work to the user, i.e. specify that two_qubit_density
refers to the density in the sampled Clifford layers and not in the circuit overall. I think this might be okay because people are often thinking of the density in the Clifford layers when they think about density anyways.
Another issue is if we have two_qubit_gate_density
and two_qubit_gate
as built-in mirror RB input options, and the user sets those but also overrides them with sampler_opts={gate_distribution=(...)}
, should we do a sanity check or let the user override the two qubit options with a potentially conflicting distribution? And for the custom samplers where specifying two_qubit_gate_density
and two_qubit_gate
doesn't make sense, for example a sampler that samples from predefined layers, should we try to pass those options along anyways and fail gracefully if they're not accepted? Or perhaps we only try to pass these options along when the sampler is edge_grab
, and specify in the docstring that they only work for the default sampler?
I like the alternative design, and I think it will be clear to the user that they have to reset backend
and reduced_coupling_map
every time they reset the sampler
since they will be initialization options, but indeed it will be harder to serialize.
Thank you for providing detailed issues.
-
Regarding the coupling of
two_qubit_density
andpauli_randomize
, I thinkMirrorRB
should just pass both toEdgeGrabSampler
, andEdgeGrabSampler
should take them as an argument of its initializer and handle them within it. -
Regarding the conflict between
two_qubit_gate_density
(ortwo_qubit_gate
) andsampler_opts
, I think it's sufficient forMirrorRB
to overwritetwo_qubit_gate_density
withsampler_opts={"two_qubit_gate_density": custom_val}
if provided. I don't thinkgate_distribution
is suitable forEdgeGrabSampler
initializer's argument. Regarding unnecessary options for a custom sampler, I think it could be passed to the custom sampler and gracefully ignored.CustomSampler.__init__(self, required_opt, **extra_sampler_opts)
(See alsoSingleQubitSampler
in the link below).
My preferable API at this moment looks like this.
(If you don't like to require **extra_sampler_opts
in sampler initializers, you can avoid that by checking the signature of a sampler's initializer in MirrorRB.__init__
and passing only valid sampler options to the sampler.)
Thanks for writing up the API!
Regarding 1., that is a nice solution, will do. I don't think pauli_randomize
is a relevant option for direct RB (at least the standard version) if someone wants to implement that in the future, but it's fine since we're heading in the general direction of moving complexity into the sampler class.
Regarding 2., Without gate_distribution
for EdgeGrabSampler
, there won't be a way to use a custom gate distribution easily, such as 90% CX gate and 10% ECR gate or some custom two-qubit gate the user wants to try. I was thinking that because it's easy for us to expose the distribution, we might want to keep it as an option instead of making the user write a custom class. But maybe there's no good use case for X% of one gate and Y% of another? I'm not really sure.
I don't think of any use case for X% of one gate and Y% of another so far. Also I'm not sure if users (including I) can understand correctly what happens if they supply gate_distribution
since the spec of EdgeGrabSampler
already looks complicated to me even without gate_distribution
.
I think it would be better to keep EdgeGrabSampler
simpler at this moment and consider such an extension after we confirm it is certainly necessary.
One example could be the universal mirror RB with SU2 gates protocol: the paper uses {CSGate
, CSdgGate
} as the two qubit gate set with the edge grab sampler. I'm okay with keeping the interface simple for now and extending it later.
Oh, I missed that case, but just for covering the case, we could extend two_qubit_gate
option to be able to take multiple 2q-gates. I think we can defer such an extension to a follow-up.