Improve integer-based Clifford operations for 1Q/2Q RB #940

itoko · 2022-10-12T09:23:01Z

Summary

This PR is a follow-up of #892, #922 for improving implementation of clifford_utils module.

Details and comments

This PR improves the implementation of clifford_utils module without changing the original algorithms for integer-based Clifford operations. It includes

Change API from methods CliffordUtils.* to functions clifford_utils.*
Remove creation of CliffordUtils objects (Change CliffordUtils back to a library class)
Simplify API by minimizing public functions
Change clifford data file format to npz

It preserves great ideas introduced in #892 such as:

Using lookup tables for Clifford operations (compose and adjoint)
- Especially, sparse lookup table for 2Q Clifford composition (that reduces memory cost to store lookup table)
Composing 2Q Clifford circuit from triplets of circuit layers (that reduces computational cost for basis translation)
Release note: :meth:.ClifforUtils.clifford_2_qubit_circuit changes its mapping between integers and 2Q Cliffords. For a given integer, it may return a different Clifford than before.

Co-authored-by: merav-aharoni <[email protected]> Including following features: * Simplify clifford_utils API * Clean up script file for clifford data generation * Change clifford data files to npz formant and add them to package * Simplify data handling for 2q clifford compose with sparse matrix

merav-aharoni

Overall looks fine. The usage of npz files is a nice improvement since they do not harm performance.
I wrote a few small comments.

merav-aharoni · 2022-10-13T10:55:49Z

qiskit_experiments/library/randomized_benchmarking/clifford_utils.py

@@ -142,16 +132,7 @@ def _clifford_1q_int_to_instruction(
 def _clifford_2q_int_to_instruction(


Why isn't this method part of CliffordUtils? Same for _clifford_1q_int_to_instruction? Same for all the methods at the bottom.

I just prefer function API to class method API for this utility module because we have no need to create CliffordUtils objects. The class just behave as a module name. That means it can be replaced with import clifford_utils as clu. Any comment, @nkanazawa1989 ? Should we deprecate CliffordUtils class if we move to the function API?

Yes, I agree. CliffordUtils can be promoted to an actual generator, but these function can be excluded if not intended to be used as a part of the generator (probably this one can be CliffordUtils method, because it takes basis gates and likely the basis gates are instance variable of the proper RB circuit generator).

merav-aharoni · 2022-10-13T11:01:06Z

qiskit_experiments/library/randomized_benchmarking/clifford_utils.py

+        self, num_qubits: int, size: int = 1, rng: Optional[Union[int, Generator]] = None
+    ):
+        """Generate a list of random clifford circuits"""
+        if num_qubits > 2:


Suggested change

if num_qubits > 2:

if num_qubits ==1:

samples = rng.integers(CliffordUtils.NUM_CLIFFORD_1_QUBIT, size=size)

return [self.clifford_1_qubit_circuit(i) for i in samples]

else if num_qubits ==2:

samples = rng.integers(CliffordUtils.NUM_CLIFFORD_2_QUBIT, size=size)

return [self.clifford_2_qubit_circuit(i) for i in samples]

else: # num_qubits > 2

return [random_clifford(num_qubits, seed=rng).to_circuit() for _ in range(size)]

Sorry, I don't catch the point of your suggestion. I've just restored the original code before #892 here. Did you find any issue in the original code?

It is not a big issue. I just meant the ordering of the if statements. It seems more natural to me to write:

if n == 1: do 1 if n == 2: do 2 else: do all other cases

Ah, looking around it, I found two bugs in ClifforUtils.random_cliffords and ClifforUtils.random_clifford_circuits in the original code... I've fixed them in 01722e2 and decided to deprecate them since they are not used anywhere at least within qiskit-experiments.

qiskit_experiments/library/randomized_benchmarking/clifford_utils.py

merav-aharoni · 2022-10-18T13:20:58Z

qiskit_experiments/library/randomized_benchmarking/rb_experiment.py

    def __compose_clifford(
        self, left_elem: SequenceElementType, right_elem: SequenceElementType
    ) -> SequenceElementType:
-        if self.num_qubits <= 2:
-            utils = self._cliff_utils
-            return utils.compose_num_with_clifford(
-                left_elem, utils.create_cliff_from_num(right_elem)
-            )
-
+        if self.num_qubits == 1:
+            return compose_1q(left_elem, right_elem)
+        if self.num_qubits == 2:
+            return compose_2q(left_elem, right_elem)
        return left_elem.compose(right_elem)


Shouldn't this method be part of CliffordUtils? Same for __adjoint_clifford

qiskit_experiments/library/randomized_benchmarking/rb_experiment.py

merav-aharoni · 2022-10-18T13:39:24Z

test/library/randomized_benchmarking/test_clifford_utils.py

-        """Check that all 2 clifford numbers form a permutation over [0, 11519]"""
-        cliff_utils = CliffordUtils(num_qubits=2, basis_gates=self.basis_gates)
-        cliff_utils.transpile_2q_cliff_layers()
-        self.is_permutation(cliff_utils.NUM_CLIFFORD_2_QUBIT, CLIFF_LAYERS_TO_NUM_2Q)


Testing that rows for a permutation was requested by @ShellyGarion in her initial review of my code.

As far as I understand, this test was necessary to check the validity of CLIFF_LAYERS_TO_NUM_2Q that defines another mapping between number and Clifford than a mapping defined in CliffordUtils.clifford_2_qubit.

I've removed CLIFF_LAYERS_TO_NUM_2Q and changed to have only one mapping between number and Clifford, so I think we don't need the test any longer (this round-trip test would be sufficient to check the validity of the mapping).
I'll let users know the change in number-2qClifford mapping via release note.

I am not sure I fully understand the new mapping, so maybe I am mistaken. For 1 qubit, we expect the mapping of all Cliffords to contain all the numbers 0-23 in some permutation. Is this still correct? Similarly for 2 qubits.

Yes. It must be correct.
For 1Q case, test_clifford_1_qubit_generation directly guarantees that [0..23] yields all possible 24 Cliffords.
For 2Q case, test_number_to_clifford_mapping_2q effectively guarantees that [0..11519] yields all possible 11520 Cliffords. Instead of enumerating all symplectic stabilizer tables, I used the round trip test (number i -> Clifford -> number i for all i in [0..11519]). Its success suggests all possible 11520 Cliffords are generated.

…erleaving circuit with delay using BackendV1

…iled circuit

nkanazawa1989

Thanks @itoko san this looks nice. Likely this is the minimum module import overhead because transpile is now on the fly with cache and cache is generated over triplets. Numpy data is good choice of data format though I have bit concern about persistency of the data because this is not version controlled.

nkanazawa1989 · 2022-10-24T05:25:40Z

qiskit_experiments/library/randomized_benchmarking/data/generate_clifford_data.py

+    _CLIFF_SINGLE_GATE_MAP_2Q,
+)
+
+NUM_CLIFFORD_1Q = CliffordUtils.NUM_CLIFFORD_1_QUBIT


This name is bit confusing because we often say number to indicate circuit depth. Perhaps SIZE_CLIFFORD_GROUP_1Q? Same for others.

nkanazawa1989 · 2022-10-24T05:52:40Z

qiskit_experiments/library/randomized_benchmarking/data/generate_clifford_data.py

+    """Produce a hashable value that is unique for each different Clifford.  This should only be
+    used internally when the classes being hashed are under our control, because classes of this
+    type are mutable."""
+    table = cliff.table


I checked the Clifford class and likely they only compare .tableau property. Why do you implement custom code for equality?

https://github.com/Qiskit/qiskit-terra/blob/0ba7a438bd5bcb9c7edfdefc2ea5116d8f47bca2/qiskit/quantum_info/operators/symplectic/clifford.py#L192

According to the stack overflow string conversion is the common approach for hash. This does not quite affect RB experiment performance though.
https://stackoverflow.com/questions/16589791/most-efficient-property-to-hash-for-numpy-array

Good questions, actually, I thought both of them and converged to the current code.

Regarding tableau, it's quite new, i.e. introduced in the latest terra 0.22. If we use the property, experiments must require terra >= 0.22. I think it would be better to use table property until it is deprecated. But I know this code is only for developers to use (so far), so the requirement might not be so demanding (the requirement would live in requirements-dev.txt). Which do you like?

The code implementing hash originated from Jake's PR in terra. I like the code because it should be more efficient than the general approach that uses string conversion in this particular case (we never call hash since we need to create a hash without collision).

nkanazawa1989 · 2022-10-24T09:27:26Z

qiskit_experiments/library/randomized_benchmarking/data/generate_clifford_data.py

+        raise Exception(
+            "_CLIFF_SINGLE_GATE_MAP_1Q must be generated by gen_cliff_single_1q_gate_map()"
+        )
+    np.savez_compressed("clifford_inverse_1q.npz", table=gen_clifford_inverse_1q())


Do you think there is any risks of version dependency? Conversion of circuit -> Clifford instance currently has dependency on terra, and index is indirectly controlled in the CliffordUtils. If someone update these code, do you have a mechanism in the unittest to detect this?

I don't think this code depends on terra because the semantics of gates won't be changed. Also CliffordUtils (but not this code) defines the num<->Clifford mapping. Updates in CliffordUtils are tested by unittest and updates in this code are tested by assertions embedded in the code. The person who updates CliffordUtils is responsible to let users know the change in num<->Clifford mapping if they do so.

nkanazawa1989 · 2022-10-24T09:35:12Z

qiskit_experiments/library/randomized_benchmarking/interleaved_rb_experiment.py

        if isinstance(self._interleaved_op, QuantumCircuit):
            interleaved_circ = self._interleaved_op
-        elif isinstance(self._interleaved_op, Clifford):
-            interleaved_circ = self._interleaved_op.to_circuit()
        elif isinstance(self._interleaved_op, Gate):
            interleaved_circ = QuantumCircuit(self.num_qubits, name=self._interleaved_op.name)
            interleaved_circ.append(self._interleaved_op, list(range(self.num_qubits)))


Is this gate direction aware? We can instantiate experiments with different physical qubits ordering qubits=[0, 1] and qubits=[1, 0], so I think this must be direction aware.

For interleaved element, it should be cared by transpile called a few lines later. However, I have recently realized a bug related with the direction of basis gates. In BackendV2, all gates are direction aware so we need to handle all basis gates in direction-aware as you pointed out. It may be a critical bug in the era of BackendV2. I'll address the issue in a separate PR.

nkanazawa1989 · 2022-10-24T09:40:04Z

qiskit_experiments/library/randomized_benchmarking/interleaved_rb_experiment.py

+
+        if isinstance(self._interleaved_op, QuantumCircuit):
+            if not self._interleaved_op.name.startswith("Clifford"):
+                self._interleaved_op.name = f"Clifford-{self._interleaved_op.name}"


Why the op name does matter?

Yes. I'm using it to know which operation must be decomposed within _decompose_clifford_ops when transpiling circuits. It's a bit hacky. Should I add some comments? As discussed in #930, if we change circuits to return unwrapped (decomposed) circuits, we don't need to worry about circuit decomposition. What do you think is good for now and future?

nkanazawa1989 · 2022-10-24T10:02:49Z

qiskit_experiments/library/randomized_benchmarking/clifford_utils.py

+    return circuits
+
+
+_CLIFFORD_LAYER = (


What is the expected overhead of this at a time of importing this module?

It's almost invisible. Previously importing clifford_utils takes about 1.41--1.43 seconds and now it takes about 1.44--1.46 seconds. About 0.03 sec increase seems acceptable to me.

qiskit_experiments/library/randomized_benchmarking/clifford_utils.py

qiskit_experiments/library/randomized_benchmarking/rb_experiment.py

nkanazawa1989 · 2022-10-24T10:13:58Z

MANIFEST.in

@@ -1,3 +1,5 @@
 include LICENSE.txt
 include requirements.txt
 include qiskit_experiments/VERSION.txt
+include qiskit_experiments/library/randomized_benchmarking/data/*.npz
+exclude qiskit_experiments/library/randomized_benchmarking/data/*.py


Suggested change

exclude qiskit_experiments/library/randomized_benchmarking/data/*.py

exclude qiskit_experiments/library/randomized_benchmarking/data/generate_clifford_data.py

I think you must explicitly state the file name. Otherwise this could cause unexpected bug in the future update.

I've found the simplest way to include *.npz and exclude *.py is do nothing here (and having no init file for the data directory). Resolved in a04310a

MANIFEST.in

qiskit_experiments/library/randomized_benchmarking/data/generate_clifford_data.py

releasenotes/notes/update-number-to-2q-clifford-mapping-c28f1f29b0205d57.yaml

qiskit_experiments/library/randomized_benchmarking/interleaved_rb_experiment.py

…ity#940) * Improve integer-based Clifford operations for 1Q/2Q RB Co-authored-by: merav-aharoni <[email protected]> Including following features: * Simplify clifford_utils API * Clean up script file for clifford data generation * Change clifford data files to npz formant and add them to package * Simplify data handling for 2q clifford compose with sparse matrix * Fix a too restrictive test * Fix performance regression in transpiled 2Q Clifford circuit generation * Fix bugs in two methods for random Clifford generation in CliffordUtils * Minor improvement of interface * Fix a bug in Backend V1 to V2 conversion that caused a failure in interleaving circuit with delay using BackendV1 * Fix a bug that interleaved circuit is not always decomposed in transpiled circuit * Exclude a data generation script file from package * Simplify MANIFEST.ini * Updates following reviewers suggestions * Improve docs and a few lines of code following reviewers suggestions

Improve the performance of transpiled circuit generation in 1Q/2Q StandardRB/InterleavedRB (about 10x speedup in 1Q SRB/IRB and 5x speedup in 2Q SRB/IRB in most cases). Including following feature pull-requests: * New algorithm for RB building Cliffords by layers (#892) * Improve custom transpilation for faster 1Q/2Q RB (#922) * Improve integer-based Clifford operations for 1Q/2Q RB (#940) * Readd support for backends with directed basis gates to RB experiments (#960) Features other than speedup: * Fix performance regression in 3 or more qubits RB (embedded at Refactor RB module for future extensions #898) * Improve validation of interleave_element in InterleavedRB * Restructure tests for RB module * Remove the barriers in front of the first Cliffords in RB circuits Co-authored-by: merav-aharoni <[email protected]> Co-authored-by: Naoki Kanazawa <[email protected]>

itoko added 2 commits October 12, 2022 18:18

Fix a too restrictive test

a9c1760

itoko mentioned this pull request Oct 12, 2022

Integer-based Clifford operations for faster 1Q/2Q RB #907

Closed

itoko requested review from merav-aharoni and nkanazawa1989 October 13, 2022 04:26

Fix performance regression in transpiled 2Q Clifford circuit generation

6c1d9ad

itoko force-pushed the feature-rb-integer-clifford branch from 953ec40 to 6c1d9ad Compare October 14, 2022 02:41

merav-aharoni reviewed Oct 18, 2022

View reviewed changes

itoko added 7 commits October 19, 2022 16:56

Add comments

26a9224

Add release note

5aeb393

Fix bugs in two methods for random Clifford generation in CliffordUtils

01722e2

Minor improvement of interface

371d5a6

Fix a bug in Backend V1 to V2 conversion that caused a failure in int…

ef11be5

…erleaving circuit with delay using BackendV1

Fix a bug that interleaved circuit is not always decomposed in transp…

1512293

…iled circuit

Exclude a data generation script file from package

fced280

nkanazawa1989 reviewed Oct 24, 2022

View reviewed changes

itoko added 2 commits October 25, 2022 11:32

Simplify MANIFEST.ini

a04310a

Updates following reviewers suggestions

851cf64

nkanazawa1989 approved these changes Oct 25, 2022

View reviewed changes

ihincks reviewed Oct 25, 2022

View reviewed changes

Improve docs and a few lines of code following reviewers suggestions

aff3fb5

itoko merged commit c626dc7 into qiskit-community:feature/rb_speedup Oct 27, 2022

itoko mentioned this pull request Nov 29, 2022

Speed up 1Q/2Q RB #982

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve integer-based Clifford operations for 1Q/2Q RB #940

Improve integer-based Clifford operations for 1Q/2Q RB #940

itoko commented Oct 12, 2022 •

edited

Loading

merav-aharoni left a comment

merav-aharoni Oct 13, 2022

itoko Oct 19, 2022

nkanazawa1989 Oct 21, 2022 •

edited

Loading

merav-aharoni Oct 13, 2022

itoko Oct 19, 2022

merav-aharoni Oct 19, 2022

itoko Oct 20, 2022

merav-aharoni Oct 18, 2022

merav-aharoni Oct 18, 2022

itoko Oct 19, 2022

merav-aharoni Oct 19, 2022 •

edited

Loading

itoko Oct 20, 2022 •

edited

Loading

nkanazawa1989 left a comment

nkanazawa1989 Oct 24, 2022

nkanazawa1989 Oct 24, 2022

itoko Oct 25, 2022 •

edited

Loading

nkanazawa1989 Oct 24, 2022

itoko Oct 25, 2022

nkanazawa1989 Oct 24, 2022

itoko Oct 25, 2022

nkanazawa1989 Oct 24, 2022

itoko Oct 25, 2022

nkanazawa1989 Oct 24, 2022

itoko Oct 25, 2022

nkanazawa1989 Oct 24, 2022

itoko Oct 25, 2022

		@@ -142,16 +132,7 @@ def _clifford_1q_int_to_instruction(
		def _clifford_2q_int_to_instruction(

-        if num_qubits > 2:
+        if num_qubits ==1:
+        samples = rng.integers(CliffordUtils.NUM_CLIFFORD_1_QUBIT, size=size)
+            return [self.clifford_1_qubit_circuit(i) for i in samples]
+        else if num_qubits ==2:
+            samples = rng.integers(CliffordUtils.NUM_CLIFFORD_2_QUBIT, size=size)
+            return [self.clifford_2_qubit_circuit(i) for i in samples]
+        else:  # num_qubits > 2
+            return [random_clifford(num_qubits, seed=rng).to_circuit() for _ in range(size)]

	exclude qiskit_experiments/library/randomized_benchmarking/data/*.py
	exclude qiskit_experiments/library/randomized_benchmarking/data/generate_clifford_data.py

Improve integer-based Clifford operations for 1Q/2Q RB #940

Improve integer-based Clifford operations for 1Q/2Q RB #940

Conversation

itoko commented Oct 12, 2022 • edited Loading

Summary

Details and comments

merav-aharoni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nkanazawa1989 Oct 21, 2022 • edited Loading

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

merav-aharoni Oct 19, 2022 • edited Loading

Choose a reason for hiding this comment

itoko Oct 20, 2022 • edited Loading

Choose a reason for hiding this comment

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

itoko Oct 25, 2022 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itoko commented Oct 12, 2022 •

edited

Loading

nkanazawa1989 Oct 21, 2022 •

edited

Loading

merav-aharoni Oct 19, 2022 •

edited

Loading

itoko Oct 20, 2022 •

edited

Loading

itoko Oct 25, 2022 •

edited

Loading