Make NLayout's qubit types wrap Qubit
#13734
Conversation
`NLayout` predates the circuit qubit indices being in Rust. Now, though, `NLayout::VirtualQubit` and `NLayout::PhysicalQubit` are promises made _on top_ of circuit qubits, so it makes sense for them to wrap the other type. This simplifies a reasonable amount of transpiler-pass code, since the conversations are easier. More importantly, this lays the groundwork for the `Qubit` type to restrict its size to be slightly less than 32 bits.
jakelishman
added
Changelog: None
|
One or more of the following people are relevant to this code:
|
Pull Request Test Coverage Report for Build 12954556034Details
💛 - Coveralls |
There was a problem hiding this comment.
I think this makes sense to do.
this lays the groundwork for the Qubit type to restrict its size to be slightly less than 32 bits.
Can you provide a little more context for this, also? I left a few comments which I think might be related to your direction here.
| impl From<Qubit> for $id { | ||
| fn from(val: Qubit) -> Self { | ||
| Self(val) | ||
| } | ||
| } |
There was a problem hiding this comment.
Should we have this conversion? I'm wondering if callsites might be more self-documenting if users are forced to use PhysicalQubit::new, for example.
There was a problem hiding this comment.
I know I used to think that a bit more, but in usage, I've changed a bit. I think that wrapper marker types like this are most useful in the typing of the functions and structs that actually act on them, rather than at the construction sites of them. You can still make logic errors in the value of the Qubit you're putting in there, but that's a logic error only at a scale larger than a single constructor call - it's not a logic error without all the surrounding context. So I'm less worried about making the constructor sites very verbose to use, because the useful part of the typing is when something else consumes the PhysicalQubit, and the construction verbosity just makes them more annoying to type and read, rather than actually safer.
The difference is with something like OperationIndex, over in the other PR: that doesn't have a public constructor at all, because the ownership of one is proof of the provenance that you got it from a DAG that verified that it does in fact refer to an Operation node. We can't make that safe against using it in the wrong DAG, but without global context there's only so much we can achieve with static typing. Still, the idea is that the only public constructors of it (DAGCircuit methods) all have sufficient context to say that it was valid at the time of creation. We can't realistically do that with Qubit in the same way, and the constructors we have already do it as far as we can, I think.
|
|
||
| impl $id { | ||
| #[inline] | ||
| pub fn new(val: u32) -> Self { | ||
| Self(val) | ||
| pub fn new(val: usize) -> Self { |
There was a problem hiding this comment.
If we want to encapsulate the storage of bit types in the future, we might want to change this to val: Qubit now.
There was a problem hiding this comment.
There's no plan to have new take a Qubit - there's no need. Since Qubit now manages the validity of the object, it's totally safe to have the tuple constructor just be public, since you can't make an invalid PhysicalQubit by using it. new then matches Qubit::new, which is usize in the same way that NodeIndex does that - a technically fallible conversion, but the panicking constructor for it.
| @@ -171,18 +186,21 @@ impl NLayout { | |||
| } | |||
|
|
|||
| #[staticmethod] | |||
| pub fn generate_trivial_layout(num_qubits: u32) -> Self { | |||
| pub fn generate_trivial_layout(num_qubits: usize) -> Self { | |||
There was a problem hiding this comment.
This is fine for now, but it feels like we're going the wrong way with changing stuff to usize if we ultimately want a u32 for bits.
Our mistake seems to have been making {Qu,Cl}bit::new take a usize rather than a u32. It led me to create #13377 (which attempts to get us to consistently use usize when working with bit indices and u32 only when creating bits / at rest). We probably ought to make bit indices u32 everywhere and rely on the ::index method to get a usize, e.g. when indexing into collections. It's just a bit uglier since then we also end up with things like (0u32..).zip(qubits) instead of qubits.iter().enumerate().
There was a problem hiding this comment.
The trouble is that u32 will also be incorrect, and usize is the more convenient size integer. u32 isn't making us any safer - we're still as casting from usize to it all the time, and as with NodeIndex, there's already a lot of places that bake in a runtime assumption, not a compile-time one, that the integers fit. Even a safely constructed Qubit isn't making us statically safe from panics, because there's runtime bounds checks on the number of Qubits in the circuit context.
All this is to say: I don't think the new method matters too much for the internal type any more - that's just the convenience entry point, and overall it seems to be the most convenient to have the dual of the index method. I made TryFrom impls for the actually fallible conversions in my follow-on PR, to avoid the panicking cases for places where that's more important.
There was a problem hiding this comment.
The inner object of Qubit does still need to be private (in a follow up) so that all the constructors can make certain that you can't hold a Qubit whose bit pattern is invalid for the expectations of Wire (assuming a 30-bit int), but I think that's about the limits of what we want to enforce here via the type system. We can't actually enforce that the value stored inside makes sense, and at some point, that's the best we'd be doing by changing the integer types around, I think.
|
The context for the " |
NLayoutpredates the circuit qubit indices being in Rust. Now, though,NLayout::VirtualQubitandNLayout::PhysicalQubitare promises made on top of circuit qubits, so it makes sense for them to wrap the other type.This simplifies a reasonable amount of transpiler-pass code, since the conversations are easier. More importantly, this lays the groundwork for the
Qubittype to restrict its size to be slightly less than 32 bits.Summary
Details and comments
This is the first step in making the widths of the
Qubittype more changeable from a circuit context, without pushing the complexity onto all transpiler passes.