Add nodes_to_linequbits method to LineTopology #5821
Conversation
| @@ -142,6 +142,13 @@ def draw(self, ax=None, tilted: bool = True, **kwargs) -> Dict[Any, Tuple[int, i | |||
| g2 = nx.relabel_nodes(self.graph, {n: (n, 1) for n in self.graph.nodes}) | |||
| return draw_gridlike(g2, ax=ax, tilted=tilted, **kwargs) | |||
|
|
|||
| def nodes_to_linequbits(self, offset=0) -> Dict[Tuple[int, int], 'cirq.LineQubit']: | |||
There was a problem hiding this comment.
LineQubit-s have only one index for their position so the return type should be Dict[int, 'cirq.LineQubit'].
| def nodes_to_linequbits(self, offset=0) -> Dict[Tuple[int, int], 'cirq.LineQubit']: | ||
| """Return a mapping from graph nodes to `cirq.LineQubit` | ||
| Args: | ||
| offset: Offset LineQubits by this amount. |
There was a problem hiding this comment.
Let's clarify a bit - how about Offset integer position of the resultant LineQubits by this amount.
@tanujkhattar - can you confirm we should have the offset argument for the 1D case here as well?
There was a problem hiding this comment.
Yes, I think it makes sense to have the offset argument for 1-D case. Also, @Caffetaria please add type annotations to offset
| Args: | ||
| offset: Offset LineQubits by this amount. | ||
| """ | ||
| return {(r, 1): LineQubit(r) + offset for r in self.graph.nodes} |
There was a problem hiding this comment.
dictionary keys should be integers from 0 to n_nodes - 1.
We can also reduce the number of calls to the LineQubit constructor -
please consider (following #4710 (comment))
return dict(enumerate(LineQubit.range(offset, offset + self.n_nodes)))
There was a problem hiding this comment.
@pavoljuhas The draw method outputs tuples even for the LineTopology.
topology = LineTopology(5)
print(topology.draw(tilted = False))
Outputs:
{(0, 1): (1, 1), (1, 1): (2, 0), (2, 1): (3, -1), (3, 1): (4, -2), (4, 1): (5, -3)}
Given this, should the nodes for LineTopology be represented as integers or tuples?
There was a problem hiding this comment.
The issue #4710 (comment) that this PR is for states that nodes_to_linequbits method is needed for relabeling nodes with
routing_graph = nx.relabel_nodes(topology.graph, topology.nodes_to_linequbits())
That does not work if it returns a dictionary with a pair-tuple keys instead of integers that are in the graph.
The issue does not say anything about drawing the graph or using nodes_to_linequbits for such.
There was a problem hiding this comment.
Ah, got it, thanks! Made the fixes.
| @@ -85,6 +85,9 @@ def test_line_topology(): | |||
| assert LineTopology(2).n_nodes == 2 | |||
| assert LineTopology(2).graph.number_of_nodes() == 2 | |||
|
|
|||
| mapping = topo.nodes_to_linequbits(offset=3) | |||
| assert all(isinstance(q, cirq.LineQubit) and q >= cirq.LineQubit(0) for q in mapping.values()) | |||
There was a problem hiding this comment.
The test does not check keys in the mappings dictionary. It will also pass if values were generated with offset=2 instead of 3; please adjust so it only passes when the offset is exactly 3.
Also it is better to check only one condition per assert statement so this should rather be split to two assertions for (i) the type and (ii) specific values of the LineQubits in the mapping dictionary.
Co-authored-by: Pavol Juhas <pavol.juhas@gmail.com>
Co-authored-by: Pavol Juhas <pavol.juhas@gmail.com>
Thanks! I've added the changes. |
pavoljuhas
added
the
automerge
CirqBot
added
the
front_of_queue_automerge
CirqBot
removed
automerge
Resolves quantumlib#4710 Co-authored-by: Pavol Juhas <juhas@google.com>
Resolves quantumlib#4710 Co-authored-by: Pavol Juhas <juhas@google.com>
Resolves #4710
Co-authored-by: Pavol Juhas juhas@google.com