Absolute tolerance on ZXZ one qubit decomposition

[boschmitt]

While looking at the code for one qubit decomposition I found a small difference between ZXZ decomposition and the other ones ('ZYZ, XYX'):

https://github.com/Qiskit/qiskit-terra/blob/62383eb90e1d3fc9790ceef9789e8f842e671a1e/qiskit/quantum_info/synthesis/one_qubit_decompose.py#L320

The following diff show the difference between _circuit_zyz and _circuit_zxz. Apart from the expected RYGate and RXGate, we also see the function _mod_2pi is called without atol, which might be a bug.

@@ -1,11 +1,11 @@
-    def _circuit_zyz(theta, phi, lam, phase, simplify=True, atol=DEFAULT_ATOL):
+    def _circuit_zxz(theta, phi, lam, phase, simplify=True, atol=DEFAULT_ATOL):
         gphase = phase - (phi + lam) / 2
         qr = QuantumRegister(1, "qr")
         circuit = QuantumCircuit(qr)
         if not simplify:
             atol = -1.0
         if abs(theta) < atol:
-            tot = _mod_2pi(phi + lam, atol)
+            tot = _mod_2pi(phi + lam)
             if abs(tot) > atol:
                 circuit._append(RZGate(tot), [qr[0]], [])
                 gphase += tot / 2
@@ -18,7 +18,7 @@
         if abs(lam) > atol:
             gphase += lam / 2
             circuit._append(RZGate(lam), [qr[0]], [])
-        circuit._append(RYGate(theta), [qr[0]], [])
+        circuit._append(RXGate(theta), [qr[0]], [])
         phi = _mod_2pi(phi, atol)
         if abs(phi) > atol:
             gphase += phi / 2

If this is indeed a bug, then we could refactor the code of the three functions: _circuit_zxz, _circuit_zyz and _circuit_xyx. We could merge these three functions into one that receive the appropriate gates as parameters. (Like: _circuit_psx_gen)

[levbishop]

Well spotted. I took a look at this and I think the right thing is to add the atol for the zxz case as you suggest. Refactoring this as you describe makes sense to me, but I think @ajavadia might have some opinions about this kind of thing.

[ajavadia]

Yes I agree. There's a good amount of duplication here and refactoring makes sense to me.

[levbishop]

If we could have a generic Euler fn that would also support XZX etc then @ewinston would be able to use it in #6581

[levbishop]

@boschmitt is there anything left here now that #6553 has merged?

[boschmitt]

@boschmitt is there anything left here now that #6553 has merged?

I only had time to briefly look over the changes, but looks like there is nothing left. Since it appears that was not a big problem to begin with, only a warning flag, I will simply close the issue.