qiskit
qiskit copied to clipboard
Qiskit
Deprecate StabilizerTable
Summary
This PR deprecates StabilizerTable, which is used in Clifford.
Related to https://github.com/Qiskit/qiskit-terra/pull/7245.
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')
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% |
| <!-- | Total: | 291 | 324 |
| 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% |
| <!-- | Total: | 6 |
| Totals | |
|---|---|
| Change from base Build 3148975703: | 0.05% |
| Covered Lines: | 60389 |
| Relevant Lines: | 71464 |
💛 - Coveralls
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
@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
=============== ============
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...
I see. then, we need to add more benchmarks to Qiskit repository.
EDITED: I'll add the benchmark for the tutorial here.
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.
@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.
@ikkoham: we'd like to get this merged soon to ensure that both it and #7245 can get into 0.20, and hopefully even some of the Clifford-related performance improvements. I know you're busy with the new primitives, so let us know if we need to ask somebody else to get this finalised.
It looks to me on a quick glance that the new from_stabilizers method will alleviate most of my usability worries about the deprecation, but we'd need to swap our internal cases that set the stabilisers and destabilisers to that as well - right now we're still logically mutating the Clifford class in those cases, which is something the deprecation seems to be telling people not to do.
We talked about this PR in the Terra meeting on Tuesday, and if you're ok with it, we'd prefer to push this from 0.20, and for you, me, Shelly and potentially Matthew to sit together and make sure we're all agreed on why StabilizerTable and PauliTable need to go, and what (if any) performance regressions are acceptable.
The benchmarks at the top of this PR indicate relatively hefty regressions - is it definitely appropriate to use PauliList as the backing for Clifford if it's slower than StabilizerTable? If the eventual plan is to win back the speed by rewriting parts of it in Rust, then is there a need to go through a painful deprecation period when we'd be likely to need another one soon after? The speed of using compiled extensions will help in some cases, but if PauliList fundamentally requires more operations to calculate the necessary values, it still might not be the best choice in moving to it.
I believe we already discussed that issue at last years quantum_info meeting. I think at least @chriseclectic and @ShellyGarion were there, maybe you (@jakelishman) and @mtreinish too. The regression is only for small number of qubits, which is not bad for scaling, so I'm sure we agreed, the adoption of PauliList is due to the simplicity that comes from using one class. In the first place I thought that Clifford should be given a dedicated implementation, which is not PauliList. If we are going to go into this issue, we will redevelop from scratch and target 0.21.
The process is:
- Define the internal data structure for Clifford (based on tableau representation).
- StabilizerTable and PauliTable will be deprecated and use PauliList instead.
If you agree this direction, I'll close this PR. @jakelishman @ShellyGarion @chriseclectic
This is my summer vacation :gift: Sorry for late, but could you review the PR again?
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.