[runtime] QubitPlacer part 1 #4700
Conversation
mpharrigan
force-pushed
the
2021-11-qubit-placement-a
branch
from
f3b7a82 to
f95de7f
Compare
|
factored out named topology stuff in #4701 |
mpharrigan
requested review from
cduck,
vtomole,
wcourtney and
a team
as code owners
|
ptal @MichaelBroughton @tanujkhattar or anyone interested |
| def place_circuit( | ||
| self, | ||
| circuit: cirq.AbstractCircuit, | ||
| problem_topo: NamedTopology, | ||
| shared_rt_info: 'cg.SharedRuntimeInfo', | ||
| rs: np.random.RandomState, | ||
| ) -> Tuple[cirq.FrozenCircuit, Dict[Any, cirq.Qid]]: | ||
| return circuit.freeze(), {q: q for q in circuit.all_qubits()} |
There was a problem hiding this comment.
Question: Why aren't there any strict checks that the resulting circuit is valid under the given topology ? Do we want to allow users to have a Placer totally ignore the Topology ?
There was a problem hiding this comment.
yes; this placer totally ignores the topology. Validation can be done by the device as part of the execution loop
| logger = _PrintLogger(n_total=len(executable_group)) | ||
| logger.initialize() | ||
|
|
||
| rs = np.random.RandomState(rt_config.random_seed) | ||
| exe: QuantumExecutable |
There was a problem hiding this comment.
Does this declaration mess with the below loop ?
There was a problem hiding this comment.
this is how you do type annotations for loop variables
There was a problem hiding this comment.
Do we need this annotation? Wouldn't the type of exe get inferred automatically based on the type of executable_group which we iterating on?
There was a problem hiding this comment.
pycharm wasn't picking it up
| logger = _PrintLogger(n_total=len(executable_group)) | ||
| logger.initialize() | ||
|
|
||
| rs = np.random.RandomState(rt_config.random_seed) |
There was a problem hiding this comment.
Question: Should we give the seed to the rt_config or the randomstate generator itself ? It seems like down the line if we kept creating randomStates off of this seed we'd get a lot of duplicate random numbers.
There was a problem hiding this comment.
two part answer
rt_configneeds to be serializable and put in a database so we can groupby and agg the results. Seed is (much!) easier than the internal state of a random state- We don't create >1 randomstate based on this seed. We create one at the top of the function (here!) and it gets passed to any subroutines that require randomness.
|
|
||
|
|
||
| @dataclasses.dataclass(frozen=True) | ||
| class NaiveQubitPlacer(QubitPlacer): |
There was a problem hiding this comment.
Rename the class to a more descriptive name.
There was a problem hiding this comment.
do you have a suggestion
There was a problem hiding this comment.
Maybe NoOpQubitPlacer?
There was a problem hiding this comment.
it's not really a "no op", it's more like an identity
|
@MichaelBroughton ptal |
| from cirq import _compat | ||
| from cirq.devices import GridQubit, LineQubit |
There was a problem hiding this comment.
Remove Duplicate imports? See lines 23-24 below
There was a problem hiding this comment.
see #4701 but I don't think there's any post merge duplication
mpharrigan
added
the
automerge
CirqBot
added
the
front_of_queue_automerge
CirqBot
removed
automerge
Splitting up quantumlib#4678 - The interface: `QubitPlacer` - The fallback: `NaiveQubitPlacer` - runtime hooks some hacks around quantumlib#4699
Splitting up #4678
QubitPlacerNaiveQubitPlacersome hacks around #4699