-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
two-qubit decomposition into sqrt(iSWAP) + approximation #9375
base: main
Are you sure you want to change the base?
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
With our gate naming scheme, should this be called |
I started the branch with that naming scheme but quickly got annoyed at all the up and down of the camel case :) |
Also @Cryoris would you like to review this? :) |
Pull Request Test Coverage Report for Build 4340575136
💛 - Coveralls |
ef8dcfc
to
d2e2f91
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The decomposition looks mostly good! I only encountered one case where the final decomposition doesn't actually include the SQiSWGate
but just an Gate
with the same name, I think due to taking the inverse of the decomposition. We could e.g. fix that by adding a SQiSWdgGate
but maybe there's also another solution. Here's a reproducing example:
import numpy as np
from qiskit.circuit import QuantumCircuit
from qiskit.circuit.library import SQiSWGate
from qiskit.synthesis.two_qubit import SQiSWDecomposer
from qiskit.quantum_info import Operator
circuit = QuantumCircuit(2)
circuit.rxx(0.2, 0, 1)
circuit.ryy(0.2, 0, 1)
circuit.rzz(0.2, 0, 1)
op = Operator(circuit).data
decomp = SQiSWDecomposer(euler_basis=["u"])
out = decomp(op)
print(out.draw())
for inst in out.data:
if len(inst.qubits) == 2:
print(type(inst.operation))
which prints
global phase: 2.6038
┌─────────────────────┐┌────────┐ ┌─────────────────────────┐ ┌────────┐┌────────────────────────────┐┌────────┐┌────────────────────────────┐
q_0: ┤ U(0,2.2455,-2.2455) ├┤0 ├─┤ U(2.9419,1.2433,2.4222) ├──┤0 ├┤ U(0.34939,2.4962,-0.64543) ├┤0 ├┤ U(2.9419,-0.71935,-1.8983) ├
└──┬────────────────┬─┘│ sqisw │┌┴─────────────────────────┴─┐│ sqisw │└──┬─────────────────────┬───┘│ sqisw │├────────────────────────────┤
q_1: ───┤ U(-π,π/2,-π/2) ├──┤1 ├┤ U(-2.2067,-2.4822,0.54952) ├┤1 ├───┤ U(-1.0943,-π/2,π/2) ├────┤1 ├┤ U(-0.93488,2.5921,-2.4822) ├
└────────────────┘ └────────┘└────────────────────────────┘└────────┘ └─────────────────────┘ └────────┘└────────────────────────────┘
<class 'qiskit.circuit.gate.Gate'> # <-- should be type SQiSWGate!
<class 'qiskit.circuit.gate.Gate'>
<class 'qiskit.circuit.gate.Gate'>
Concerning the gate itself: I think it would be more consistent to go back to the name SiSwapGate
to be consistent with our naming scheme. We've also moved away from other aliases like "Fredkin" or "Toffoli" in favor of consistency. I'd be open to add an alias SQiSW = SiSwapGate
but I think it would be better if the base class would follow our convention 🙂
for instruction in inverse_decomposition: | ||
if isinstance(instruction.operation, SQiSWGate): | ||
inverse_decomposition_with_sqisw_dg.z(0) | ||
inverse_decomposition_with_sqisw_dg.append(SQiSWGate().inverse(), [0, 1]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand this correctly, the final decomposition should contain a SQiSWGate
right? Won't inverting this twice end up with an Instruction
which though called "sqisw" is not actually an instance of SQiSWGate
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started trying to fix this as you suggest but thinking about it I think the overhead of adding a new gate (and all that goes along with it like definitions and adding to equivalence library) is not worth it. This problem has to be fixed in Qiskit such that taking the inverse of a gate twice gives the same gate. I opened an issue on this 18 months ago #6879. This has become a higher priority recently and I think once it gets fixed this automatically gets fixed too. Right now it's not really a problem because if the device supports siswap gate then the generated qasm is correct.
|
||
|
||
@ddt | ||
class TestSQiSWSynth(QiskitTestCase): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to add a test for each of the different regions (green, blue, brown and red) in the paper so we're sure each of the cases is triggered 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i added a bunch of tests
qiskit/synthesis/__init__.py
Outdated
.. autosummary:: | ||
:toctree: ../stubs/ | ||
|
||
SQiSWDecomposer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it be called SQiSWDecomposer
or SQiSWDecomposition
like SolovayKitaevDecomposition
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now it's named like TwoQubitBasisDecomposer and XXDecomposer which are more related to this class as they all take SU(4) and decompose it over some basis.
I agree we need consistency and my intent is to do this as I move stuff over to qiskit.synthesis (over a few different PRs)
|
||
class SQiSWDecomposer: | ||
""" | ||
A class for decomposing 2-qubit unitaries into at most 3 uses of the sqrt(iSWAP) gate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps it's worth to explain somewhere the difference from the usual two-qubit decomposition, and when to use each of the algorithms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would still like to check the docs, after they will be generated when the conflict is solved.
3561ce2
to
290760e
Compare
I addressed the review comments. Also I added approximation which is a good boost to the performance of the method (details in the PR description). I also added more corner case tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates and the additional tests! I left some more comments below. 🙂
Returns: | ||
QuantumCircuit: Synthesized circuit. | ||
""" | ||
if not approximate: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a more general question: isn't basis_fidelity
and approximate
redundant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm good question. I was copying the structure of the XXDecomposer code. I can think about removing approximate
but I have to see if there is some underlying reason. I'm making some changes to the whole qiskit.synthesis folder so I can have this on the list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
approximate
is used by XXDecomposer
only when computing infidelities. The method used doesn't depend on the values of the fidelities in basis_fidelity
, in particular does not depend on whether they are less than one. I don't know "less than one" means, maybe it's advice on what to choose if you know you are passing 1.0. The same holds if basis_fidelity
is not a dict
, but a float, as it is here. In this case infidelities are computed for several values of strength
(which corresponds to a rotation angle I think) that are hardcoded in XXDecomposer
. The infidelities are computed from the strengths by a linear model. If approximate
is True
the parameters of the linear model are determined from basis_fidelity
. If approximate
is False
then basis_fidelity
is ignored completely and infidelities are computed without any reference to parameters passed when calling the object nor those passed when constructing it. That's sort of the reverse of what I would expect, but that's what the code does.
circuit = circuit.compose(red_decomp, [0, 1]) | ||
if eigenphase_crossing: | ||
# y, z -> -z, -y | ||
circuit = circuit.compose(XGate().power(1 / 2), [0], front=True) |
There was a problem hiding this comment.
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 not use the SX(dg)Gate
for this? 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm no its the same
if abs(C) < _EPS: | ||
C = 0.0 | ||
|
||
α = np.arccos(np.cos(2 * x) - np.cos(2 * y) + np.cos(2 * z) + 2 * np.sqrt(C)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know what the best practice for non-ASCII characters is -- my feeling would say to rather use alpha
etc to make life easier for future developers 😉 but I'm not fixed on this if you want to keep it as is 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's just nicer i think to read :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like reading
larger volume when approximation is enabled. | ||
|
||
Args: | ||
euler_basis (list(str)): single-qubit gates basis in the decomposition. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about adding an example on how to use this class here? 🙂 Would be nice to advertise this feature to users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add.
|
||
An associated decomposition method that synthesizes arbitrary two-qubit | ||
unitaries into two or three applications of SiSwap has been added in | ||
:class:`.SiSwapDecomposer`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be cool to show an example in the release notes, too 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add.
# copyright notice, and modified files need to carry a notice indicating | ||
# that they have been altered from the originals. | ||
|
||
# pylint: disable=invalid-name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this only because of the variable u
? If yes maybe we could just rename that one and avoid a global disable 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes. these variable naming rules are pretty annoying I think. I think we need to relax this at the qiskit level. Like why should naming my unitary u
be a problem? @mtreinish what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm in favour of relaxing the lint-enforced naming rules in general, although I've got to say the code in this PR isn't exactly the greatest advert for it. There's a lot of mix-and-match of variable styles with no clear relation to the mathematical paper they're drawn from (the non-iSWAP-specific code), and "why should naming my unitary u
be a problem?" is indicative - u
is mostly a bad name, because the majority of things we deal with in Qiskit are unitary. In tests you can use some name that identifies it as the base of the test, and in synthesis code, I'm guessing there's a lot of different unitaries kicking around at different points, so they ought to have proper names as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But often the main point of a function is to do something with a single unitary. In this case naming it unitary
seems reasonable. XXDecomposer
does this and it looks pretty good there. I don't like u
, but if I used a editor that had good support for Python semantics, I might not mind it. I have to distinguish occurrences of u
visually. If it's really just as easy with a modern editor to deal with, I'd say go ahead and use it.
|
||
class SiSwapDecomposer: | ||
""" | ||
A class for decomposing 2-qubit unitaries into at most 3 uses of the sqrt(iSWAP) gate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the PR text you're mentioning this code differs from the paper, would it make sense to summarize the differences either here or as comments in the code to know what's going on when we come back to this code in the future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add.
Co-authored-by: Julien Gacon <gaconju@gmail.com>
Co-authored-by: Julien Gacon <gaconju@gmail.com>
Co-authored-by: Julien Gacon <gaconju@gmail.com>
Standard gates | ||
============================================================= | ||
Standard gates (:mod:`qiskit.circuit.library.standard_gates`) | ||
============================================================= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's been a bad merge here - this file had its docstring reduced to a single by Shelly just recently to avoid mistakes in the documentation, and it should stay a single line. This file isn't built into the documentation at all (the only changes needed are to qiskit/circuit/library/__init__.py
), but we've had a few cases where this file was erroneously updated instead only, leading to missing documentation.
It's better to put this back to just being one line, but the import should stay.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, see the details in PR #9668. Please add this gate to the docs in the init file of circuit/library.
from qiskit.synthesis.su4.utils import average_infidelity | ||
|
||
from .circuits import apply_reflection, apply_shift, canonical_xx_circuit | ||
from .utilities import EPSILON | ||
from .polytopes import XXPolytope | ||
|
||
|
||
def _average_infidelity(p, q): | ||
""" | ||
Computes the infidelity distance between two points p, q expressed in positive canonical | ||
coordinates. | ||
""" | ||
|
||
a0, b0, c0 = p | ||
a1, b1, c1 = q | ||
|
||
return 1 - 1 / 20 * ( | ||
4 | ||
+ 16 | ||
* ( | ||
math.cos(a0 - a1) ** 2 * math.cos(b0 - b1) ** 2 * math.cos(c0 - c1) ** 2 | ||
+ math.sin(a0 - a1) ** 2 * math.sin(b0 - b1) ** 2 * math.sin(c0 - c1) ** 2 | ||
) | ||
) | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
qiskit.quantum_info
shouldn't be able to access qiskit.synthesis
, because the latter is way higher up in the hierarchy. Of course, the actual solution is that all the code currently in qiskit.quantum_info.synthesis
, and at least the bits that are concerned with circuit objects not just matrices, ought to move into qiskit.synthesis
, but that's a problem for a different PR.
For right now, it might be a shade better just to eat this code duplication right now to avoid creating even larger circular import cycles - I saw you already hit one caused by this in a file above, because you had to make an import more specific.
self.definition = qc | ||
|
||
def __array__(self, dtype=None): | ||
"""Return a numpy.array for the DCX gate.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"""Return a numpy.array for the DCX gate.""" | |
"""Return a numpy.array for the :math:`\\sqrt{iSWAP}` gate.""" |
perhaps?
q = QuantumRegister(2, "q") | ||
qc = QuantumCircuit(q, name=self.name) | ||
rules = [(RXXGate(-np.pi / 4), [q[0], q[1]], []), (RYYGate(-np.pi / 4), [q[0], q[1]], [])] | ||
for instr, qargs, cargs in rules: | ||
qc._append(instr, qargs, cargs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're going to use the _append
form, we ought to construct a validated CircuitInstruction
to add - the form of allowing qargs
and cargs
to _append
will be deprecated, as part of drawing a clearer distinction between the responsibilities of the caller of append
and the caller of _append
.
If you don't want to worry about it, it shouldn't really matter for performance to just use append
.
def __init__(self, euler_basis: Optional[List[str]]): | ||
# decomposer for the local single-qubit gates | ||
from qiskit.transpiler.passes.optimization.optimize_1q_decomposition import ( | ||
Optimize1qGatesDecomposition, # pylint: disable=cyclic-import | ||
) | ||
|
||
euler_basis = euler_basis or ["u"] | ||
self._decomposer1q = Optimize1qGatesDecomposition(euler_basis) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know how much you mind about the type hints, but List
is only very rarely what's meant when it's in an input position. Most likely, you just want anything that can be converted to a list, which is typing.Iterable[str]
. If so, you'd need to change your assignment code to euler_basis = ["u"] if euler_basis is None else list(euler_basis)
to be properly correct.
It also probably doesn't matter, but your current code will convert euler_basis=[]
to ["u"]
. I don't know if that's intended, or if it's something you'd want Optimize1qGatesDecomposition
to throw an error on. I would also mention that the more ideal case would be making a dummy Target
than passing the basis directly, because Matthew wants to start getting all transpiler passes transitioned to remove the old-style interfaces for easier maintainability moving forwards.
Function from | ||
https://scipy-cookbook.readthedocs.io/items/Finding_Convex_Hull_Minimum_Point.html |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear to me what the licence of this code is. The best I can find is in the repo of the docs website, which has a stipulation that source-code reproductions must include their licence header and binary distributions must include it in their documentation. The licence comes from the times when Scipy was part of Enthought.
I think we need to be more careful about just copy-pasting code like this - it's not clear to me if we're actually safe to use this.
return 1 - 1 / 20 * ( | ||
4 | ||
+ 16 | ||
* ( | ||
np.cos(a0 - a1) ** 2 * np.cos(b0 - b1) ** 2 * np.cos(c0 - c1) ** 2 | ||
+ np.sin(a0 - a1) ** 2 * np.sin(b0 - b1) ** 2 * np.sin(c0 - c1) ** 2 | ||
) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar comment about preferring math
functions if appropriate here.
# copyright notice, and modified files need to carry a notice indicating | ||
# that they have been altered from the originals. | ||
|
||
# pylint: disable=invalid-name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm in favour of relaxing the lint-enforced naming rules in general, although I've got to say the code in this PR isn't exactly the greatest advert for it. There's a lot of mix-and-match of variable styles with no clear relation to the mathematical paper they're drawn from (the non-iSWAP-specific code), and "why should naming my unitary u
be a problem?" is indicative - u
is mostly a bad name, because the majority of things we deal with in Qiskit are unitary. In tests you can use some name that identifies it as the base of the test, and in synthesis code, I'm guessing there's a lot of different unitaries kicking around at different points, so they ought to have proper names as well.
u = random_unitary(4, seed=seed) | ||
decomposer = SiSwapDecomposer(euler_basis=["rz", "ry"]) | ||
circuit = decomposer(u) | ||
self.assertLessEqual(circuit.count_ops().get("siswap", 0), 3) | ||
self.assertEqual(Operator(circuit), Operator(u)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In all these in this file, should we also be testing that the only gates in the output are those in the Euler basis as well?
@combine( | ||
p=[ | ||
[np.pi / 4, np.pi / 6, -np.pi / 12], # red and blue intersection (bottom face) | ||
[np.pi / 4, np.pi / 6, np.pi / 12], # inverse of above (and locally equivalent) | ||
[np.pi / 4, np.pi / 8, np.pi / 8], # half-way between CX and SWAP (peak red polytope) | ||
[np.pi / 4, np.pi / 8, -np.pi / 8], # inverse of above (and locally equivalent) | ||
[np.pi / 4, np.pi / 16, np.pi / 16], # quarter-way between CX and SWAP | ||
[np.pi / 4, np.pi / 16, np.pi / 16], # inverse of above (and locally equivalent) | ||
[np.pi / 6, np.pi / 8, np.pi / 24], # red and blue and green intersection | ||
[np.pi / 6, np.pi / 8, -np.pi / 24], # inverse of above (not locally equivalent) | ||
[np.pi / 16, np.pi / 24, np.pi / 48], # red and blue and purple intersection | ||
[np.pi / 16, np.pi / 24, -np.pi / 48], # inverse of above (not locally equivalent) | ||
[np.pi / 6, np.pi / 12, -np.pi / 16], # inside red polytope | ||
] | ||
) | ||
def test_siswap_special_points(self, p): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's really good to see this test - also great to have very explicit testing of a load of points we know to be special.
@@ -113,6 +113,7 @@ | |||
SdgGate | |||
SwapGate | |||
iSwapGate | |||
SiSwapGate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SiSwap Gate should also be added to the documentation in this init file, and removed from the docs in circuit/library/standard_gates.
See the following PR: #9668
Is there any update on this? Is this something we want to target for 0.45.0? |
Hello, hoping this feature gets merged eventually. |
Summary
Adds the sqrt(iSWAP) (
SiSwapGate
) as well as a method to decompose into it that uses 2 applications for ~79% of Haar-random unitaries and 3 applications for the rest.Based on https://arxiv.org/pdf/2105.06074v3.pdf. This does not exactly follow the paper but is inspired by that result. The algorithm given in the paper seems to have some bugs so this is a different implementation.
In addition this PR implements an approximate synthesis, which results in further reductions: for ~95% of Haar-random unitaries 2-applications of SiSwap suffice when the gate has 99% fidelity.
Optimal approximation is found by distance in the Weyl chamber, which is a proxy for trace distance. The algorithm approximates more when the gate has less fidelity.
I added extensive tests because there are lots and lots of corner cases.
Co-authored by @thomasverrill. Closes #9333