[WIP] New algorithm for RB using transpiled Cliffords by merav-aharoni · Pull Request #851 · qiskit-community/qiskit-experiments

merav-aharoni · 2022-07-19T12:49:20Z

Summary

We implement a new algorithm for RB, 1 and 2 qubits.

Details and comments

Here are the main ideas behind the implementation:

We transpile all the Cliffords in advance and store them in a file. The file that generates the transpiled Cliffords is qiskit_experiments/library/randomized_benchmarking/generate_transpiled_circuits.py. The transpiled Cliffords are stored in the same directory, in a file named transpiled_circs_[1|2]_<basis_gates>.qpy.
Every Clifford is represented by a number. We store a list of the compositions of Cliffords represented as numbers. For example, if Clifford1.compose(Clifford2) == Clifford3, then we conceptually, we store {(1, 2) : 3}.
Similarly, we store for each number representing a Cliffords, the number representing the inverse Clifford. This data, along with the data in (2) is stored in the file qiskit_experiments/library/randomized_benchmarking/clifford_data.py. The data is generated by the file, in the same directory, create_clifford_map.py.
For (2) above, we don't actually store the map, but only the results of the compose in an array. This is more efficient in performance. The result is found using the indices of the input Cliffords. Similarly, for (3) we also only store the array of inverse numbers.
For the compose, we don't actually store the full compose table of all-cliffords X all-cliffords. Instead, we define an array of single-gate-cliffords. This comprises all Cliffords that consist of a single gate. These arrays are stored in clifford_data.py as well. There are 8 such Cliffords for 1-qubit, and 20 such Cliffords for 2-qubits. It is sufficient to store the compose table of all-cliffords X single-gate-cliffords, since for every clifford on the right hand side, we can break it down into single gate Cliffords, and do the composition one at a time. This greatly reduces the storage space for the array of composition results (from O(n^2) to O(n)).
We currently support two sets of basis gates: {rz, sx, cx} and {s, h, x, cx}. To support a new set of basis gates, one must first generate the two files mentioned above.

This PR supersedes PR #825. Therefore I will close that PR.

…enerate once all 24 transpiled Cliffords. Then, for every rb_circuit, select Cliffords at random and compose them to a circuit

…. Added parameter to all calls to compose to use inplace=True. Removed redundant method generate_all_transpiled_clifford_circuits

…use front=True, because I assume front=False when creating the circuits

… the previous version of rb_experiment.

New algorithm for generating Clifford circuits for single qubit.

…cuits, generate_1q_transpiled_clifford_circuits

…ling_single_qubit

… otherwise randomization is not identical in the two experiments

…ing called by the child class CurveAnalaysis. Added parameter to rb_experiment to determine whether to use the old algorithm or new one

…s change regarding _format_data

…eters for test_full_sampling_single_qubit

…lse, so that all tests will pass

…ding support for interleave

…_qubit_clifford

…circuit

…cuits. Transformed interleaved element into a transpiled clifford circuit. Added relevant tests

…lement

merav-aharoni · 2022-07-25T16:35:06Z

@ShellyGarion , I believe I addressed all your comments, except for one where I asked a question. Please review again, in particular the new tests.

nkanazawa1989

Thanks for proposing new framework of performant RB circuit generation. Seems like the speedup is very promising, but we should improve the implementation from software design viewpoint.

First of all, new framework introduce tight coupling to the file system, and it generates cache files inside the software (this must be avoided). You should at least use a temp location provided by the operating system, and also should be able to create multiple cache files for different basis gates -- if we really want to and decide to rely on such non in-memory cache mechanism. Apart from this, you should be careful not to break conventional workflow (requiring pre cache generation), and not to restrict capability of experiment (i.e. <2Q).

Perhaps I would employ python descriptor to implement such mechanism and let the descriptor generate cache data when an element is called for the first time. Anyways, I suggest you to start from writing a design doc so that we can be on the same page before reviewing a huge PR. Here is the public Qiskit RFCs and I think this can be discussed publicly.

nkanazawa1989 · 2022-07-26T18:00:37Z


-    def clifford_1_qubit(self, num):
+    @classmethod
+    def clifford_1_qubit(cls, num):


I was planning to deprecate CliffordUtils because it uses a class just as a namespace and doesn't make any difference from defining a set of functions in a module unless you need to define a subclass for a particular RB experiment.

We could remove CliffordUtils as a class, and just keep the methods. I think it is nicer and more expressive to have all these methods grouped in a class, to indicate the common functionality.

When the method does not access any class-relevant data (i.e. doesn't use the cls parameter) you can make is a @staticmethod instead.

Right, but still these are just a collection of functions. I wonder why these must be class methods, i.e.

from qiskit_experiments.library.randomized_benchmarking import clifford_utils clifford_utils.clifford_1_qubits(...)

v.s.

from qiskit_experiments.library.randomized_benchmarking.clifford_utils import CliffordUtils CliffordUtils.clifford_1_qubits(...)

nkanazawa1989 · 2022-07-26T18:08:41Z

        (2, 2, 3, 3, 4, 4),
    ]
+    GENERAL_CLIFF_LIST = ["id", "h", "sdg", "s", "x", "sx", "sxdg", "y", "z", "cx"]
+    TRANSPILED_CLIFF_LIST = ["sx", "rz", "cx"]


This assumes a particular IBM-ish backend. Other providers may use different basis set depending on their hardware architecture, or, even us may want to use different set, e.g. "ecr" rather than "cx" which are locally equivalent.

It is difficult to make the code efficient without optimizing it for a specific set of basis gates.
For non-IBM backend, one can change the basis gates, and pre-generate the data files (there is a code for this in generate_transpiled_circuits.py).
As for an ecr gate, it is currently not one of the gates that the Clifford class can handle:
https://qiskit.org/documentation/stubs/qiskit.quantum_info.Clifford.html

Right, but Qiskit is backend agnostic. This violates very important policy of Qiskit.

nkanazawa1989 · 2022-07-26T18:15:22Z

+            clifford_single_gate_to_num[(gate.name, qubit_as_str)] = num
+        else:
+            print("not found")
+    file.write(f"CLIFF_SINGLE_GATE_MAP_1Q = {clifford_single_gate_to_num}\n")


Seems like file is not defined within current scope. Also you should NOT induce strong coupling to file system. This makes code unreadable, and also hardly guarantees multiple platform support.

By the way is there any special reason to generate text data? This is expensive to load because of the deserialization overhead.

Do you think it would be better if the user defined the path to the required files?

What do you suggest instead of text data?

Some tmp file dir, or qiskit experiments data dir (e.g. in app data in Mac) -- at least somewhere not in the package. Note that qiskit provider code is kind of doing this, i.e. it saves token. However this should be usually avoided (local file system).

Depends on data structure.

nkanazawa1989 · 2022-07-26T18:22:19Z

+        self._transpiled_cliff_circuits = {}
+        # transpiled clifford circuits for 1 and 2 qubits respectively
+        self._transpiled_cliff_circuits[1] = None
+        self._transpiled_cliff_circuits[2] = None


This is also problematic because in principle we should be able to run 3Q+ RB though its outcome might not be statistically confident.

I could of course define this as an array. But I preferred to write it this way to stress that for now only 1 and 2 qubit -rb are supported. Many changes will be needed to support more than 2 qubits. One option is to keep the legacy code for 3 or more qubits, but I am not sure if that code worked for more than 2 qubits. @ShellyGarion - do you know if the previous version worked in this case?

Then this should be implemented as a subclass of StandardRB, i.e. StandardRB1Q and StandardRB2Q (because constructor argument is qubits: Sequence[int]). However, such subclasses still make the RB experiment different from standard execution model.

nkanazawa1989 · 2022-07-26T18:25:40Z

+        )
+        n = self.num_qubits
+        if self._transpiled_cliff_circuits[n] is None:
+            if os.path.isfile(transpiled_circs_file):


This is no longer our standard experiment workflow. This seems like a research code, i.e. framework is performant but dedicated to very limited use case. We should be able to create circuit without caching. For example, we may run this experiment with multiple backends with different basis set.

ShellyGarion

The main idea behind this improvement seems to be correct.
I would still suggest to add some more tests the data in clifford_data.py has been generated correctly and satisfies the group properties.

In CLIFF_COMPOSE_DATA_1Q each row and each column is a permutation of [0,...,23]
CLIFF_INVERSE_DATA_1Q is a permutation of [0,...,23]
In CLIFF_COMPOSE_DATA_2Q each row is a permutation
CLIFF_INVERSE_DATA_2Q is a permutation

merav-aharoni · 2022-07-27T08:52:15Z

Hi @nkanazawa1989 , thank you for your careful comments. As you suggested, I will start by writing a design doc, along with some of the questions you raised, which are also questions I had, in particular with regard to storing data in files and regarding the generality of the code. This way we can discuss these issue in a wider forum. I don't think https://github.com/Qiskit/rfcs is the right place for this document, as the specification there is for " 'substantial' changes to the Qiskit meta-package". I recall we once had a place (in Box?) for such documents. Does anyone recall where that is?

gadial · 2022-07-27T09:33:20Z

+
+    # basis_gates must be set for randomized benchmarking
+    transpiler_options = {
+        "basis_gates":  ["rz", "sx", "cx"],


Why not use this set as a default if the user does not pass this option?

It is possible. The question is whether we think it is better to have a default, or to make sure the user specifies their needs. @nkanazawa1989 , @ShellyGarion ? what do you think?

RB experiment itself should be general. Experiment must define a protocol, not executable (target) code.

gadial · 2022-07-27T09:36:33Z


-    def clifford_1_qubit(self, num):
+    @classmethod
+    def clifford_1_qubit(cls, num):


When the method does not access any class-relevant data (i.e. doesn't use the cls parameter) you can make is a @staticmethod instead.

gadial · 2022-07-27T10:29:13Z

+        name = inst.name
+        gates_with_delay = basis_gates.copy()
+        gates_with_delay.append("delay")
+        single_gate_map = (


A slightly more elegant way of doing this:

cliff_single_gate_maps = {1: CLIFF_SINGLE_GATE_MAP_1Q, 2: CLIFF_SINGLE_GATE_MAP_2Q} single_gate_map = cliff_single_gate_maps[rb_num_qubits]

Answering to the comment above regarding @staticmethod: this method actually calls another class method: cls.clifford_1_qubit_circuit(num) so I think it must be a classmethod. Or am I missing something? I did change a couple of other methods to static, as you suggested.

Regarding your second comment - I agree. Nicer!

gadial · 2022-07-27T10:31:10Z

+        if set(basis_gates).issubset(set(cls.TRANSPILED_CLIFF_LIST)):
+            if name in {"sx", "cx"}:
+                map_index = name
+            elif name == "delay":


You can do this check once in the beginning of the method

gadial · 2022-07-27T10:37:30Z

+            suffix += "_" + basis_gates[-1]
+        circs_file_name = "/transpiled_circs_" + str(num_qubits) + "q" + suffix + ".qpy"
+        root_dir = os.path.dirname(os.path.abspath(__file__))
+        transpiled_circs_file = root_dir + circs_file_name


Using os.path.join() is considered more robust than using +

gadial · 2022-07-27T10:40:32Z

+            num = cliff_to_num_2q[cliff.__repr__()]
+            # qubit_as_str is not really necessary. It is only added to be consistent
+            # with the representation for 2 qubits
+            qubit_as_str = "[" + str(qubit) + "]"


can also do qubit_as_str = f"[{qubit}]"

gadial · 2022-07-27T10:42:35Z

+        cliff = cliff1.adjoint()
+        invs[i] = cliff_to_num_1q[cliff.__repr__()]
+
+    file.write("CLIFF_COMPOSE_DATA_1Q = [")


even if storing text and not bytes (bytes might be better) you can use json.dump instead of manually writing what is essentially a json data file.

gadial · 2022-07-27T10:49:08Z

+        max_qubit = max(self.physical_qubits) + 1
+        all_rb_circuits = []
+
+        if is_interleaved:


This breaks the current structure where StandardRB did not know about InterleavedRB. Can't say if it's good or bad, but I usually prefer ensuring classes know as little about each other as possible (so if someone wants to understand interleaved RB, all the relevant interleaved logic will be in the interleaved rb file).

I agree with this comment. I also was not sure if this was best. The other option would be to copy the _build methods to InterleavedRB. There would be a lot of code duplication, because I didn't see any reasonable way to break these down into smaller methods.
But if you think this is better, I will make this change.

merav-aharoni · 2022-07-27T11:31:54Z

Based on @nkanazawa1989 's comments regarding usage of files, I suggest the following: we add two parameters to StandardRB (and to interleavedRB): transpiled_cliffords_file and clifford_data_file. The user will specify the path for the relevant files in these parameters. For each of these files, if it exists, the code will use it, and if it doesn't exist, the code will invoke the relevant script to create the file using the basis_gates. These parameters will be mandatory. We will provide the default versions for these files as they are now.
I would appreciate your input, @nkanazawa1989 , @ShellyGarion , @gadial .

…_single_clifford and use transpile() directly instead

…INVERSE_DATA

nkanazawa1989 · 2022-07-27T16:51:27Z

I still don't like the idea of using local files. I think current issues are

Partly transpiled circuit data set must be pre-generated for performance, i.e. this is virtual extension of basis gates. This is no longer the standard experiment workflow and this turns the RB experiment into a special case. This means a user needs extra learning for this particular experiment, and one may hesitate migrating from Ignis to QE.
Generated data must be stored for next execution, preferably the data must live after program is terminated. For example, you may run the experiment multiple times with automated system, e.g. crontab/shell script in linux. However, this file system coupling increases test complexity and also maintenance overhead.
New mechanism must assume a particular basis gates to pre-generate the virtual basis gates. This tightly couples experiment to a particular target backend.

I would switch to the StagedPassManager and implement the backend specific transpile routine in the qiskit ibm provider. This allows you to assume IBM-specific basis gates, and storing pre generated circuit QPY as a part of the qiskit ibm provider package. Then, QE SRB can call IBM provider's RB-transpiler if the backend is provided by the IBM provider. Otherwise, it will call legacy transpile routine.

I feel this discussion is no longer a part of code review. I hope you will write a design doc (before continue the code cleanup), so that we can first get concrete design to implement.

(EDIT)

I don't complain the general idea of how it works. I think this is reasonable and scalable approach. The point is how we "standardize" the workflow of all built-in experiments.

itoko · 2022-07-28T10:19:25Z

I don't fully understand the background of this PR, but it seems to me that this PR address a performance issue on RB experiments. Is there any profile data that shows which part is the bottleneck in the current code? I think we need to discuss what approach we should take based on that.

yaelbh · 2022-07-28T10:52:48Z

As we've already discussed in various contexts, an experiment has several different functionalities: build (transpiled or non-transpiled) circuits, run them, analyze, save to the database. We've already noticed that the circuit building should become more independent (beyond override of the _transpiled_circuits method). By "more indpendent", I mean the ability of a user to specify the (usually transpiled) circuits as an input to the run. Then I'd imagine some procedure that looks roughly like this:

Outside of the experiment flow, outside of BaseExperiment.run and everything - a user saves whatever is required for her to generate transpiled RB circuits (can be e.g. the circuits themselves, or Clifford decompostions). She does it in her file system, in the location that she chooses. She can write her own code for this, and also there can be code in qiskit-experiments to help her do it, but probably as some utility and not as part of BaseExperiment or even of StandardRB. The qiskit-experiments code can refrain from writing to files; instead it returns Python objects, which the user can dump by herself.
The user generates the transpiled circuits. This again happens outside of the main flow, and can even be done by code that the user writes.
The user runs the experiment, with the transpiled circuits specified in the input.

Other places where we've recently encountered the same topic:

The request for modularity came up, to my understanding, in conversations of @chriseclectic with users.
PR [WIP] Add caching of transpiled circuit generation to BaseExperiment #815 that @chriseclectic started and @ItamarGoldman is about to continue - about circuit caching. Note by the way that Haggai and I would like a follow-up PR to save the cache to a file, which can be loaded later. So we see that not only circuit caching - also file handling is a recurring issue.
@ItamarGoldman has suggested to pre-transpile say 1000 RB circuits, then in every execution sample one of them.
In the whole-backend PR [WIP] Functions to facilitate experiments on entire backends #859, I build something I've named BasicExperiment, which is an experiment whose only purpose is to store transpiled circuits.

merav-aharoni · 2022-07-28T10:54:58Z

Hi @itoko , thanks for looking at this issue. Profiling results for the existing rb can be seen in https://github.ibm.com/MERAV/prof_qiskit_exps/tree/main/profiling/output.
I presented these results in one of the experiments squad meeting. The main bottleneck was identified as transpile. This solution was also discussed in one of the squad meetings. I can try to find the recording, if you are interested.

yaelbh · 2022-07-28T12:26:43Z

To my previous comment, add:
5. @coruscating has asked, in the context of QV: "Would be nice to have standalone functions in experiments to help with playing around with the data (for example method to allow loading custom circuits while keeping analysis the same)".

…ents into 2_qubits_rb

merav-aharoni · 2022-10-03T10:40:02Z

Superseded by https://github.com/Qiskit/qiskit-experiments/tree/feature/rb_speedup.

merav-aharoni and others added 30 commits May 25, 2022 20:50

New algorithm for generating Clifford circuits for single qubit. We g…

9d81eed

…enerate once all 24 transpiled Cliffords. Then, for every rb_circuit, select Cliffords at random and compose them to a circuit

Changed basis for transpilation to match the one in single_qubit_test…

c836831

…. Added parameter to all calls to compose to use inplace=True. Removed redundant method generate_all_transpiled_clifford_circuits

Modified the methods in create_clifford_map so that compose does not …

86cad33

…use front=True, because I assume front=False when creating the circuits

Changed generation of generation of random numbers to be identical to…

9a873dc

… the previous version of rb_experiment.

Test to run on device

76ff366

Merge pull request #2 from merav-aharoni/rb_performance

ab7d4f3

New algorithm for generating Clifford circuits for single qubit.

added methods for new algorithm to generate rb circuits: build_rb_cir…

274432a

…cuits, generate_1q_transpiled_clifford_circuits

Added the method _layout_for_rb_single_qubit and added test_full_samp…

890d890

…ling_single_qubit

In test_full_sampling_single_qubit fixed num_samples to be 1, because…

8dc9aa1

… otherwise randomization is not identical in the two experiments

Tidied up build_rb_circuits

f0c3cc8

Added documentation and moved methods

10a7e9f

Added test_single_qubit_parallel

aea856a

Merge branch 'main' into transpiled-rb

eec3721

Changed name _format_data to format_data because the method wasn't be…

dd0b62b

…ing called by the child class CurveAnalaysis. Added parameter to rb_experiment to determine whether to use the old algorithm or new one

Fixed handling of num_samples>1. Cleaned out prints. Reverted previou…

b9c9f41

…s change regarding _format_data

Added assertExperimentDone to test_single-qubit_parallel. Fixed param…

92f5580

…eters for test_full_sampling_single_qubit

Modified assertAllIdentity to support circuits with rz gates

3e9c386

Changed name _new_rb to _transpiled_rb. Also changed default to be Fa…

4557572

…lse, so that all tests will pass

removed fast_rb.py

0673e23

removed rb_on_device.py

c543fa4

Removed temporary 'import time'

3c2dca8

Added support for interleaved rb single qubit

b9e0884

Fixed handling of interleaved element

f899247

Fixed bug caused by change of interface of _buil_rb_circuits after ad…

2bc7469

…ding support for interleave

added test_number_to_clifford_mapping and fixed the method num_from_1…

bbaf218

…_qubit_clifford

Added support for computation of the Clifford to number mapping of a …

402bf69

…circuit

Moved setting of interleaved metadata to be under 'if is_interleaved'

67f07ca

Added transpilation of interleaved element before creating the rb cir…

d72beb1

…cuits. Transformed interleaved element into a transpiled clifford circuit. Added relevant tests

Fixed incorrect parameter 'qubits' in test_non_clifford_interleaved_e…

7f9778f

…lement

Added support for 'delay' as interleaved element

af42386

merav-aharoni added 4 commits July 25, 2022 13:56

Split test_rb_utils into itself and test_clifford_utils

1fdecee

Added tests for composing a clifford with a number

08e3a27

Added test for inverse clifford by num

60505a8

Changed random to rng because of failure on Windows

14411d4

chriseclectic requested a review from nkanazawa1989 July 26, 2022 16:09

nkanazawa1989 suggested changes Jul 26, 2022

View reviewed changes

ShellyGarion self-requested a review July 27, 2022 04:11

ShellyGarion reviewed Jul 27, 2022

View reviewed changes

gadial reviewed Jul 27, 2022

View reviewed changes

merav-aharoni added 4 commits July 27, 2022 15:05

Removed dependency on Aer for transpile. Removed the method transpile…

2990180

…_single_clifford and use transpile() directly instead

Changed parameter backend to be optional, as it was before

0d63efa

Improved format for CLIFF_SINGLE_GATE_MAP, CLIFF_COMPOSE_DATA, CLIFF_…

e90a5e5

…INVERSE_DATA

Changed usage from cliff.__repr__ to repr(cliff)

b91b4a0

merav-aharoni mentioned this pull request Jul 28, 2022

Added design doc for new rb algorithm #861

Closed

merav-aharoni added 4 commits August 4, 2022 15:21

Merge branch 'main' into 2_qubits_rb

30654f4

Fixed a bug where transpiled_circuits were loaded multiple times

632fa52

Merge branch '2_qubits_rb' of github.com:merav-aharoni/qiskit-experim…

0923667

…ents into 2_qubits_rb

Merge branch 'main' into 2_qubits_rb

5cf5c5f

merav-aharoni changed the title ~~New algorithm for RB using transpiled Cliffords~~ [WIP] New algorithm for RB using transpiled Cliffords Aug 17, 2022

itoko mentioned this pull request Aug 24, 2022

Refactor RB module for future extensions #898

Merged

merav-aharoni closed this Oct 3, 2022

Conversation

merav-aharoni commented Jul 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Details and comments

Uh oh!

merav-aharoni commented Jul 25, 2022

Uh oh!

nkanazawa1989 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ShellyGarion left a comment

Choose a reason for hiding this comment

Uh oh!

merav-aharoni commented Jul 27, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nkanazawa1989 Jul 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

merav-aharoni commented Jul 27, 2022

Uh oh!

nkanazawa1989 commented Jul 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

merav-aharoni commented Jul 19, 2022 •

edited

Loading

nkanazawa1989 Jul 27, 2022 •

edited

Loading

nkanazawa1989 commented Jul 27, 2022 •

edited

Loading