Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add the Solovay-Kitaev algorithm to the transpiler passes #5657

Merged
merged 126 commits into from Dec 9, 2022

Conversation

LNoorl
Copy link
Contributor

@LNoorl LNoorl commented Jan 20, 2021

Summary

Add the Solovay-Kitaev algorithm from https://arxiv.org/abs/quant-ph/0505030 by C. Dawson and M. Nielsen to the transpiler passes.

Details and comments

For example, the following circuit

             ┌─────────┐
        q_0: ┤ RX(0.8) ├
             └─────────┘

can be decomposed into

        global phase: -π/8
             ┌───┐┌───┐┌───┐
        q_0: ┤ H ├┤ T ├┤ H ├
             └───┘└───┘└───┘

with an L2-error of approximately 0.01.

The code to do the above is:

import numpy as np
from qiskit.circuit import QuantumCircuit
from qiskit.circuit.library import TGate, HGate, TdgGate
from qiskit.converters import circuit_to_dag, dag_to_circuit
from qiskit.transpiler.passes import SolovayKitaevDecomposition
from qiskit.quantum_info import Operator

circuit = QuantumCircuit(1)
circuit.rx(0.8, 0)
dag = circuit_to_dag(circuit)

print('Original circuit:')
print(circuit.draw())

basis_gates = [TGate(), TdgGate(), HGate()]
skd = SolovayKitaevDecomposition(recursion_degree=2, basis_gates=basis_gates)

discretized = dag_to_circuit(skd.run(dag))

print('Discretized circuit:')
print(discretized.draw())

print('Error:', np.linalg.norm(Operator(circuit).data - Operator(discretized).data))

Preliminary results

Here are some benchmark results for the QFT circuit on three qubits using the basis gate set h t tdg and combinations thereof of depth up to 10.

Error (in diamond norm):

approxerror

Runtime:

approxtime

TODO

  • Fix the global phase
  • Fix the accumulation roundoff errors
  • Add test for multiqubit circuits
  • Fix the basis gates in the unit tests
  • Allow setting the basis gates by string
  • Fix pylint

@Cryoris
Copy link
Contributor

Cryoris commented Nov 22, 2022

@mtreinish @alexanderivrii all set! Let me know if there's other changes we should add 🙂

@alexanderivrii
Copy link
Contributor

@Cryoris, thank you for following up on the code reorganization comment. From this point of view, everything looks great, but I haven't really looked at anything else. My understanding was that @mtreinish is doing the actual review. (Of course, I would be happy to help, if required).

Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is getting close, it'll be really good to get this functionality included. I left a few comments and questions inline, the biggest thing still to do from my PoV are fix the docs so we're actually including them in the build.

qiskit/synthesis/discrete_basis/__init__.py Show resolved Hide resolved
qiskit/synthesis/discrete_basis/commutator_decompose.py Outdated Show resolved Hide resolved
qiskit/synthesis/discrete_basis/gate_sequence.py Outdated Show resolved Hide resolved
qiskit/synthesis/discrete_basis/gate_sequence.py Outdated Show resolved Hide resolved
Comment on lines +143 to +144
adjoint = GateSequence()
adjoint.gates = [gate.inverse() for gate in reversed(self.gates)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason to just do GateSequence([gate.inverse() for gate in reversed(self.gates)])? I think this question holds for a bunch of places we're constructing new GateSequence objects.

Copy link
Contributor

@Cryoris Cryoris Nov 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah because we can more efficiently compute the values that would otherwise be computed from scratch in the __init__ 🙂 I'll leave a comment in the code to make that clear

@@ -133,7 +133,7 @@
DAGLongestPath

Synthesis
=============
=========
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add thew transpiler pass to this list and re-export it from this module so people can do from qiskit.transpiler.passes import SolovayKitaev

qiskit/synthesis/discrete_basis/solovay_kitaev.py Outdated Show resolved Hide resolved
Comment on lines +81 to +83
# if a file, load the dictionary
if isinstance(data, str):
data = np.load(data, allow_pickle=True)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason to keep the save functionality around since the computation is much faster now?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's likely still slow for large basis sets/depths or you'd quickly like to restart a calculation 🙂

qiskit/synthesis/discrete_basis/__init__.py Outdated Show resolved Hide resolved
docs/apidocs/synthesis_discretebasis.rst Outdated Show resolved Hide resolved
qiskit/transpiler/passes/synthesis/plugin.py Outdated Show resolved Hide resolved
mtreinish
mtreinish previously approved these changes Dec 9, 2022
Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM now thanks for the updates. I pushed a couple commits to fix the docs build and clean things up a bit.

mtreinish
mtreinish previously approved these changes Dec 9, 2022
@mergify mergify bot merged commit 9c1c18c into Qiskit:main Dec 9, 2022
Cryoris added a commit to Cryoris/qiskit-terra that referenced this pull request Jan 12, 2023
* add sk pass

* add utils

* add sk skeleton

* add example test file

* Add implementations initial version

* make the test run

* fix lint

* fix phase, add test

* fix the phase calculation

* add accuracy test

* cleanup of unused methods, append more efficient

* Add unittests for Solovay Kitaev Utils

* Add unitttests for commutator_decompose

* Remove unittests for non-public functions

* Fix pylint issues

* refactor simplify

* fix the test imports

* improve and add docstrings

* fix lint in testfile

* fix lint in utils

* fix lint in sk

* Add unittests

* fix phase, lint and allow basis gates as strings

* rm print

* fix adjoint and dot

* Fix unittests and add docstring to them

* move to circuit to GateSequence

* Fix assertion

* allow caching the basic approxs

* Fix bug in append from GateSequence

* Remove unnecesssary code

* Fix bug in test setup

* Add unittests for multiqubit QFT-circuit

* Add unittests for QFT with more qubits

* make compatible with current tests

* Remove unnecessary testcases

* Remove unnecessary unittests

* Fix bug in unittests

* Add release notes

* import scipy; scipy.optimize.fsolve() does not work

scipy.optimize has to be imported, since it is not imported when scipy is imported

* numpy

* Update qiskit/transpiler/passes/synthesis/solovay_kitaev_utils.py

Co-authored-by: Luciano Bello <bel@zurich.ibm.com>

* document ValueError

* rm unused methods, add GateSequence tests

* fix lint

* try fixing lint and docs

* cleanup

* improve readability of math
* move commutator_decompose into utils
* rm tests that are never run
* rm trailing todos and commented code

* add default dataset, cleanup tests

* rm unused imports

* replace diamond norm by trace distance

* rm unused imports

* fix deprecated use of DAGNode.type

* black

* really black!

* fail nicely

* test

* attempt to fix default approx path

* Move decompositions into py file dict

* speed up the basic approx generation

* add candidate checking by string

further reduces the runtime by a factor of about 1.8!

* use a tree for basic approx generation

Co-authored-by: Jake Lishman <jake.lishman@ibm.com>

* fix tests

* more speedups!

* use kdtree!

* refactor: generation as sep. function

* remove this masssssive file!

* start plugin work

* first attempt at synthesis plugin

* plugin itself works

-- but not when called via transpile or the UnitarySynthesis

* unitary synth works!

* use class-level method for SolovayKitaev

* docstrings

* add tests which cover still missing feature

* rename to SKSynth and better reno

* re-organize files

- algorithm to qiskit.synthesis
- plugin to qiskit.transpiler.passes.synthesis

* cover sklearn-free case

* missing future import

* move sk pass to transpiler subdirectory

* reno & try fixing slow optional module-level imports

* attempt 2 to fix sklearn import

* comments from Matthew's review

* directly construct dag

* (previous commit incomplete: missed this here)

* add SK to plugin toctree

* try fixing sphinx

* Apply Jake's comments

Removed the SK plugin from the plugin.py docs for now

* Fix doc example

* Update docs

* Fix tests

* Fix docs error

Co-authored-by: Julien Gacon <gaconju@gmail.com>
Co-authored-by: Luciano Bello <bel@zurich.ibm.com>
Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: New Feature Include in the "Added" section of the changelog Community PR PRs from contributors that are not 'members' of the Qiskit repo synthesis
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet