Clifford Mirror RB experiment #1090

coruscating · 2023-03-17T09:02:53Z

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

…ircuits, comment pauli label

…release note

…in tests

…tream main

the default integer method wasn't working with the later `Clifford()` invocation.

nkanazawa1989

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

docs/manuals/verification/mirror_rb.rst

releasenotes/notes/mirror-rb-ec4d695a9a923971.yaml

qiskit_experiments/library/randomized_benchmarking/sampling_utils.py

qiskit_experiments/library/randomized_benchmarking/mirror_rb_analysis.py

nkanazawa1989 · 2023-03-27T09:20:03Z

qiskit_experiments/library/randomized_benchmarking/mirror_rb_analysis.py

+
+        # Calculate EPG
+        if self._gate_counts_per_clifford is not None and self.options.gate_error_ratio:
+            epg_dict = _calculate_epg(


Can we apply the same logic to MRB alpha?

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

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.

qiskit_experiments/library/randomized_benchmarking/mirror_rb_analysis.py

test/library/randomized_benchmarking/test_randomized_benchmarking.py

nkanazawa1989 · 2023-03-27T09:29:15Z

test/library/randomized_benchmarking/test_randomized_benchmarking.py

+
+
+@ddt
+class TestRunMirrorRB(RBRunTestCase):


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

now `sampling_algorithm` specifies the algorithm in str form and `sampler_opts` is passed at sampler instantiation. two-qubit gate and density are only passed to the default edge grab sampler, otherwise users have to implement their own methods. also addressed some review comments.

also updated test backends to V2.

Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com>

qiskit_experiments/library/randomized_benchmarking/mirror_rb_experiment.py

and added validation for analysis option

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.

qiskit_experiments/library/randomized_benchmarking/mirror_rb_experiment.py

itoko · 2023-03-28T14:18:44Z

qiskit_experiments/library/randomized_benchmarking/mirror_rb_experiment.py

+            pauli_sampler = SingleQubitSampler(seed=self.experiment_options.seed)
+            pauli_sampler.gate_distribution = [GateDistribution(prob=1, op=GenericPauli(1))]
+
+        if self.experiment_options.start_end_clifford:
+            clifford_sampler = SingleQubitSampler(seed=self.experiment_options.seed)
+            clifford_sampler.gate_distribution = [GateDistribution(prob=1, op=GenericClifford(1))]


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 · 2023-03-28T14:22:26Z

qiskit_experiments/library/randomized_benchmarking/mirror_rb_experiment.py

+
+    """
+
+    sampler_map = {"edge_grab": EdgeGrabSampler, "single_qubit": SingleQubitSampler}


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

nkanazawa1989 · 2023-03-28T14:07:17Z

qiskit_experiments/library/randomized_benchmarking/sampling_utils.py

+        try:
+            self._coupling_map = CouplingMap(coupling_map)
+        except (ValueError, TypeError) as exc:
+            raise TypeError("Invalid coupling map provided.") from exc


qiskit_experiments/library/randomized_benchmarking/sampling_utils.py

nkanazawa1989 · 2023-03-28T14:44:18Z

qiskit_experiments/library/randomized_benchmarking/mirror_rb_experiment.py

+            inverting_pauli_layer=inverting_pauli_layer,
+        )
+
+        self._distribution = self.sampler_map.get(sampling_algorithm)(seed=seed, **sampler_opts)


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

nkanazawa1989 · 2023-03-28T14:49:18Z

qiskit_experiments/library/randomized_benchmarking/mirror_rb_experiment.py

+        physical_qubits: Sequence[int],
+        lengths: Iterable[int],
+        distribution: BaseSampler = EdgeGrabSampler,
+        start_end_clifford: bool = True,


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

nkanazawa1989 · 2023-03-28T14:56:06Z

qiskit_experiments/library/randomized_benchmarking/mirror_rb_analysis.py

+                target_bs.append(circ_result["metadata"]["target"])
+
+        self.set_options(
+            data_processor=DataProcessor(


Cool. It looks clean now 💯

nkanazawa1989 · 2023-03-28T14:56:18Z

qiskit_experiments/library/randomized_benchmarking/mirror_rb_analysis.py

+        )
+
+
+class ComputeQuantities(DataAction):


See #1090 (comment)

nkanazawa1989 · 2023-03-28T15:00:31Z

qiskit_experiments/library/randomized_benchmarking/mirror_rb_analysis.py

+
+        # Calculate EPG
+        if self._gate_counts_per_clifford is not None and self.options.gate_error_ratio:
+            epg_dict = _calculate_epg(


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 · 2023-03-28T15:15:07Z

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

- protect data processor node - moved edge grab distribution validation to setter - fixed tests and added invalid distribution tests

itoko · 2023-03-28T15:50:03Z

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 · 2023-03-30T06:57:33Z

qiskit_experiments/library/randomized_benchmarking/mirror_rb_experiment.py

+_self_adjoint_gates = [CXGate, CYGate, CZGate, ECRGate, SwapGate, iSwapGate]
+
+
+class MirrorRB(StandardRB):


Any thoughts on this point @coruscating ?

itoko · 2023-03-30T07:03:37Z

qiskit_experiments/library/randomized_benchmarking/mirror_rb_experiment.py

+    def _set_distribution_options(self):
+        """Set the coupling map and gate distribution of the sampler
+        based on experiment options. This method is currently implemented
+        for the default "edge_grab" sampler."""


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 · 2023-03-30T15:29:20Z

@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 · 2023-03-31T03:41:59Z

Thank you for providing detailed issues.

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.
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 · 2023-04-04T02:19:27Z

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 · 2023-04-17T08:41:41Z

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 · 2023-04-17T13:30:35Z

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 · 2023-04-19T03:28:38Z

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 · 2023-07-18T13:26:22Z

All committers have signed the CLA.

albertzhu01 and others added 30 commits March 11, 2023 11:21

add mirror RB experiment and analysis files

921c185

edit appropriate mirrorRB-relevant files for PR

9cd7074

remove prints and add paper ref

2de778b

update docstrings

a28e6d2

delete commented series code

9d440e8

fix pygsti version

8d656b6

remove outcomes setting

b0432fd

cleaned RB tutorial

e1de0e4

move coupling map outside loop, delete elements_without_inv and int_c…

859a94e

…ircuits, comment pauli label

reformat files with black and lint

c1444ae

moved Clifford sampling in edgegrab algo

6857a48

use full connectivity for coupling map if coupling map is not provided

8445f4a

changed AnalysisResult imports

63f171c

edited BitGenerator and SeedSequence imports

bbf111f

removed MirrorRBPyGSTi from library init file

2308072

fix full connectivity map generation code

ebfa4d6

remove MirrorRBPyGSTi from mirror_rb_experiment.py, rb tutorial, and …

de33876

…release note

fix coupling map full connectivity generation and update aer imports …

6fccca1

…in tests

reformat imports (temporarily) to pass lint with merged code from ups…

d2f8ff9

…tream main

merged main branch

a7b7a70

added edge grab docstring

20b36db

moved sampler to its own file

c8c8a7b

updated edgegrab

ea53a5d

coupling map

07b4b28

updated release note

31f565e

partial refactoring

41f9e81

fix rebase errors

1f58ffb

update docs and remove edgegrab from utils

961068d

changed 1Q from integer to clifford

7d79339

the default integer method wasn't working with the later `Clifford()` invocation.

merge main

e82df72

coruscating added 3 commits March 26, 2023 02:43

change sampler output to list of named tuple

4951cb1

change gate distribution to named tuple

585c053

merge main

c209d1e

nkanazawa1989 requested changes Mar 27, 2023

View reviewed changes

coruscating and others added 5 commits March 27, 2023 15:12

Merge remote-tracking branch 'upstream/main' into mirrorrb-rebased

3070431

subclassMirrorRBAnalysis from RBAnalysis

d423e58

also updated test backends to V2.

change y-axis option to analyzed_quantity

b71d098

changed to CouplingMap type and added validation

0510478

Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com>

itoko reviewed Mar 28, 2023

View reviewed changes

qiskit_experiments/library/randomized_benchmarking/mirror_rb_experiment.py Show resolved Hide resolved

coruscating added 2 commits March 28, 2023 00:32

added data processor node for analysis

1c98049

and added validation for analysis option

change sampler to return a generator

fb41d6d

itoko requested changes Mar 28, 2023

View reviewed changes

nkanazawa1989 reviewed Mar 28, 2023

View reviewed changes

Address review comments and fix tests

7710f7c

- protect data processor node - moved edge grab distribution validation to setter - fixed tests and added invalid distribution tests

coruscating modified the milestones: Release 0.5, Release 0.6 Mar 28, 2023

nkanazawa1989 reviewed Mar 28, 2023

View reviewed changes

itoko reviewed Mar 30, 2023

View reviewed changes

coruscating removed this from the Release 0.6 milestone Jan 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clifford Mirror RB experiment #1090

Clifford Mirror RB experiment #1090

coruscating commented Mar 17, 2023 •

edited

nkanazawa1989 left a comment

nkanazawa1989 Mar 27, 2023

coruscating Mar 28, 2023

nkanazawa1989 Mar 28, 2023

nkanazawa1989 Mar 27, 2023

itoko left a comment •

edited

itoko Mar 28, 2023 •

edited

itoko Mar 28, 2023

nkanazawa1989 Mar 28, 2023

nkanazawa1989 Mar 28, 2023

nkanazawa1989 Mar 28, 2023

nkanazawa1989 Mar 28, 2023

nkanazawa1989 Mar 28, 2023

nkanazawa1989 Mar 28, 2023

coruscating commented Mar 28, 2023

itoko commented Mar 28, 2023 •

edited

nkanazawa1989 left a comment

itoko left a comment •

edited

itoko Mar 30, 2023

itoko Mar 30, 2023

coruscating commented Mar 30, 2023

itoko commented Mar 31, 2023 •

edited

coruscating commented Apr 4, 2023

itoko commented Apr 17, 2023

coruscating commented Apr 17, 2023 •

edited

itoko commented Apr 19, 2023

CLAassistant commented Jul 18, 2023 •

edited


		"""

		sampler_map = {"edge_grab": EdgeGrabSampler, "single_qubit": SingleQubitSampler}

		_self_adjoint_gates = [CXGate, CYGate, CZGate, ECRGate, SwapGate, iSwapGate]


		class MirrorRB(StandardRB):

Clifford Mirror RB experiment #1090

Are you sure you want to change the base?

Clifford Mirror RB experiment #1090

Conversation

coruscating commented Mar 17, 2023 • edited

Summary

Details and comments

nkanazawa1989 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itoko left a comment • edited

Choose a reason for hiding this comment

itoko Mar 28, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coruscating commented Mar 28, 2023

itoko commented Mar 28, 2023 • edited

nkanazawa1989 left a comment

Choose a reason for hiding this comment

itoko left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coruscating commented Mar 30, 2023

itoko commented Mar 31, 2023 • edited

coruscating commented Apr 4, 2023

itoko commented Apr 17, 2023

coruscating commented Apr 17, 2023 • edited

itoko commented Apr 19, 2023

CLAassistant commented Jul 18, 2023 • edited

coruscating commented Mar 17, 2023 •

edited

itoko left a comment •

edited

itoko Mar 28, 2023 •

edited

itoko commented Mar 28, 2023 •

edited

itoko left a comment •

edited

itoko commented Mar 31, 2023 •

edited

coruscating commented Apr 17, 2023 •

edited

CLAassistant commented Jul 18, 2023 •

edited