qiskit-experiments icon indicating copy to clipboard operation
qiskit-experiments copied to clipboard

Clifford Mirror RB experiment

Open coruscating opened this issue 1 year ago • 10 comments

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

coruscating avatar Mar 17 '23 09:03 coruscating

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

coruscating avatar Mar 23 '23 05:03 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.

coruscating avatar Mar 28 '23 15:03 coruscating

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 avatar Mar 28 '23 15:03 itoko

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

coruscating avatar Mar 30 '23 15:03 coruscating

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

itoko avatar Mar 31 '23 03:03 itoko

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.

coruscating avatar Apr 04 '23 02:04 coruscating

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.

itoko avatar Apr 17 '23 08:04 itoko

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.

coruscating avatar Apr 17 '23 13:04 coruscating

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.

itoko avatar Apr 19 '23 03:04 itoko

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Jul 18 '23 13:07 CLAassistant