Remove fancy ctrl decomps (#4065)

* bugfix

* changelog

* Apply suggestions from code review

Co-authored-by: Tom Bromley <[email protected]>

* mark tests for old behavior xfail

---------

Co-authored-by: Lee James O'Riordan <[email protected]>
Co-authored-by: Tom Bromley <[email protected]>

doc/releases/changelog-0.30.0.md

@@ -209,38 +209,31 @@
   [(#3851)](https://github.com/PennyLaneAI/pennylane/pull/3851)
 
   Three decompositions from [arXiv:2302.06377](https://arxiv.org/abs/2302.06377) are provided and
-  compare favourably to the already-available ZYZ decomposition:
+  compare favourably to the already-available `qml.ops.ctrl_decomp_zyz`:
 
   ```python
   wires = [0, 1, 2, 3, 4, 5]
   control_wires = wires[1:]
 
-  op = qml.RX(np.pi / 2, wires=0)
+  @qml.qnode(qml.device('default.qubit', wires=6))
+  def circuit():
+      with qml.QueuingManager.stop_recording():
+          # the decomposition does not un-queue the target
+          target = qml.RX(np.pi/2, wires=0)
+      qml.ops.ctrl_decomp_bisect(target, (1, 2, 3, 4, 5))
+      return qml.state()
 
-  with qml.tape.QuantumTape() as tape:
-      qml.ctrl(op, control=control_wires)
-
-  with qml.tape.QuantumTape() as zyz_tape:
-      qml.RZ(np.pi / 2, wires=0)
-      qml.RY(np.pi / 4, wires=0)
-      qml.MultiControlledX(wires=control_wires + [0], control_values="11111", work_wires=[6, 7, 8])
-      qml.RY(-np.pi / 4, wires=0)
-      qml.MultiControlledX(wires=control_wires + [0], control_values="11111", work_wires=[6, 7, 8])
-      qml.RZ(-np.pi / 2, wires=0)
+  print(qml.draw(circuit, expansion_strategy="device")())
   ```
 
-  Fewer CNOT gates are used:
-
-  ```pycon
-  >>> tape.expand(depth=5).specs["gate_types"]["CNOT"]
-  60
-  >>> zyz_tape.expand(depth=5).specs["gate_types"]["CNOT"]
-  144
   ```
-
-  The decompositions are applied automatically when expanding tapes or decomposing operations in
-  PennyLane, but can also be accessed directly using
-  [ctrl_decomp_bisect()](https://docs.pennylane.ai/en/stable/code/api/pennylane.ops.op_math.ctrl_decomp_bisect.html).
+  0: ──H─╭X──U(M0)─╭X──U(M0)†─╭X──U(M0)─╭X──U(M0)†──H─┤  State
+  1: ────├●────────│──────────├●────────│─────────────┤  State
+  2: ────├●────────│──────────├●────────│─────────────┤  State
+  3: ────╰●────────│──────────╰●────────│─────────────┤  State
+  4: ──────────────├●───────────────────├●────────────┤  State
+  5: ──────────────╰●───────────────────╰●────────────┤  State
+  ```
 
 * A new decomposition to `qml.SingleExcitation` has been added that halves the number of
   CNOTs required.
@@ -635,6 +628,10 @@ Nothing for this release!
 
 <h3>Bug fixes 🐛</h3>
 
+* `ctrl_decomp_bisect` and `ctrl_decomp_zyz` are no longer used by default when decomposing
+  controlled operations due to the presence of a global phase difference in the zyz decomposition of some target operators.
+  [(#4065)](https://github.com/PennyLaneAI/pennylane/pull/4065)
+
 * Fixed a bug where `qml.math.dot` returned a numpy array instead of an autograd array, breaking autograd derivatives
   in certain circumstances.
   [(#4019)](https://github.com/PennyLaneAI/pennylane/pull/4019)

pennylane/ops/op_math/controlled.py

@@ -30,10 +30,6 @@
 from pennylane.wires import Wires
 
 from .symbolicop import SymbolicOp
-from .controlled_decompositions import (
-    ctrl_decomp_zyz,
-    ctrl_decomp_bisect,
-)
 
 
 def ctrl(op, control, control_values=None, work_wires=None):
@@ -478,8 +474,8 @@ def has_decomposition(self):
             return True
         if isinstance(self.base, qml.PauliX):
             return True
-        if len(self.base.wires) == 1 and getattr(self.base, "has_matrix", False):
-            return True
+        # if len(self.base.wires) == 1 and getattr(self.base, "has_matrix", False):
+        #    return True
         if self.base.has_decomposition:
             return True
 
@@ -572,16 +568,16 @@ def _decompose_no_control_values(op: "operation.Operator") -> List["operation.Op
     if isinstance(op.base, qml.PauliX):
         # has some special case handling of its own for further decomposition
         return [qml.MultiControlledX(wires=op.active_wires, work_wires=op.work_wires)]
-    if (
-        len(op.base.wires) == 1
-        and len(op.control_wires) >= 2
-        and getattr(op.base, "has_matrix", False)
-        and qmlmath.get_interface(*op.data) == "numpy"  # as implemented, not differentiable
-    ):
-        # Bisect algorithms use CNOTs and single qubit unitary
-        return ctrl_decomp_bisect(op.base, op.control_wires)
-    if len(op.base.wires) == 1 and getattr(op.base, "has_matrix", False):
-        return ctrl_decomp_zyz(op.base, op.control_wires)
+    # if (
+    #    len(op.base.wires) == 1
+    #    and len(op.control_wires) >= 2
+    #    and getattr(op.base, "has_matrix", False)
+    #    and qmlmath.get_interface(*op.data) == "numpy"  # as implemented, not differentiable
+    # ):
+    # Bisect algorithms use CNOTs and single qubit unitary
+    #    return ctrl_decomp_bisect(op.base, op.control_wires)
+    # if len(op.base.wires) == 1 and getattr(op.base, "has_matrix", False):
+    #    return ctrl_decomp_zyz(op.base, op.control_wires)
 
     if not op.base.has_decomposition:
         return None

tests/ops/op_math/test_controlled.py

@@ -1520,6 +1520,7 @@ def test_qubit_unitary(M):
     assert not equal_list(list(tape), expected)
 
 
+@pytest.mark.xfail
 @pytest.mark.parametrize(
     "M",
     [

tests/ops/op_math/test_controlled_decompositions.py

@@ -201,6 +201,7 @@ def test_trivial_ops_in_decomposition(self):
         assert len(decomp) == 5
         assert all(qml.equal(o, e) for o, e in zip(decomp, expected))
 
+    @pytest.mark.xfail
     @pytest.mark.parametrize("test_expand", [False, True])
     def test_zyz_decomp_no_control_values(self, test_expand):
         """Test that the ZYZ decomposition is used for single qubit target operations
@@ -225,6 +226,7 @@ def test_zyz_decomp_no_control_values(self, test_expand):
         expected = qml.ops.ctrl_decomp_zyz(base, (0,))
         assert equal_list(decomp, expected)
 
+    @pytest.mark.xfail
     @pytest.mark.parametrize("test_expand", [False, True])
     def test_zyz_decomp_control_values(self, test_expand):
         """Test that the ZYZ decomposition is used for single qubit target operations
@@ -627,6 +629,7 @@ def expected_circuit():
         expected = expected_circuit()
         assert np.allclose(res, expected, atol=tol, rtol=tol)
 
+    @pytest.mark.xfail
     @pytest.mark.parametrize("op", zip(gen_ops, gen_ops_best))
     @pytest.mark.parametrize("control_wires", cw5)
     @pytest.mark.parametrize("all_the_way_from_ctrl", [False, True])