Named Topologies #4370
Named Topologies #4370
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
mpharrigan
requested review from
cduck,
vtomole,
wcourtney and
a team
as code owners
|
@dstrain115 ptal I will move the files to wherever. It might be easier to read the diff from your last review if it doesn't include a move. Let me know |
CirqBot
added
size: XL
There was a problem hiding this comment.
Looking over this I have a few higher level questions/comments:
- It looks to me like the gist of this code functionality is to enable a user to have some device/connectivity graph and identify all the different ways they could generate a line/slantedrectangle subgraph on it.
get_placementsworks on two networkx graphs, mentions that the intended application is device placement but makes no use of thecirq.Deviceclass, would it be possible to also have:get_placements(my_device: cirq.Device, my_desired_subgraph: nx.Graph)? Building on this, devices has this:
def qid_pairs(self) -> Optional[FrozenSet['cirq.SymmetricalQidPair']]:
"""Returns a set of qubit edges on the device, if possible."""Why can't we just add a method to the device class that returns a generator for these desired subgraph types (with the added benefit of directing our users towards devices instead of having two very similar things that both mention topologies + placements etc. and seperating the functionality when we don't need to) ?
def all_subgraphs(self, my_desired_subgraph):
"""Returns a generator to all possible placements with my_desired_subgraph topology."""Looking at the tutorial for the intended usage, a SYC23_GRAPH graph is created, but we already have that implicitly defined as a cirq.Device. To me this functionality seems far more intuitive to add to the Device class and we don't loss any generality/functionality in supporting our own hardware here by doing this. Does anyone else see any downsides to this ?
- Given the size and amount of contention on this PR, I think it might be better if we first got the library implementation agreed on and then moved to do a notebook/tutorial in a second PR since this tutorial does look pretty short and is a little lacking in terms of structure when you compare it to other notebooks we offer like: https://quantumai.google/cirq/tutorials/variational_algorithm or https://www.tensorflow.org/quantum/tutorials/gradients
That's one part, yes. The first is to assign names to common graphs we use in applications.
Pending #3245, we should be able to more easily go from device to graph. This functionality works for any two graphs, irrespective of whether they correspond to a
I think this hinges on the "this is tightly coupled to Devices" premise. All things being equal, I prefer free functions to methods. This makes code re-use easier.
Yes, this is a long-standing feature request. The implicit definition based on GridQubits is very dangerous is you have two qubits that "seem" adjacent by their indices but aren't actually coupled (either by design or because of a fault). The device roadmap item is out of scope for this PR.
I'm definitely in the camp of "something is better than nothing". I think these concepts are inherently visual and you can get a lot by just plotting a bunch of example graphs. |
This is annoying to do with all the __init__.pys and the json stuff and you can't import cirq from inside cirq.
|
Moved to cirq-core ptal |
|
How do we deal with the notebook failure? The new notebook won't work against Cirq stable because the functionality isn't in cirq stable |
There was a problem hiding this comment.
Things are looking good! Just a few remaining questions on the TiltedSquareLattice and the get_placements function then we should be good to merge.
I'm still not a huge fan of keeping the tutorial in this PR since it does seem pretty slim compared to our other tutorials and will need to be constantly iterated on as more components of testbed, devices etc. land in the repo in order to bring it up to standard with the other tutorials we have. This seems like it'd be a lot more work than just doing a completely seperate PR with only this tutorial in it so we could have a more in depth review on it. Do you have strong feelings here ? I would prefer to heir on the side of caution with this and stick to go/smaller-cls .
| g = nx.Graph() | ||
|
|
||
| def _add_edge(unit_row: int, unit_col: int, *, which: int): | ||
| """Helper function to add edges in 'unit cell coordinates'.""" | ||
| y = unit_col + unit_row | ||
| x = unit_col - unit_row | ||
|
|
||
| if which == 0: | ||
| # Either in the bulk or on a ragged boundary, we need this edge | ||
| g.add_edge((x, y), (x, y - 1)) | ||
| elif which == 1: | ||
| # This is added in the bulk and for a "top" (extra height) ragged boundary | ||
| g.add_edge((x, y), (x + 1, y)) | ||
| elif which == 2: | ||
| # This is added in the bulk and for a "side" (extra width) ragged boundary | ||
| g.add_edge((x, y), (x - 1, y)) | ||
| elif which == 3: | ||
| # This is only added in the bulk. | ||
| g.add_edge((x, y), (x, y + 1)) | ||
| else: | ||
| raise ValueError() # coverage: ignore | ||
|
|
||
| # Iterate over unit cells, which are in units of 2*width, 2*height. | ||
| # Add all all four edges when we're in the bulk. | ||
| unit_cell_height = self.height // 2 | ||
| unit_cell_width = self.width // 2 | ||
| for unit_row in range(unit_cell_height): | ||
| for unit_col in range(unit_cell_width): | ||
| for i in range(4): | ||
| _add_edge(unit_row, unit_col, which=i) | ||
|
|
||
| extra_h = self.height % 2 | ||
| if extra_h: | ||
| # Add extra height to the final half-row. | ||
| for unit_col in range(unit_cell_width): | ||
| _add_edge(unit_cell_height, unit_col, which=0) | ||
| _add_edge(unit_cell_height, unit_col, which=1) | ||
|
|
||
| extra_w = self.width % 2 | ||
| if extra_w: | ||
| # Add extra width to the final half-column | ||
| for unit_row in range(unit_cell_height): | ||
| _add_edge(unit_row, unit_cell_width, which=0) | ||
| _add_edge(unit_row, unit_cell_width, which=2) | ||
|
|
||
| if extra_w and extra_h: | ||
| # Add the final corner node when we have both ragged boundaries | ||
| _add_edge(unit_cell_height, unit_cell_width, which=0) |
There was a problem hiding this comment.
I think we might be able to tighten this up and just do something like:
rect1 = set((i + j, i - j) for i in range(w//2 + 1) for j in range(h//2 + 1))
rect2 = set(((i + j) // 2, (i - j) // 2) for i in range(1, w + 1, 2) for j in range(1, h + 1, 2))
all_nodes = rect1 | rect2
g = nx.Graph()
for u in all_nodes:
for dx, dy in [(1,0), (-1, 0), (0,1), (0,-1)]:
v = (u[0] + dx, u[1] + dy)
if v in all_nodes:
g.add_edge(u, v)here w=self.width and h=self.height.
There was a problem hiding this comment.
| n_nodes = (self.width // 2 + 1) * (self.height // 2 + 1) | ||
| n_nodes += ((self.width + 1) // 2) * ((self.height + 1) // 2) | ||
| object.__setattr__(self, 'n_nodes', n_nodes) |
There was a problem hiding this comment.
Can we just use len(g.nodes) or g.number_of_nodes here ?
There was a problem hiding this comment.
We could. I wanted a formula for the number of nodes in terms of width and height; the unit test verifies that the two methods agree. The potential benefit of a contributor being able to read off a formula by looking at the code offsets the downside of not using g.number_of_nodes()
| """Get 'placements' mapping small_graph nodes onto those of `big_graph`. | ||
|
|
||
| We often consider the case where `big_graph` is a nx.Graph representation of a Device | ||
| whose nodes are `cirq.Qid`s like `GridQubit`s and `small_graph` is a NamedTopology graph. | ||
| In this case, this function returns a list of placement dictionaries. Each dictionary | ||
| maps the nodes in `small_graph` to nodes in `big_graph` with a monomorphic relationship. | ||
| That's to say: if an edge exists in `small_graph` between two nodes, it will exist in | ||
| `big_graph` between the mapped nodes. | ||
|
|
||
| We restrict only to unique set of `big_graph` qubits. Some monomorphisms may be basically | ||
| the same mapping just rotated/flipped which we purposefully exclude. This could | ||
| exclude meaningful differences like using the same qubits but having the edges assigned | ||
| differently, but it prevents the number of placements from blowing up. | ||
| """ |
There was a problem hiding this comment.
Can we tighthen up this docstring a bit ? it's looking a little on the longer side and doesn't list the args: or returns:. It won't render too well on the devsite without it. Also might want to mention that there is some ordering imposed on the returned subgraphs thanks to the sorted lower down in the function.
There was a problem hiding this comment.
Refactored into Args, Returns. Tightened up some of the wording. Kept most of the content. I think it's all pretty important.
This is in a class of functions that doesn't correspond to the super pure general everything function: instead, it encodes general ideas I usually need when doing the particular form of graph matching for putting a circuit on a device. It might not be for everyone. The pure, general version would just be networkx's sugraph_monomorphism_iter(). The intricacies in this version need to be documented and motivated
| object.__setattr__(self, 'graph', graph) | ||
|
|
||
| def draw(self, ax=None, tilted: bool = True, **kwargs) -> Dict[Any, Tuple[int, int]]: | ||
| """Draw this graph. |
There was a problem hiding this comment.
I would add a small bit of detail here, such as Draw this graph using matplotlib. so that the docstring is not redundant with the function name.
There was a problem hiding this comment.
added matplotlib notes here and everywhere
| object.__setattr__(self, 'n_nodes', n_nodes) | ||
|
|
||
| def draw(self, ax=None, tilted=True, **kwargs): | ||
| """Draw this graph |
There was a problem hiding this comment.
nit: same comment here, plus missing period.
With the move to cirq-core, I've moved this out of the "tutorials" directory. As you point out, it's not really a tutorial. It's documentation. [I guess originally when I put it in cirq-google, I put it with the other notebooks which are all tutorials, but cirq regular-doc-pages is a mix of markdown and notebooks so we gucci]. I'm going to strongly advocate for including visual diagrams with this PR. A picture is worth a thousand lines of docstring.
Agile / small CLs / small PRs / whatever you want to call it are really a call for exactly this: MVP plus iteration trumps waterfall. |
|
@MichaelBroughton pushed changes for latest round of review; PTAL |
mpharrigan
added
the
automerge
CirqBot
added
the
front_of_queue_automerge
CirqBot
removed
automerge
"Named topologies" provide a mapping from a simple dataclass to a unique graph. This PR also includes basic utilities for matching problem topologies to devices    
"Named topologies" provide a mapping from a simple dataclass to a unique graph. This PR also includes basic utilities for matching problem topologies to devices    
"Named topologies" provide a mapping from a simple dataclass to a unique graph. This PR also includes basic utilities for matching problem topologies to devices