Support routing for directed device graphs#7810
Conversation
|
Added additional tests to cover edge cases for directed device graphs and the tag_inserted_swaps parameter. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7810 +/- ##
==========================================
- Coverage 99.62% 99.61% -0.01%
==========================================
Files 1104 1104
Lines 98959 99067 +108
==========================================
+ Hits 98583 98690 +107
- Misses 376 377 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
bced992 to
e71ea5f
Compare
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
Used pylint disable for argument-count warnings Refactoring to reduce args would change public API, breaking |
|
@tanujkhattar ! can you review this pr. |
| mm.int_to_physical_qid[mm.logical_to_physical[swap[0]]], | ||
| mm.int_to_physical_qid[mm.logical_to_physical[swap[1]]], |
There was a problem hiding this comment.
Can we still use mm.int_to_logical_qid like we did earlier?
There was a problem hiding this comment.
Yes, we still use mm.int_to_logical_qid and mm.int_to_physical_qid exactly as before. The
_route method uses mm to map logical qubits to physical qubits when emitting SWAPs, and the return statement in
route_circuit builds the swap permutation map using mm.int_to_logical_qid[k]. The only change is that we now emit standard SWAP gates during routing, and the directional decomposition happens in a separate post-processing step using map_operations_and_unroll
There was a problem hiding this comment.
Out of curiosity -- Was this is an AI generated answer? It feels like one (nothing wrong with it, I'm just curious :))
There was a problem hiding this comment.
when you specially asked about mm.int_to_logical_qid i thought did i break the code some where else. to cross check myself i used ai to understand where mm is used .
| qubit_pair: tuple[cirq.Qid, cirq.Qid], | ||
| device_graph: nx.Graph, | ||
| tag_inserted_swaps: bool = False, | ||
| ) -> None: |
There was a problem hiding this comment.
I think it'll be cleaner if you make the change "Replace a SWAP with a Directional SWAP" in the route_circuit method between Step-4 and Step-5. You can inspect whether a gate has a RoutingTag or not and replace it with a directional swap gate in the routing_ops. You can define a map_func and use cirq.map_operations in order to do this replacement. This will help make the API cleaner and communicate the fact that routing logic doesn't care about direction, and we just use this little trick to decompose the swap gate using directional gates in the end.
AFAIR, the router tries to preserve moment structure so you can also choose to encapsulate the decomposition of directional swap in a cirq.CircuitOperation or define a new DirectionalSwap gate that performs the decomposition and use it here. But I don't have strong opinions here.
Also add a little comment to the class that explains how the directional graphs are handled (i.e. the routing logic is equivalent to that of an undirected graph; you just use a directional swap decomposition to decompose the swap gates into CNOTs and single qubit gates)
tanujkhattar
left a comment
There was a problem hiding this comment.
Final comment. Looks good overall
| inserted_swap = mm.mapped_op( | ||
| ops.SWAP(mm.int_to_logical_qid[swap[0]], mm.int_to_logical_qid[swap[1]]) | ||
| # Emit a standard SWAP gate (will be replaced with directional | ||
| # decomposition in post-processing if needed for directed graphs) | ||
| swap_qubits = ( | ||
| mm.int_to_physical_qid[mm.logical_to_physical[swap[0]]], | ||
| mm.int_to_physical_qid[mm.logical_to_physical[swap[1]]], | ||
| ) | ||
| swap_op = ops.SWAP(*swap_qubits) | ||
| if tag_inserted_swaps: | ||
| inserted_swap = inserted_swap.with_tags(ops.RoutingSwapTag()) | ||
| routed_ops[timestep].append(inserted_swap) | ||
| swap_op = swap_op.with_tags(ops.RoutingSwapTag()) | ||
| routed_ops[timestep].append(swap_op) |
There was a problem hiding this comment.
I believe this can be reverted to it's original form? Do we need this change?
There was a problem hiding this comment.
will revert back - just felt like it was easier on the eyes.
c7abc62 to
3376049
Compare
3376049 to
e133470
Compare
|
@pavoljuhas ! review . |
pavoljuhas
left a comment
There was a problem hiding this comment.
The routing result should not depend on the tag_inserted_swaps value. That argument is a flag for tagging the new operations or not, but otherwise it should have no effect.
Also, please see the inline comments.
| **Handling Directed Graphs:** | ||
| When the device_graph is directed (i.e., edges represent unidirectional CNOT constraints), | ||
| the routing logic still operates as if the graph were undirected. This is because SWAP | ||
| gates are logically symmetric regardless of underlying gate direction constraints. | ||
| After routing completes, any inserted SWAP gates tagged with `RoutingSwapTag` are | ||
| decomposed into a directional-aware sequence using the Hadamard trick: | ||
| ``SWAP = CNOT(ctrl,tgt) - H⊗H - CNOT(ctrl,tgt) - H⊗H - CNOT(ctrl,tgt)`` |
There was a problem hiding this comment.
nit - tweak the section for formatting consistency and remove the possibly confusing detail on tagging:
| **Handling Directed Graphs:** | |
| When the device_graph is directed (i.e., edges represent unidirectional CNOT constraints), | |
| the routing logic still operates as if the graph were undirected. This is because SWAP | |
| gates are logically symmetric regardless of underlying gate direction constraints. | |
| After routing completes, any inserted SWAP gates tagged with `RoutingSwapTag` are | |
| decomposed into a directional-aware sequence using the Hadamard trick: | |
| ``SWAP = CNOT(ctrl,tgt) - H⊗H - CNOT(ctrl,tgt) - H⊗H - CNOT(ctrl,tgt)`` | |
| Handling Directed Graphs: | |
| When the device_graph is directed (e.g., edges represent unidirectional CNOT constraints), | |
| the routing logic still operates as if the graph were undirected. This is because SWAP | |
| gates are logically symmetric regardless of underlying gate direction constraints. | |
| After routing completes, any inserted SWAP gates are decomposed into a directional-aware | |
| sequence using the Hadamard trick: | |
| ``SWAP = CNOT(ctrl, tgt) - H⊗H - CNOT(ctrl, tgt) - H⊗H - CNOT(ctrl, tgt)`` |
|
|
||
| # 5. Return the routed circuit by packing each inner list of ops as densely as possible and | ||
| # preserving outer moment structure. Also return initial map and swap permutation map. | ||
| # 4.5. Replace tagged SWAP gates with directional decompositions if needed. |
There was a problem hiding this comment.
nit - renumber to 5 and below to 6, etc.
| final_circuit = self._replace_swaps_with_directional_decomposition( | ||
| routed_circuit, tag_inserted_swaps | ||
| ) |
There was a problem hiding this comment.
Please add a if nx.is_directed(self.device_graph): guard here so that we do not call possibly expensive _replace_swaps_with_directional_decomposition when it is a no-op.
| if not tag_inserted_swaps: | ||
| # If swaps weren't tagged, we can't identify which ones to decompose. | ||
| # In this case, we assume the graph is undirected or the user will | ||
| # handle decomposition elsewhere. | ||
| return circuit |
There was a problem hiding this comment.
The resulting circuit should not depend on tag_inserted_swaps as that is a markup-only flag for whether the new operations should be tagged or not.
I suggest to construct a set of routing-added SWAP operations in the caller and pass it here. The decomposition should happen for those SWAPs only. The tag_inserted_swaps argument can be removed, because line 357 already copies the tags from the decomposed operation.
| if not any(isinstance(tag, ops.RoutingSwapTag) for tag in op.tags): | ||
| return op |
There was a problem hiding this comment.
Please remove, the decomposition should happen also with tag_inserted_swaps=False when no tags are added.
| assert len(tagged_hadamards) > 0, "Expected tagged Hadamard gates!" | ||
| assert len(tagged_cnots) > 0, "Expected tagged CNOT gates!" |
There was a problem hiding this comment.
As a general style thing - either use assert tagged_hadamards to check if non-empty or compare with the actual expected length. Also please avoid non-essential assertion messages.
| ) | ||
|
|
||
| # Verify the routed circuit is mathematically equivalent to the original | ||
| cirq.testing.assert_circuits_have_same_unitary_given_final_permutation( |
There was a problem hiding this comment.
I'd suggest to move this check to test_directed_device_reverse_only_edge which will give it a bit more check if that test routes a circuit with bidirectional and directed edges.
| result = router._replace_swaps_with_directional_decomposition(circuit, True) | ||
|
|
||
| # The SWAP should be unchanged since there's no edge between q0 and q2 | ||
| assert list(result.all_operations()) == [swap_on_non_edge] |
There was a problem hiding this comment.
This is testing impossible condition. While you can call the internal function with any arguments, in the actual code the _replace_swaps_with_directional_decomposition is passed an already routed circuit which can have only q[0], q[1] from device_graph. Comparing with operation that uses other qubit q[2] is not sensible.
Please either compare with expected routing result or if easier just delete this test.
|
done !, removed some tests and added a new test for 100% coverage lines |
And move `test_repr` to the end so it is not mixed with new tests.
Also replace `router.route_circuit` with `router.__call__` when extra outputs are unused and be a bit more specific in assertions.
Check routing with CNOT-s on bi-directional edge in addition to reverse-oriented CNOT.
pavoljuhas
left a comment
There was a problem hiding this comment.
LGTM with a few tweaks pushed in. I have also shortened
and updated the PR description as it will be used for
commit message so it should be of similar length and style.
Thank you for contributing this!
Enhance the `RouteCQC` transformer to allow directed device graphs.
The routing algorithm is the same as for bidirectional graphs,
except for unidirectional edges q1 -> q2 where inserted SWAP
operations are replaced by their Hadamard decomposition
CNOT(q1, q2)
H(q1), H(q2)
CNOT(q1, q2)
H(q1), H(q2)
CNOT(q1, q2)
Fixes #5863
---------
Co-authored-by: Pavol Juhas <juhas@google.com>
| @@ -34,7 +34,8 @@ def test_directed_device() -> None: | |||
| cirq.testing.assert_same_circuits(routed_circuit, circuit) | |||
|
|
|||
|
|
|||
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Enhance the
RouteCQCtransformer to allow directed device graphs.The routing algorithm is the same as for bidirectional graphs,
except for unidirectional edges q1 -> q2 where inserted SWAP
operations are replaced by their Hadamard decomposition
Fixes #5863