Improve initial qubit mapping algorithm #144
Conversation
recirq/quantum_chess/dynamic_look_ahead_heuristic_circuit_transformer_test.py
Outdated
Show resolved
Hide resolved
|
|
||
| Args: | ||
| g: A physical qubits graph or a logical qubits graph. | ||
| """ |
There was a problem hiding this comment.
should we move the floyd-warshall implementation into a separate file? That may be better organizationally. And since it is a fairly generic algorithm, it seems like something that we could add test coverage for independently of the rest of the initial mapping implementation
There was a problem hiding this comment.
i agree the floyd-warshall algorithm and the various bfs algorithms are fairly generic, but their implementation all have to be aware of whether it's graph param is a logical qubit graph (where edges contain moment info) or physical qubit graph or either, so i think it might make more sense to keep them in the same class.
but yah, it's def worth adding test coverage independently for these more generic algorithms; maybe we can keep the test files separate? kind of like what we have right now in dynamic_look_ahead_heuristic_circuit_transformer_test.py, but with more / better test cases?
There was a problem hiding this comment.
Agreed, lets add smaller focused files if we can. Besides the tests I'm also a bit concerned about the potential for circuit_transformer.py to explode in size if we add more transformers, but should be fine for now.
There was a problem hiding this comment.
by the way, I realized another reason it would be nice to refactor out the floyd-warshall implementation: the swap update step needs the precomputed node-to-node shortest path distances, too. We can leave that for a future pull request though.
There was a problem hiding this comment.
sg, i refactored it out to a separate function!
recirq/quantum_chess/dynamic_look_ahead_heuristic_circuit_transformer_test.py
Outdated
Show resolved
Hide resolved
recirq/quantum_chess/dynamic_look_ahead_heuristic_circuit_transformer_test.py
Outdated
Show resolved
Hide resolved
|
|
||
| Args: | ||
| g: A physical qubits graph or a logical qubits graph. | ||
| """ |
There was a problem hiding this comment.
by the way, I realized another reason it would be nice to refactor out the floyd-warshall implementation: the swap update step needs the precomputed node-to-node shortest path distances, too. We can leave that for a future pull request though.
| """ | ||
| qubits = [] | ||
| neighbors_sorted_by_moment = sorted(lg[lq], key=lambda x: x[1]) | ||
| for neighbor in neighbors_sorted_by_moment: |
There was a problem hiding this comment.
nit, this can be simplified:
for neighbor in neighbors_sorted_by_moment.keys():
There was a problem hiding this comment.
neighbors_sorted_by_moment is a list but updated to make this a bit cleaner.
recirq/quantum_chess/dynamic_look_ahead_heuristic_circuit_transformer_test.py
Outdated
Show resolved
Hide resolved
| class DynamicLookAheadHeuristicCircuitTransformer(CircuitTransformer): | ||
| """Optimizer that transforms a circuit to satify a device's constraints. | ||
|
|
||
| This implements the initial mapping algorithm and the SWAP update algorithm |
There was a problem hiding this comment.
Referencing the paper is great, but it would be great to have a one paragraph summary for those who are too busy to read the whole paper.
| q = op.qubits[0] | ||
| if q not in g: | ||
| g[q] = [] | ||
| if len(op.qubits) == 2: |
There was a problem hiding this comment.
Should we change this to elif len(op.qubits) == 2 and then add an else that handles len(op.qubits) > 2 (possibly by throwing a ValueError? It might be a good idea to make sure we skip measurement gates properly too.
There was a problem hiding this comment.
sg, updated to throw a ValueError when len(ops.qubits) > 2; also added a check to skip measurement gates.
recirq/quantum_chess/dynamic_look_ahead_heuristic_circuit_transformer_test.py
Outdated
Show resolved
Hide resolved
| t = ct.ConnectivityHeuristicCircuitTransformer(device) | ||
| device.validate_circuit(t.transform(c)) | ||
| # t = ct.DynamicLookAheadHeuristicCircuitTransformer(device) | ||
| # device.validate_circuit(t.transform(c)) |
There was a problem hiding this comment.
Should we add tests for some of the public functions? For instance, adding functionality to test least_connected_qubit should be simple enough.
There was a problem hiding this comment.
yah, tests are added in initial_mapping_utils_test.py (which was previously dynamic_look_ahead_heuristic_circuit_transformer_test.py).
| a4 = cirq.NamedQubit('a4') | ||
| a5 = cirq.NamedQubit('a5') | ||
| a6 = cirq.NamedQubit('a6') | ||
| a7 = cirq.NamedQubit('a7') |
There was a problem hiding this comment.
Optional: I think the style guide would have these e capitalized (i.e. A7), but this goes against chess algebraic style, so I am not sure which is better.
There was a problem hiding this comment.
i'm leaning towards using lower case for consistency since that's currently used in https://github.com/quantumlib/ReCirq/blob/master/recirq/quantum_chess/constants.py and https://github.com/quantumlib/ReCirq/blob/master/recirq/quantum_chess/circuit_transformer_test.py.
There was a problem hiding this comment.
I'd agree on lower, for consistency with chess.
recirq/quantum_chess/dynamic_look_ahead_heuristic_circuit_transformer_test.py
Outdated
Show resolved
Hide resolved
|
|
||
|
|
||
| @pytest.mark.parametrize('transformer', [ct.ConnectivityHeuristicCircuitTransformer]) |
There was a problem hiding this comment.
Why are we parameterizing if it only has one option? Same with several functions below.
There was a problem hiding this comment.
added DynamicLookAheadHeuristicCircuitTransformer to the list now that it's updated to use SwapUpdater.
| 4: [3], | ||
| } | ||
| assert imu.get_least_connected_qubit(g, deque([0, 1, 2])) in {0, 2} | ||
| assert imu.get_least_connected_qubit(g, deque([3, 4])) in {3, 4} |
There was a problem hiding this comment.
Do we want to have a test case that is less ambiguous?
| Args: | ||
| g: A physical qubits graph or a logical qubits graph. | ||
| v: The size of the graph. | ||
| m: A mapping of qubit to index. |
There was a problem hiding this comment.
would the interface be simpler if we keyed things by Qid directly and returned a dict from Pair[Qid, Qid] -> int instead of requiring an intermediate Qid -> index mapping? Then the only args we'd need to provide would be the graph dict
There was a problem hiding this comment.
updated and it's much cleaner now!
There was a problem hiding this comment.
just need to update the return type hint to something like Dict[Tuple[Qid, Qid], int] (who/what is using these type hints anyways...?)
There was a problem hiding this comment.
ty for catching that! i'm guessing they're for eventual docs generation...?
Fixes #127 and relates to quantumlib/unitary#48.