Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[runtime] QubitPlacer 2 - RandomDevicePlacer #4719

Merged
merged 9 commits into from
Jan 19, 2022

Conversation

mpharrigan
Copy link
Collaborator

@mpharrigan mpharrigan commented Dec 2, 2021

As part of the cirqflow runtime, add RandomDevicePlacer which uses cirq.get_placements to map from a NamedTopology graph to a device graph.

This requires shimming over the ability to get a device graph from a cirq.Device.

@tanujkhattar @MichaelBroughton feel free to browse while I add tests

@google-cla google-cla bot added the cla: yes Makes googlebot stop complaining. label Dec 2, 2021
@CirqBot CirqBot added the size: L 250< lines changed <1000 label Dec 2, 2021
@mpharrigan mpharrigan force-pushed the 2021-11-qubit-placement-b branch 2 times, most recently from aad1a94 to ff69dd9 Compare December 2, 2021 00:54
@mpharrigan mpharrigan force-pushed the 2021-11-qubit-placement-b branch from f8cfb20 to c463b2d Compare January 6, 2022 01:16
@mpharrigan mpharrigan marked this pull request as ready for review January 6, 2022 01:16
@mpharrigan mpharrigan requested review from cduck, vtomole, wcourtney and a team as code owners January 6, 2022 01:16
@mpharrigan
Copy link
Collaborator Author

PTAL @MichaelBroughton @tanujkhattar

Copy link
Collaborator

@tanujkhattar tanujkhattar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My main comment is to get rid of default_topo_node_to_qubit and instead add a nodes_to_qubits abstract method on the NamedTopology class.

return abs(qubit1.row - qubit2.row) + abs(qubit1.col - qubit2.col)

return nx.Graph(
pair for pair in itertools.combinations(qubits, 2) if _manhattan_distance(*pair) == 1
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pair for pair in itertools.combinations(qubits, 2) if _manhattan_distance(*pair) == 1
pair for pair in itertools.combinations(qubits, 2) if pair[0].is_adjacent(pair[1])

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is copied from cirq contrib and is indeed a ridiculous implementation, but I'll make this change here

Comment on lines 20 to 21
from typing import Dict, Any, Tuple, TYPE_CHECKING
from typing import List, Callable
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merge into a single from typing import ... line.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pycharm hast forsaken me

cirq-google/cirq_google/workflow/qubit_placement.py Outdated Show resolved Hide resolved
Comment on lines +86 to +108
def default_topo_node_to_qubit(node: Any) -> cirq.Qid:
"""The default mapping from `cirq.NamedTopology` nodes and `cirq.Qid`.

There is a correspondence between nodes and the "abstract" Qids
used to construct un-placed circuit. `cirq.get_placements` returns a dictionary
mapping from node to Qid. We use this function to transform it into a mapping
from "abstract" Qid to device Qid. This function encodes the default behavior used by
`RandomDevicePlacer`.

If nodes are tuples of integers, map to `cirq.GridQubit`. Otherwise, try
to map to `cirq.LineQubit` and rely on its validation.

Args:
node: A node from a `cirq.NamedTopology` graph.

Returns:
A `cirq.Qid` appropriate for the node type.
"""

try:
return cirq.GridQubit(*node)
except TypeError:
return cirq.LineQubit(node)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd propose to add a method nodes_to_qubits in the NamedTopology class and use it here instead of this hack, especially because the lambda argument in the qubit placer class below is also not serializable.

Note that we already have nodes_to_gridqubits method on TiltedSquareLattice and have an open issue to add a similar nodes_to_linequbits method for the LineTopology.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm I don't consider this a hack.

NamedTopology.nodes_to_qubits may seem obvious for LineTopology and TiltedSquareLattice but (a) maybe that isn't so and (b) it would be non-obvious for other topology types.

(a) what if you happened to use a different qubit type, especially for LineTopology. You could have written your circuit using GridQubit(x, 0) or even GridQubit(x, y) for fixed y where you know that that defines an actual list on the device. What if you used NamedQubit's a, b, c, ...,

(b) Imagine additional named topologies that don't have a corresponding qubit type. Consider a hexagonal topology. what would be the nodes_to_qubits function? there are multiple ways to handle this https://www.redblobgames.com/grids/hexagons/ including mapping to gridqubits or defining a new coordinate system.

nodes_to_gridqubits and nodes_to_linequbits as non-abstract, non-interface, topology-specific methods makes sense. you can even imagine having LineTopology.nodes_to_gridqubits. But I think it would be too tricky to have as part of the NamedTopology interface

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I imagine there would be more places where we'll end up needing the topo_node_to_qubit_func parameter, i.e. all qubit placement / routing related utilities. It would be nice if we can avoid passing this additional parameter around by putting the mapping in the named topology itself.
  2. Do we imagine a use case of NamedTopology's where the topology itself should exist but the mapping routines need not exist because they are hard to write? IIUC, whenever someone creates a new NamedToplogy (eg: the hexagonal topology), they'd also have to figure out a way to map the topology to qubits for it to be useful. If that's the case, then it's just a matter of whether we should do it outside the class or inside.
  3. Assuming 2 above is true, we can deal with the problem of different mappings for a specific named topology by either deriving from the topology class and overriding the nodes_to_qubits method, or by parameterising the topology class with flags to control the placement behavior.
  4. Moving the placement logic to named topology has an added benefit that it makes it serializable. I'm curious to know your perspective on why that is not so important?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The true solution would be to let you define Circuits with arbitrary qubit identifiers, and then you could use integers or tuples of integers or whatever is appropriate for your problem. The current construct is a shim around that. It's like if in classical programming you couldn't use variable names and had to use hardcoded memory addresses. Most of what follows expands on this idea.

  1. any qubit mapping worth its salt shouldn't care about the form of the input "qubit" (logical qubit, node, whatever). The shim is that the input qubits must be cirq.Qid even if that doesn't make complete sense.
  2. No; it's not straightforward like that. There are qubit mapping strategies that are completely agnostic to the input and output topologies; some that make make some assumptions (e.g. maybe specialize for a linear or planar target topo); and some basic strategies that make very strict assumptions (like the Naive + offset placer). That's why the mapping should happen in the mapping strategy.
  3. Philosophically, this would be a step in the wrong direction. I want to be able to describe ~~problem~~ topologies independent of their implementation.
  4. The placement logic is serializable. If I tell you that I used NaiveQubitPlacer to run my problem, you'd know what strategy I used. If I gave you a json document with RandomQubitPlacer and the target topology, you'd know what strategy I used.

There's just this annoying thing you need to "integrate out" where I'd like to describe my problem in terms of namedtopology nodes but have to use gridqubit or linequbit

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some semi-relevant hearsay #4713

@tanujkhattar tanujkhattar self-assigned this Jan 13, 2022
@mpharrigan
Copy link
Collaborator Author

@tanujkhattar ptal

also cc @MichaelBroughton if you want to review this

@mpharrigan
Copy link
Collaborator Author

bump

@tanujkhattar tanujkhattar added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Jan 19, 2022
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Jan 19, 2022
@CirqBot CirqBot merged commit 7ffc77c into quantumlib:master Jan 19, 2022
@CirqBot CirqBot removed automerge Tells CirqBot to sync and merge this PR. (If it's running.) front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. labels Jan 19, 2022
MichaelBroughton pushed a commit to MichaelBroughton/Cirq that referenced this pull request Jan 22, 2022
As part of the cirqflow runtime, add `RandomDevicePlacer` which uses `cirq.get_placements` to map from a `NamedTopology` graph to a device graph.

This requires shimming over the ability to get a device graph from a `cirq.Device`.

@tanujkhattar @MichaelBroughton feel free to browse while I add tests
rht pushed a commit to rht/Cirq that referenced this pull request May 1, 2023
As part of the cirqflow runtime, add `RandomDevicePlacer` which uses `cirq.get_placements` to map from a `NamedTopology` graph to a device graph.

This requires shimming over the ability to get a device graph from a `cirq.Device`.

@tanujkhattar @MichaelBroughton feel free to browse while I add tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/workflow cla: yes Makes googlebot stop complaining. size: L 250< lines changed <1000
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants