Clifford Mirror RB experiment

[coruscating]

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 Qiskit/qiskit#9475 is merged, the redundant circuit generation step can be removed (thanks to @itoko for helping with this).

[nkanazawa1989]

Thanks Helena. This version looks much cleaner than old one. Probably I added too much comments.

[nkanazawa1989]

Can we apply the same logic to MRB alpha?

[coruscating]

I think this comment doesn't apply any more now that the analysis class is subclassing from RBAnalysis?

[nkanazawa1989]

Nope. Option and _create_analysis_results are still inherited from RBAnalysis and an algorithm to exclude 1q contribution is still applied here. I mathematically validated this exclusion for standard RB case, but I'm not sure if it's still valid for MRB alpha. This mechanism should be validated otherwise you should drop these options.

[nkanazawa1989]

Why you don't have a test for other y_axis? Sorry if I overlooked.

[itoko]

Thank you for the updates, @coruscating . Now it looks better to me but I think we can improve the API of Samplers a bit more.
IMO, MirrorRB class should not be aware of GateDistribution, which should be a helper class to implement Samplers. And I think it's too demanding for users to ask to implement _set_distribution_options (i.e. subclass MirrorRB class) for all samplers other than EdgeGrabSampler. I also think BaseSampler should have no implementation, so it should have only __init__ and __call__.
The psuede code in my mind looks like (updated March 30):

# in MirrorRB.__init__
sampler_opts.update({"backend": backend, "physical_qubits": physical_qubits, "seed": seed})
self._distribution = self.sampler_map.get(sampling_algorithm)(**sampler_opts)
# BaseSampler
def __init__():
    # do nothing
# EdgeGrabSampler
def __init__(seed=None, physical_qubits=None, backend=None, reduced_coupling_map=None, **sampler_opts):
    # Move logics currently in MirrorRB._set_distribution_options here, e.g.
    if backend:
        coupling_map = CouplingMap(backend.coupling_map)
    else:
        coupling_map = CouplingMap.from_full(len(physical_qubits))
    self._coupling_map = coupling_map.reduce(physical_qubits)
    ...
# SingleQubitSampler
def __init__(seed=None, gate_distribution=None, **sampler_opts):

Also why don't we have Pauli1QSampler and Clifford1QSampler, which are dedicated samplers for 1Q Paulis and 1Q Cliffords? If we have those, we don't need to introduce GenericClifford, GenericPauli and SimpleQubitSample._probs_by_gate_size.

[itoko]

Why don't we have Pauli1QSampler and Clifford1QSampler, which are dedicated samplers for 1Q Paulis and 1Q Cliffords? If we have those, we can simply write pauli_sampler=Pauli1QSampler(seed=self.experiment_options.seed) here and readers don't need to understand the spec of GateDistribution and GenericPauli.

[itoko]

I feel having this and introducing sampler_opts is a good idea.

[nkanazawa1989]

💯

[nkanazawa1989]

You need to catch key error with friendly message. get without default value is just directly accessing the attribute.

[nkanazawa1989]

Fair enough. Let's do in follow-up.

[nkanazawa1989]

Cool. It looks clean now 💯

[nkanazawa1989]

See #1090 (comment)

[nkanazawa1989]

Nope. Option and _create_analysis_results are still inherited from RBAnalysis and an algorithm to exclude 1q contribution is still applied here. I mathematically validated this exclusion for standard RB case, but I'm not sure if it's still valid for MRB alpha. This mechanism should be validated otherwise you should drop these options.

[coruscating]

@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.

[itoko]

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.

[nkanazawa1989]

Thanks for the update. Now analysis class looks much better. Just minor comment and I'll delegate approval to @itoko

[itoko]

Aside from requiring sampler's options at initialization or allowing their replacement layer, my point is that users (not MirrorRB class) should handle Sampler's construction completely. (Note that Sampler's interface is very important because the optionality of benchmarking circuits is one of the selling points of Mirror RB.)

For that, a naive interface would look like MirrorRB(sampler=MySampler(a, b, c, x, y, z), x=x, y=y, z=z)) but we don't want to ask users to set the duplicated options (x, y, z) twice. So current interface like MirrorRB(sampler_algorithm="my_sampler", sampler_opts={a: a, b: b, c: c}, x=x, y=y, z=z) (or MirrorRB(sampler=MySampler, sampler_opts={a: a, b: b, c: c}, x=x, y=y, z=z)) looks good to me. I recognize that is a good "syntax sugar" of the former. That means, in principle, users need to set full options for Sampler by themselves (via sampler_opts) but they don't need to specify options duplicated with arguments of MirrorRB initializer exceptionally. Consequently, I think MirrorRB class should not do anything more than sampler_opts.update({mirror_rb_initializer_args}) and sampler_map[sampler_algorithm](**sampler_opts), and all objects necessary for a Sampler (e.g. reduced coupling map) should be provided via sampler_opts or created within the Sampler (not in MirrorRB). (See also the sample code in the previous comment I updated.) What do you think? @coruscating
I think current code is heading towards the direction and almost there but some codes in MirrorRB seem still do what should be done in Sampler, i.e. _set_distribution_options.

Added later:
Alternative design of MirrorRB initializer might be having sampler: Optional[BaseSampler] = None, building EdgeGrabSampler in the case of None and allowing to reset sampler via experiment.sampler = MySampler(...). In this case, we don't need to have sampler_map and sampler_opts. Concerns on serialization make difficult to take this simpler design?

Anyway, if a user wants to reset backend or any experiment options which are used for building a sampler, it should be the user's responsibility to reset sampler at the same time. I think it's not demanding since it can be usually avoided by replacing

exp = Experiment()
for config in configs:
   exp.set_experiment_options(config=config)
   exp.run(...)

with

for config in configs:
   exp = Experiment(config=config)
   exp.run(...)

[itoko]

Any thoughts on this point @coruscating ?

[itoko]

I suggest to replace this method with just a getter and a setter for distribution (any better name? layer_sampler?) and move all logics in this method under EdgeGrabSampler. (as I mentioned the reason in the main comment)

[coruscating]

@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.

[itoko]

Thank you for providing detailed issues.

1. Regarding the coupling of two_qubit_density and pauli_randomize, I think MirrorRB should just pass both to EdgeGrabSampler, and EdgeGrabSampler should take them as an argument of its initializer and handle them within it.

2. Regarding the conflict between two_qubit_gate_density (or two_qubit_gate) and sampler_opts, I think it's sufficient for MirrorRB to overwrite two_qubit_gate_density with sampler_opts={"two_qubit_gate_density": custom_val} if provided. I don't think gate_distribution is suitable for EdgeGrabSampler 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 also SingleQubitSampler 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.)

[coruscating]

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.

[itoko]

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.

[coruscating]

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.

[itoko]

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.

[CLAassistant]

All committers have signed the CLA.