Clifford does not use StabilizerTable as internal data and PendingDeprecation of PauliTable and StabilizerTable #7269

ikkoham · 2021-11-15T14:27:38Z

Summary

Clifford does not use StabilizerTable as internal data and PendingDeprecation of PauliTable and StabilizerTable.

Details and comments

Performance regression occurs due to stacking of x and z and tracking of phase.
We need to check the regression is acceptable or not.
=> Performance check: small regression for small qubits (acceptable?)

I updated the code not to use PauliList.
We don't have the performance regression now.
This is new benchmark result:

       before           after         ratio
     [3f9edfc8]       [0371d4f9]
     <main>           <pull/7269/head>
          443±7ms          409±3ms     0.92  quantum_info.CliffordComposeBench.time_compose('1,7000')
         506±20ms         494±10ms     0.98  quantum_info.CliffordComposeBench.time_compose('2,5000')
         731±40ms          716±6ms     0.98  quantum_info.CliffordComposeBench.time_compose('3,5000')
         533±10ms          523±7ms     0.98  quantum_info.CliffordComposeBench.time_compose('4,2500')
         612±10ms          592±4ms     0.97  quantum_info.CliffordComposeBench.time_compose('5,2000')
         56.8±1ms         55.6±3ms     0.98  quantum_info.CliffordDecomposeBench.time_decompose('1,1000')
-      1.14±0.01s       1.02±0.01s     0.90  quantum_info.CliffordDecomposeBench.time_decompose('2,500')
       1.58±0.02s       1.47±0.02s     0.93  quantum_info.CliffordDecomposeBench.time_decompose('3,100')
-        623±20ms          527±3ms     0.85  quantum_info.CliffordDecomposeBench.time_decompose('4,50')
          196±4ms          176±5ms    ~0.90  quantum_info.CliffordDecomposeBench.time_decompose('5,10')
-         729±6ms         570±20ms     0.78  quantum_info.RandomCliffordBench.time_random_clifford('1,3000')
-         665±5ms         525±20ms     0.79  quantum_info.RandomCliffordBench.time_random_clifford('2,2500')
-         599±4ms          487±2ms     0.81  quantum_info.RandomCliffordBench.time_random_clifford('3,2000')
-         507±4ms          416±4ms     0.82  quantum_info.RandomCliffordBench.time_random_clifford('4,1500')
-         492±4ms          429±4ms     0.87  quantum_info.RandomCliffordBench.time_random_clifford('5,1000')
-        408±10ms          346±3ms     0.85  quantum_info.RandomCliffordBench.time_random_clifford('6,700')

coveralls · 2021-11-15T15:05:24Z

Pull Request Test Coverage Report for Build 3149420026

291 of 324 (89.81%) changed or added relevant lines in 7 files are covered.
6 unchanged lines in 3 files lost coverage.
Overall coverage increased (+0.05%) to 84.503%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/quantum_info/synthesis/clifford_decompose.py	31	32	96.88%
qiskit/quantum_info/operators/symplectic/clifford.py	181	213	84.98%

Files with Coverage Reduction	New Missed Lines	%
qiskit/quantum_info/operators/symplectic/clifford.py	1	87.43%
qiskit/extensions/quantum_initializer/squ.py	2	79.78%
qiskit/quantum_info/operators/symplectic/stabilizer_table.py	3	87.33%

Totals
Change from base Build 3148975703:	0.05%
Covered Lines:	60389
Relevant Lines:	71464

💛 - Coveralls

ShellyGarion · 2021-11-16T07:07:50Z

Perhaps it's also relevant to check the RB performance since it requires to generate and compose large sequences of random Cliffords.
There is a benchmark here: https://github.com/Qiskit/qiskit/blob/master/test/benchmarks/randomized_benchmarking.py
However, it's based on the old ignis code that is going to be deprecated soon, and not on the qiskit.experiments code
https://qiskit.org/documentation/experiments/tutorials/randomized_benchmarking.html

ikkoham · 2021-11-16T07:19:43Z

@ShellyGarion Thanks. I checked RB:

[  0.00%] ·· Benchmarking virtualenv-py3.8
[ 12.50%] ··· Running (randomized_benchmarking.RandomizedBenchmarkingBenchmark.time_ibmq_backend_transpile--).
[ 25.00%] ··· Running (randomized_benchmarking.RandomizedBenchmarkingBenchmark.time_ibmq_backend_transpile_single_thread--).
[ 37.50%] ··· randomized_benchmarking.RandomizedBenchmarkingBenchmark.time_ibmq_backend_transpile                                                                                                                                                                          ok
[ 37.50%] ··· =============== ============
                 rb_pattern
              --------------- ------------
                   [[0]]        846±20ms
                  [[0, 1]]     3.82±0.04s
               [[0, 1], [2]]   4.73±0.1s
              =============== ============

[ 50.00%] ··· randomized_benchmarking.RandomizedBenchmarkingBenchmark.time_ibmq_backend_transpile_single_thread                                                                                                                                                            ok
[ 50.00%] ··· =============== ============
                 rb_pattern
              --------------- ------------
                   [[0]]        814±20ms
                  [[0, 1]]     3.84±0.05s
               [[0, 1], [2]]   4.72±0.1s
              =============== ============

[ 50.00%] · For qiskit-terra commit e02119b2 <main>:
[ 50.00%] ·· Building for virtualenv-py3.8.......
[ 50.00%] ·· Benchmarking virtualenv-py3.8
[ 62.50%] ··· Running (randomized_benchmarking.RandomizedBenchmarkingBenchmark.time_ibmq_backend_transpile--).
[ 75.00%] ··· Running (randomized_benchmarking.RandomizedBenchmarkingBenchmark.time_ibmq_backend_transpile_single_thread--).
[ 87.50%] ··· randomized_benchmarking.RandomizedBenchmarkingBenchmark.time_ibmq_backend_transpile                                                                                                                                                                          ok
[ 87.50%] ··· =============== ===========
                 rb_pattern
              --------------- -----------
                   [[0]]        821±10ms
                  [[0, 1]]     4.10±0.3s
               [[0, 1], [2]]    9.60±3s
              =============== ===========

[100.00%] ··· randomized_benchmarking.RandomizedBenchmarkingBenchmark.time_ibmq_backend_transpile_single_thread                                                                                                                                                            ok
[100.00%] ··· =============== ============
                 rb_pattern
              --------------- ------------
                   [[0]]        881±60ms
                  [[0, 1]]     3.95±0.04s
               [[0, 1], [2]]   4.93±0.1s
              =============== ============

ShellyGarion · 2021-11-16T07:26:23Z

Thanks @ikkoham - but now I think that this benchmark is only for the transpilation process, and does not benchmark the actual generation of the RB sequences themselves...

ikkoham · 2021-11-16T07:27:21Z

I see. then, we need to add more benchmarks to Qiskit repository.

EDITED: I'll add the benchmark for the tutorial here.

chriseclectic

This looks like a good approach to me. Left a few suggestions for some small improvements.

qiskit/quantum_info/operators/symplectic/clifford.py

qiskit/quantum_info/operators/symplectic/pauli_list.py

qiskit/quantum_info/operators/symplectic/clifford.py

chriseclectic · 2021-11-16T22:12:21Z

qiskit/quantum_info/states/stabilizerstate.py

+from qiskit.quantum_info.operators.symplectic.clifford_circuits import (
+    _append_x,
+)
+from qiskit.quantum_info.states.quantum_state import QuantumState


 class StabilizerState(QuantumState):


Doesn't need to be in this PR, but we should improve the init method of StabilizerState so it can also be constructed from a PauliList, or data format that can itself be used to initialize a PauliList.

releasenotes/notes/deprecate-stabilizer-table-9efd08c7de1a5b4d.yaml

qiskit/quantum_info/states/stabilizerstate.py

ShellyGarion · 2021-11-22T14:54:42Z

In clifford_circuits and stabilzer_state it may be good to add some comments to the reader of the code, since it's not the exact formulas from Aaronson-Gottesman.

ikkoham · 2021-11-24T10:19:21Z

@ShellyGarion Thank you for your comments. It is true that the previous codes are hard to read. So, I added methods: stabilizer_tableau() and stabilizer_list(), and so on. I think now it's more readable.

qiskit/quantum_info/synthesis/clifford_decompose.py

ShellyGarion · 2022-08-30T07:09:06Z

qiskit/quantum_info/operators/symplectic/clifford.py


            # Validate table is a symplectic matrix
-            if validate and not Clifford._is_symplectic(self._table.array):
+            if validate and not Clifford._is_symplectic(self.tableau[:, :-1]):


Consider defining a property and a setter for the symplectic part of the clifford tableau, say symplectic, instead of the previous cliff.table.array and not call tableau[:, :-1] (which appears several times in the code).

Thanks. The properties are nice. The property name symplectic is well used in the literature? I know that we check the symplectic inner product in the method _is_symplectic, but I don't know whether Z components and X components is called symplectic.

The 2nx2n array in the clifford tableau is a symplectic binary matrix, see e.g. https://arxiv.org/abs/1406.2170 (eq. 3 and 4) and https://en.wikipedia.org/wiki/Symplectic_group for the definition.
But of course, it's possible to choose a different name if you like.

Yeah, it's an element of symplectic group. Thank you for the explanation. Then, I prefer Clifford.symplectic_matrix (a bit long).

Add in c1a8f98. It is more readable.

qiskit/quantum_info/operators/symplectic/clifford.py

ShellyGarion · 2022-09-22T07:18:31Z

releasenotes/notes/deprecate-stabilizer-table-9efd08c7de1a5b4d.yaml

+    release. Instead, the :class:`~qiskit.quantum_info.PauliList` should be used.
+    With this change, :attr:`.Clifford.table`, :attr:`.Clifford.stabilizer`, and
+    :attr:`.Clifford.destabilizer` have been deprecated so that you should use
+    :attr:`.Clifford.tableau`, :attr:`.Clifford.stabilizers`, :attr:`.Clifford.destabilizers`.


The methods now are called Clifford.stab and Clifford.destab?
Also, there are other new methods added.

Just bumping this comment in my review too.

ShellyGarion

This PR LGTM. It makes significant changes in the Clifford class API, but it seems to still contain all the needed functionality of a Clifford class.

jakelishman

I'm still concerned that some of the deprecations in Clifford tell users to used functions/attributes that are being added in the same PR, which breaks our deprecation policy. That said, this PR has been bounced from Terra releases so many times now (albeit in large part because it was breaking the deprecation policy...) that it may be more healthy for the development of the class to bend the rules this time. I'm concerned about affecting downstream users, but honestly, I think a large number of people are working around the Qiskit Clifford class right now because it's too slow, and we're currently blocking updates to that because of this PR.

Could you check through that all the deprecation notices are pointing to the right place, just one last time, and then I'll sign off on this.

jakelishman · 2022-09-27T22:58:30Z

releasenotes/notes/deprecate-stabilizer-table-9efd08c7de1a5b4d.yaml

+    release. Instead, the :class:`~qiskit.quantum_info.PauliList` should be used.
+    With this change, :attr:`.Clifford.table`, :attr:`.Clifford.stabilizer`, and
+    :attr:`.Clifford.destabilizer` have been deprecated so that you should use
+    :attr:`.Clifford.tableau`, :attr:`.Clifford.stabilizers`, :attr:`.Clifford.destabilizers`.


Just bumping this comment in my review too.

qiskit/quantum_info/operators/symplectic/clifford.py

jakelishman · 2022-09-28T00:02:42Z

qiskit/quantum_info/operators/symplectic/clifford.py

+    @deprecate_function(
+        "Indexing or iterating through a Clifford object directly is deprecated as of "
+        "Qiskit Terra 0.22.0 and will be removed no sooner than 3 months after the release date. "
+        "Instead, index or iterate through the Clifford.tableau attribute."
+    )
    def __getitem__(self, key):
        """Return a stabilizer Pauli row"""
-        return self._table.__getitem__(key)
+        return self.table.__getitem__(key)


The tableau attribute is new in this release, so we shouldn't really be issuing a DeprecationWarning telling users to use it unless there's already been a previous release with tableau in it. (See top review comment, though - I'm prepared to accept this PR despite breaking the deprecation policy, on somewhat pragmatic grounds of us having pushed this and held up development on the Clifford class for too long now.)

jakelishman · 2022-09-28T00:03:14Z

qiskit/quantum_info/operators/symplectic/clifford.py

    def __setitem__(self, key, value):
        """Set a stabilizer Pauli row"""
-        self._table.__setitem__(key, value)
+        self.tableau.__setitem__(key, value)


Is this going to do the right thing? I'm not 100% sure, but I didn't think the tableau representation was exactly the same.

Thanks, it was wrong...

ikkoham · 2022-09-28T02:34:35Z

OK, then let's stop deprecation. It follows the deprecation policy.

In any case, the deprecation of PauliTable will not be ready in time for 0.22, and I would like to deprecate PauliTable and StabilizerTable at the same time.

jakelishman

Oh, I wasn't expecting you to actually swap them all over! Thank you! This does make me feel happier about the whole thing - I know it's a bit longer til we can fully remove StabilizerTable, but we still maintain our deprecation policy. Thanks a lot!

ShellyGarion mentioned this pull request Nov 16, 2021

Add StabilizerState support to state_fidelity #7242

Open

ShellyGarion self-requested a review November 16, 2021 15:27

chriseclectic reviewed Nov 16, 2021

View reviewed changes

ShellyGarion mentioned this pull request Nov 17, 2021

Performence benchmarks for the experiments qiskit-community/qiskit-experiments#531

Closed

ShellyGarion reviewed Nov 22, 2021

View reviewed changes

releasenotes/notes/deprecate-stabilizer-table-9efd08c7de1a5b4d.yaml Outdated Show resolved Hide resolved

ShellyGarion reviewed Nov 22, 2021

View reviewed changes

qiskit/quantum_info/states/stabilizerstate.py Outdated Show resolved Hide resolved

ShellyGarion reviewed Nov 22, 2021

View reviewed changes

qiskit/quantum_info/states/stabilizerstate.py Outdated Show resolved Hide resolved

ikkoham marked this pull request as ready for review November 22, 2021 15:13

ikkoham requested a review from a team as a code owner November 22, 2021 15:13

ikkoham changed the title ~~[WIP] Deprecate StabilizerTable~~ Deprecate StabilizerTable Nov 22, 2021

ShellyGarion reviewed Nov 24, 2021

View reviewed changes

qiskit/quantum_info/synthesis/clifford_decompose.py Outdated Show resolved Hide resolved

ShellyGarion reviewed Nov 24, 2021

View reviewed changes

qiskit/quantum_info/synthesis/clifford_decompose.py Outdated Show resolved Hide resolved

ShellyGarion reviewed Nov 24, 2021

View reviewed changes

qiskit/quantum_info/synthesis/clifford_decompose.py Outdated Show resolved Hide resolved

ShellyGarion reviewed Nov 24, 2021

View reviewed changes

qiskit/quantum_info/synthesis/clifford_decompose.py Outdated Show resolved Hide resolved

ShellyGarion previously approved these changes Nov 25, 2021

View reviewed changes

ikkoham dismissed ShellyGarion’s stale review via fe3bb81 November 25, 2021 13:42

ShellyGarion self-requested a review November 25, 2021 14:07

ShellyGarion previously approved these changes Nov 25, 2021

View reviewed changes

ikkoham mentioned this pull request Nov 30, 2021

Deprecate PauliTable #7245

Closed

1 task

chriseclectic added mod: quantum info Related to the Quantum Info module (States & Operators) Changelog: Deprecation Include in "Deprecated" section of changelog labels Nov 30, 2021

kdk added this to the 0.19 milestone Nov 30, 2021

kdk assigned chriseclectic Nov 30, 2021

ShellyGarion reviewed Aug 30, 2022

View reviewed changes

ikkoham added 2 commits August 31, 2022 11:13

add Clifford.symplectic_matrix method

c1a8f98

Merge branch 'main' into qinfo/deprecate-stabilizer-table

36fb159

ShellyGarion reviewed Sep 22, 2022

View reviewed changes

qiskit/quantum_info/operators/symplectic/clifford.py Outdated Show resolved Hide resolved

ShellyGarion reviewed Sep 22, 2022

View reviewed changes

ShellyGarion previously approved these changes Sep 22, 2022

View reviewed changes

ikkoham added 4 commits September 26, 2022 14:25

Merge branch 'main' into qinfo/deprecate-stabilizer-table

02d2e11

Merge branch 'main' into qinfo/deprecate-stabilizer-table

ca5b46e

Merge branch 'main' into qinfo/deprecate-stabilizer-table

f531c10

Merge branch 'main' into qinfo/deprecate-stabilizer-table

f73267a

ikkoham requested a review from itoko September 27, 2022 05:43

jakelishman reviewed Sep 28, 2022

View reviewed changes

remove deprecation

e3be4fc

ikkoham dismissed ShellyGarion’s stale review via e3be4fc September 28, 2022 03:14

ikkoham added 2 commits September 28, 2022 12:30

fix __setitem__

2bdec57

add pending deprecation to PauliTable

8dc8b68

ikkoham changed the title ~~Deprecate StabilizerTable~~ Clifford does not use StabilizerTable as internal data and PendingDeprecation of PauliTable and StabilizerTable Sep 28, 2022

ikkoham added 3 commits September 28, 2022 13:11

update releasenote

9195d84

update releasenote

8b6b40a

refactor

bb306a4

mtreinish added the Changelog: New Feature Include in the "Added" section of the changelog label Sep 28, 2022

ikkoham requested a review from jakelishman September 28, 2022 22:20

jakelishman approved these changes Sep 29, 2022

View reviewed changes

jakelishman added the automerge label Sep 29, 2022

Merge branch 'main' into qinfo/deprecate-stabilizer-table

cdf93f6

mergify bot merged commit 17de3b0 into Qiskit:main Sep 29, 2022

ikkoham deleted the qinfo/deprecate-stabilizer-table branch September 29, 2022 12:47

itoko mentioned this pull request Oct 4, 2022

Improve the performance of Clifford.from_circuit() #8585

Merged

ikkoham mentioned this pull request Feb 8, 2023

Deprecate PauliTable and StabilizerTable #9547

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clifford does not use StabilizerTable as internal data and PendingDeprecation of PauliTable and StabilizerTable #7269

Clifford does not use StabilizerTable as internal data and PendingDeprecation of PauliTable and StabilizerTable #7269

ikkoham commented Nov 15, 2021 •

edited

Loading

coveralls commented Nov 15, 2021 •

edited

Loading

ShellyGarion commented Nov 16, 2021

ikkoham commented Nov 16, 2021

ShellyGarion commented Nov 16, 2021

ikkoham commented Nov 16, 2021 •

edited

Loading

chriseclectic left a comment •

edited

Loading

chriseclectic Nov 16, 2021

ShellyGarion commented Nov 22, 2021

ikkoham commented Nov 24, 2021

ShellyGarion Aug 30, 2022

ikkoham Aug 30, 2022 •

edited

Loading

ShellyGarion Aug 30, 2022

ikkoham Aug 30, 2022

ikkoham Aug 31, 2022

ShellyGarion Sep 22, 2022

jakelishman Sep 27, 2022

ShellyGarion left a comment

jakelishman left a comment

jakelishman Sep 27, 2022

jakelishman Sep 28, 2022 •

edited

Loading

jakelishman Sep 28, 2022

ikkoham Sep 28, 2022

ikkoham commented Sep 28, 2022 •

edited

Loading

jakelishman left a comment

Clifford does not use StabilizerTable as internal data and PendingDeprecation of PauliTable and StabilizerTable #7269

Clifford does not use StabilizerTable as internal data and PendingDeprecation of PauliTable and StabilizerTable #7269

Conversation

ikkoham commented Nov 15, 2021 • edited Loading

Summary

Details and comments

coveralls commented Nov 15, 2021 • edited Loading

Pull Request Test Coverage Report for Build 3149420026

💛 - Coveralls

ShellyGarion commented Nov 16, 2021

ikkoham commented Nov 16, 2021

ShellyGarion commented Nov 16, 2021

ikkoham commented Nov 16, 2021 • edited Loading

chriseclectic left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ShellyGarion commented Nov 22, 2021

ikkoham commented Nov 24, 2021

Choose a reason for hiding this comment

ikkoham Aug 30, 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

ShellyGarion left a comment

Choose a reason for hiding this comment

jakelishman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakelishman Sep 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikkoham commented Sep 28, 2022 • edited Loading

jakelishman left a comment

Choose a reason for hiding this comment

ikkoham commented Nov 15, 2021 •

edited

Loading

coveralls commented Nov 15, 2021 •

edited

Loading

ikkoham commented Nov 16, 2021 •

edited

Loading

chriseclectic left a comment •

edited

Loading

ikkoham Aug 30, 2022 •

edited

Loading

jakelishman Sep 28, 2022 •

edited

Loading

ikkoham commented Sep 28, 2022 •

edited

Loading