One or more of the the following people are requested to review this:

• @Qiskit/terra-core

I don't think this is a change to be made this lightly; GateDirection does only make sense on physically mapped circuits, and the single q register is (for better or worse) how Qiskit represents that. This PR represents more than a "fix" to GateDirection, it represents a removal of what constitutes a "physical circuit" from Qiskit. If anything the pass is incorrect because it doesn't enforce that there's exactly one qreg that's called q.

(Fwiw, I don't like that method of defining physical qubits, it's just what we've got at the moment.)

I'm not actually convinced that the use of single register is a contract on the physical qubit allocation. We do that in ApplyLayout, but any physical circuit relies solely on dag.find_bit(x).index (or an equivalent) to find the physical qubit. It's the index in qubits list not the single physical register. we should use for the mapping. That being said since ApplyLayout works this way I'm sure there is an implicit assumption in other places, but removing the check here doesn't hurt anything because I contend it really shouldn't be a hard requirement.

Also, even if it's our API contract we don't need to eagerly raise an exception like this, it's not the job of every pass to do input validation.

but any physical circuit relies solely on dag.find_bit(x).index (or an equivalent) to find the physical qubit. It's the index in qubits list not the single physical register

This isn't fully true in any of the routing passes; they typically do

canonical_register = dag.qregs["q"]
initial_layout = Layout.generate_trivial_layout(canonical_register)
# and sometimes
layout_mapping = {bit: index for index, bit in enumerate(canonical_register)}

Sometimes they also look at dag.qubits or use dag.find_bit in some cases, but they all begin by requiring a single q qreg, no loose qubits and all the qubits in order, so the assumption of what constitutes a physical circuit is still there. That's not directly used here, but in general dag.find_bit(bit).index cannot be relied on to refer to a physical qubit index; you need to be sure that you've got a physical-mapped circuit before it's safe to make that assumption. A single q-named qreg being the "proof" of that is at the least implied to be part of the API surface because of the error messages in the checking cases (e.g.)

I agree that it's not necessarily the contract of GateDirection to raise an exception, but the problem that the call in the linked issue here is not a physical circuit as Qiskit "defines" it isn't gone, and so this PR would be us sanctioning a currently incorrect use of the API.

In a more strongly typed language, this wouldn't be allowed; the caller would need to produce some sort of PhysicalCircuit or PhysicalQubit instances in order to use GateDirection at compile-time, so it's clear in the call contract that the onus is on the user to assert that they've got physical qubits. I'm not a fan of this PR because it's implicitly saying that any circuit can now be automatically considered physical without the caller typing it as such themselves to be clear at what point they began to make that assumption. We've had enough misunderstandings and in some cases bugs between "virtual" and "physical" qubits across the library that I'm not keen to remove what little library-side enforcement of it we have without good reason or a replacement.

More to the point: is there any reason why it would be especially onerous for the calling in the linked issue just to use the "correct" form? All that's needed is to manually make the circuit physical by changing

q1 = QuantumRegister(1)
q2 = QuantumRegister(1)
qc = QuantumCircuit(q1, q2)
qc.cx(1, 0)
qc.cx(0, 1)

into

q1 = QuantumRegister(1)
q2 = QuantumRegister(1)
qc = QuantumCircuit(QuantumRegister(name="q", bits=q1[:] + q2[:]))
qc.cx(1, 0)
qc.cx(0, 1)

(if the q1 and q2 even need to be registers in the first place)

TBH, I don't think the enforcement is enforcing anything of value, and this PR is of such little impact it's wasted effort to continue arguing over it. The thing to remember is that there is nothing stopping someone from running gate direction standalone. You could easily do:

q1 = QuantumRegister(1)
q2 = QuantumRegister(1)
qc = QuantumCircuit(q1, q2)
qc.cx(1, 0)
qc.cx(0, 1)
direction_qc = GateDirection(CouplingMap.from_line(2, bidirectional=False))(qc)

and that is a completely valid use case from my PoV. The pass shouldn't be enforcing that it is running as part of full pipeline especially as it has a small well defined job (which maybe could be documented better). I agree the model around physical vs virtual circuits is far from ideal, and things can get mixed frequently. But that doesn't change because of this PR, nor does this PR make the situation any appreciably different, except from a philosophical standpoint. I think short of defining physical qubits and physical circuits explicitly and then making a hard contract to make the distinction clear in which domain a pass (or anything) is working in this ambiguity is something we have to live with (and have been). All this really does is increase the general utility of the pass because it makes it easier to use in custom workflows (not just the above example), without introducing any potential regression.

The only thing I would potentially ask for here to address your concerns is we can document the implicit assumption that qubit indices are the physical qubit index in either the target or coupling map. This is the implicit assumption the pass is actually working off of, so making it explicit in the documentation seems like a good step.

I don't agree that a check that the input is physical is worthless, nor that this PR is innocent in slackening what little definition of physical circuits we have, but I won't prevent it from merging (why I didn't put a "request changes" review in the first place).

I totally agree that somebody can call or use GateDirection outside our pipelines, and that's a large part of why I think it's important that they should have to prove that they've got a circuit that's defined on physical qubits before they're allowed to call it.

(and yeah, I'd appreciate it if it is going to merge that we do at least document the required contract that the bit indices represent the physical qubits in the coupling map)

I guess I just don't agree that our meaning of physical in the transpiler is a flat register "q" for all qubits in the target. I've always viewed that as more an implementation detail of just ApplyLayout and nothing more. I'm not surprised we have things actively checking against it (I knew sabre swap, and stochastic swap used it) but for every pass I've written that's operating on physical qubits the register is always ignored. I think we should open up a separate issue to clarify this, either via documentation or actually typing it like we've talked about forever.

Sure, that works well for me - I would for sure be keen to formalise the concept of physical qubits.