Tweak swap update algorithm to ensure termination (#164)

* add a test case that reproduces the swap updater infinite loop, and fix it by keeping track of previously used swaps to prevent cycles

Author: weinstein

recirq/quantum_chess/swap_updater.py

@@ -106,7 +106,11 @@ def __init__(self,
         self.dlists = mcpe.DependencyLists(circuit)
         self.mapping = mcpe.QubitMapping(initial_mapping)
         self.swap_factory = swap_factory
-        self.pairwise_distances = _pairwise_shortest_distances(self.device_qubits)
+        self.pairwise_distances = _pairwise_shortest_distances(
+            self.device_qubits)
+        # Tracks swaps that have been made since the last circuit gate was
+        # output.
+        self.prev_swaps = set()
 
     def _distance_between(self, q1: cirq.GridQubit, q2: cirq.GridQubit) -> int:
         """Returns the precomputed length of the shortest path between two qubits."""
@@ -123,11 +127,12 @@ def generate_candidate_swaps(
         """
         for gate in gates:
             for gate_q in gate.qubits:
-                yield from (
-                    (gate_q, swap_q)
-                    for swap_q in gate_q.neighbors(self.device_qubits)
-                    if mcpe.effect_of_swap((gate_q, swap_q), gate.qubits,
-                                           self._distance_between) > 0)
+                for swap_q in gate_q.neighbors(self.device_qubits):
+                    swap_qubits = (gate_q, swap_q)
+                    effect = mcpe.effect_of_swap(swap_qubits, gate.qubits,
+                                                 self._distance_between)
+                    if swap_qubits not in self.prev_swaps and effect > 0:
+                        yield swap_qubits
 
     def _mcpe(self, swap_q1: cirq.GridQubit, swap_q2: cirq.GridQubit) -> int:
         """Returns the maximum consecutive positive effect of swapping two qubits."""
@@ -152,6 +157,7 @@ def update_iteration(self) -> Generator[cirq.Operation, None, None]:
                 # and added to the final circuit.
                 for q in gate.qubits:
                     self.dlists.pop_front(q)
+                self.prev_swaps.clear()
                 yield physical_gate
             else:
                 # physical_gate needs to be fixed up with some swaps.
@@ -171,6 +177,8 @@ def update_iteration(self) -> Generator[cirq.Operation, None, None]:
             # connectivity graph.
             raise ValueError("no swaps founds that will improve the circuit")
         chosen_swap = max(candidates, key=lambda swap: self._mcpe(*swap))
+        self.prev_swaps.add(chosen_swap)
+        self.prev_swaps.add(tuple(reversed(chosen_swap)))
         self.mapping.swap_physical(*chosen_swap)
         yield from self.swap_factory(*chosen_swap)
 

recirq/quantum_chess/swap_updater_test.py

@@ -196,3 +196,65 @@ def test_disconnected_components():
             cirq.Circuit(
                 SwapUpdater(circuit, allowed_qubits,
                             initial_mapping).add_swaps()))
+
+
+def test_termination_in_local_minimum():
+    grid_2x5 = cirq.GridQubit.rect(2, 5)
+    q = list(cirq.NamedQubit(f'q{i}') for i in range(6))
+    # The initial mapping looks like:
+    #   _|_0_|_1_|_2_|_3_|_4_|
+    #   0|q0 |   |   |   |q4 |
+    #   1|q1 |q2 |   |q3 |q5 |
+    initial_mapping = {
+        q[0]: cirq.GridQubit(0, 0),
+        q[1]: cirq.GridQubit(1, 0),
+        q[2]: cirq.GridQubit(1, 1),
+        q[3]: cirq.GridQubit(1, 3),
+        q[4]: cirq.GridQubit(0, 4),
+        q[5]: cirq.GridQubit(1, 4),
+    }
+    # Here's the idea:
+    #   * there are two "clumps" of qubits: (q0,q1,q2) and (q3,q4,q5)
+    #   * the active gate(s) span the two clumps
+    #   * there are also gates on qubits within clumps
+    #   * intra-clump gate cost contribution outweighs inter-clump gate cost
+    # In that case, we need to swap qubits away from their respective clumps in
+    # order to progress beyond any of the active gates. But we never will
+    # because doing so would increase the overall cost due to the intra-clump
+    # contributions. In fact, no greedy algorithm would be able to make progress
+    # in this case.
+    circuit = cirq.Circuit()
+    # Cross-clump active gates
+    circuit.append(
+        [cirq.CNOT(q[0], q[3]),
+         cirq.CNOT(q[1], q[4]),
+         cirq.CNOT(q[2], q[5])])
+    # Intra-clump q0,q1,q2
+    circuit.append(
+        [cirq.CNOT(q[0], q[1]),
+         cirq.CNOT(q[0], q[2]),
+         cirq.CNOT(q[1], q[2])])
+    # Intra-clump q3,q4,q5
+    circuit.append(
+        [cirq.CNOT(q[3], q[4]),
+         cirq.CNOT(q[3], q[5]),
+         cirq.CNOT(q[4], q[5])])
+
+    updater = SwapUpdater(circuit, grid_2x5, initial_mapping,
+                          lambda q1, q2: [cirq.SWAP(q1, q2)])
+    # Iterate until the SwapUpdater is finished or an assertion fails, keeping
+    # track of the ops generated by the previous iteration.
+    prev_it = list(updater.update_iteration())
+    while not updater.dlists.all_empty():
+        cur_it = list(updater.update_iteration())
+
+        # If the current iteration adds a SWAP, it better not be the same SWAP
+        # as the previous iteration...
+        # If we pick the same SWAP twice in a row, then we're going in a loop
+        # forever without making any progress!
+        def _is_swap(ops):
+            return len(ops) == 1 and ops[0] == cirq.SWAP(*ops[0].qubits)
+
+        if _is_swap(prev_it) and _is_swap(cur_it):
+            assert set(prev_it[0].qubits) != set(cur_it[0].qubits)
+        prev_it = cur_it