Repository: PennyLaneAI/pennylane
Pull request #1338 (2021-05-19 14:30:29)

Fix  ControlledPhaseShift
**Context:**

In PR #1064 the new ControlledPhaseShift was added, however the CNOT wires are fixed to 0 and 1 instead of the user input

**Description of the Change:** Wires in the CNOT decomposition are no longer fixed to 0 and 1
**Benefits:** Fixes new operation
**Possible Drawbacks:** None
**Related GitHub Issues:** None?


josh146: Wow! Great catch @ryanlevy. Thanks for this!

This is something that definitely should have been picked up by our tests, so a sign that our tests are lacking 🙂 Do you think you will be able to add a test that ensures the decomposition is correct for a weird set of wires (e.g., `wires=[2, 0]`)?

It could be something as simple as

```python
original_mat = qml.ControlledPhaseShift(x, wires=[2, 0]).matrix
decomposed_mats = [op.matrix for op in qml.ControlledPhaseShift.decomposition(x, wires=[2, 0])]
```
and performing matrix multiplication to ensure that the original matrix and decomposed matrix match.
josh146: By the way, don't forget to update the changelog with a link back to this PR, and add your name to the contributors section :)
codecov[bot]: # [Codecov](https://codecov.io/gh/PennyLaneAI/pennylane/pull/1338?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=PennyLaneAI) Report
> Merging [#1338](https://codecov.io/gh/PennyLaneAI/pennylane/pull/1338?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=PennyLaneAI) (cea3366) into [master](https://codecov.io/gh/PennyLaneAI/pennylane/commit/012704f3da025004115b28d706b61a2901d1f31e?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=PennyLaneAI) (012704f) will **not change** coverage.
> The diff coverage is `n/a`.

> :exclamation: Current head cea3366 differs from pull request most recent head 238ac54. Consider uploading reports for the commit 238ac54 to get more accurate results
[![Impacted file tree graph](https://codecov.io/gh/PennyLaneAI/pennylane/pull/1338/graphs/tree.svg?width=650&height=150&src=pr&token=9S3YzlUgGk&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=PennyLaneAI)](https://codecov.io/gh/PennyLaneAI/pennylane/pull/1338?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=PennyLaneAI)

```diff
@@           Coverage Diff           @@
##           master    #1338   +/-   ##
=======================================
  Coverage   98.15%   98.15%           
=======================================
  Files         145      145           
  Lines       11079    11079           
=======================================
  Hits        10875    10875           
  Misses        204      204           
```


| [Impacted Files](https://codecov.io/gh/PennyLaneAI/pennylane/pull/1338?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=PennyLaneAI) | Coverage Δ | |
|---|---|---|
| [pennylane/ops/qubit.py](https://codecov.io/gh/PennyLaneAI/pennylane/pull/1338/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=PennyLaneAI#diff-cGVubnlsYW5lL29wcy9xdWJpdC5weQ==) | `98.53% <ø> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/PennyLaneAI/pennylane/pull/1338?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=PennyLaneAI).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=PennyLaneAI)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/PennyLaneAI/pennylane/pull/1338?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=PennyLaneAI). Last update [012704f...238ac54](https://codecov.io/gh/PennyLaneAI/pennylane/pull/1338?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=PennyLaneAI). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=PennyLaneAI).

ryanlevy: Thanks for the reminder! 

I've updated the tests, but the `.matrix` is a bit lacking to really test things (CNOT's matrix is the same if `wires=[0,1]` or `wires=[1,0]` for example). Hope this is ok!
josh146: > CNOT's matrix is the same if wires=[0,1] or wires=[1,0] for example

Ah, of course! This is because PennyLane allows arbitrary labels for wires (e.g., `CNOT(wires=['a', 0])`), and as a consequence of this, wire labels have no implicit ordering.

Patch:
diff --git a/.github/CHANGELOG.md b/.github/CHANGELOG.md
index 64120bf97..e5e1b72d3 100644
--- a/.github/CHANGELOG.md
+++ b/.github/CHANGELOG.md
@@ -237,6 +237,9 @@ random_mat2 = rng.standard_normal(3, requires_grad=False)
 
 <h3>Bug fixes</h3>
 
+* Fixes incorrect wires in the decomposition of the `ControlledPhaseShift` operation.
+  [(#1338)](https://github.com/PennyLaneAI/pennylane/pull/1338)
+
 * Fixed tests for the `Permute` operation that used a QNode and hence expanded
   tapes twice instead of once due to QNode tape expansion and an explicit tape
   expansion call.
@@ -266,7 +269,7 @@ random_mat2 = rng.standard_normal(3, requires_grad=False)
 This release contains contributions from (in alphabetical order):
 
 Marius Aglitoiu, Vishnu Ajith, Thomas Bromley, Olivia Di Matteo, Tanya Garg, Diego Guala,
-Anthony Hayes, Josh Izaac, Pavan Jayasinha, Nahum Sá, Brian Shi, Antal Száva
+Anthony Hayes, Josh Izaac, Pavan Jayasinha, Ryan Levy, Nahum Sá, Brian Shi, Antal Száva
 
 # Release 0.15.1 (current release)
 
diff --git a/pennylane/ops/qubit.py b/pennylane/ops/qubit.py
index 9046a2728..bcefc7fd4 100644
--- a/pennylane/ops/qubit.py
+++ b/pennylane/ops/qubit.py
@@ -930,9 +930,9 @@ class ControlledPhaseShift(DiagonalOperation):
     def decomposition(phi, wires):
         decomp_ops = [
             qml.PhaseShift(phi / 2, wires=wires[0]),
-            qml.CNOT(wires=[0, 1]),
+            qml.CNOT(wires=wires),
             qml.PhaseShift(-phi / 2, wires=wires[1]),
-            qml.CNOT(wires=[0, 1]),
+            qml.CNOT(wires=wires),
             qml.PhaseShift(phi / 2, wires=wires[1]),
         ]
         return decomp_ops


Test patch:
diff --git a/tests/ops/test_qubit_ops.py b/tests/ops/test_qubit_ops.py
index f0a4d8d18..e7562fd0f 100644
--- a/tests/ops/test_qubit_ops.py
+++ b/tests/ops/test_qubit_ops.py
@@ -997,21 +997,51 @@ class TestOperations:
     @pytest.mark.parametrize("phi", [-0.1, 0.2, 0.5])
     @pytest.mark.parametrize("cphase_op", [qml.ControlledPhaseShift, qml.CPhase])
     def test_controlled_phase_shift_decomp(self, phi, cphase_op):
-        """Tests that the ControlledPhaseShift and CPhase operation calculates the correct decomposition"""
-        op = cphase_op(phi, wires=[0, 1])
-        decomp = op.decomposition(phi, wires=[0, 1])
+        """Tests that the ControlledPhaseShift and CPhase operation
+        calculates the correct decomposition"""
+        op = cphase_op(phi, wires=[0, 2])
+        decomp = op.decomposition(phi, wires=[0, 2])
 
         mats = []
         for i in reversed(decomp):
             if i.wires.tolist() == [0]:
-                mats.append(np.kron(i.matrix, np.eye(2)))
+                mats.append(np.kron(i.matrix, np.eye(4)))
             elif i.wires.tolist() == [1]:
-                mats.append(np.kron(np.eye(2), i.matrix))
-            else:
-                mats.append(i.matrix)
+                mats.append(np.kron(np.eye(2), np.kron(i.matrix, np.eye(2))))
+            elif i.wires.tolist() == [2]:
+                mats.append(np.kron(np.eye(4), i.matrix))
+            elif isinstance(i, qml.CNOT) and i.wires.tolist() == [0, 1]:
+                mats.append(np.kron(i.matrix, np.eye(2)))
+            elif isinstance(i, qml.CNOT) and i.wires.tolist() == [0, 2]:
+                mats.append(
+                    np.array(
+                        [
+                            [1, 0, 0, 0, 0, 0, 0, 0],
+                            [0, 1, 0, 0, 0, 0, 0, 0],
+                            [0, 0, 1, 0, 0, 0, 0, 0],
+                            [0, 0, 0, 1, 0, 0, 0, 0],
+                            [0, 0, 0, 0, 0, 1, 0, 0],
+                            [0, 0, 0, 0, 1, 0, 0, 0],
+                            [0, 0, 0, 0, 0, 0, 0, 1],
+                            [0, 0, 0, 0, 0, 0, 1, 0],
+                        ]
+                    )
+                )
 
         decomposed_matrix = np.linalg.multi_dot(mats)
-        exp = ControlledPhaseShift(phi)
+        lam = np.exp(1j * phi)
+        exp = np.array(
+            [
+                [1, 0, 0, 0, 0, 0, 0, 0],
+                [0, 1, 0, 0, 0, 0, 0, 0],
+                [0, 0, 1, 0, 0, 0, 0, 0],
+                [0, 0, 0, 1, 0, 0, 0, 0],
+                [0, 0, 0, 0, 1, 0, 0, 0],
+                [0, 0, 0, 0, 0, lam, 0, 0],
+                [0, 0, 0, 0, 0, 0, 1, 0],
+                [0, 0, 0, 0, 0, 0, 0, lam],
+            ]
+        )
 
         assert np.allclose(decomposed_matrix, exp)