Use qubit-index newtypes in Rust space #10761
Conversation
jakelishman
added
Changelog: None
|
One or more of the the following people are requested to review this:
|
jakelishman
force-pushed
the
rust/qubit-newtype
branch
from
28dc70d to
906b87b
Compare
This converts all of our Rust-space components that are concerned with virtual and physical qubits (via the `NLayout` class) to represent those integers with newtypes wrapping them as half-size indices, and changes the `NLayout` API to only allow access via these types and not by raw integers. The way this is done should have no overhead in time usage from Rust, and is actually a reduction in memory usage because all the qubits are stored at half the width they were previously (for most systems). This is done to add type safety to all our components that were concerned with the mixing of these two qubits. The implementation of this commit already turned up the logical bug fixed by Qiskitgh-10756.
jakelishman
force-pushed
the
rust/qubit-newtype
branch
from
906b87b to
6c16345
Compare
There was a problem hiding this comment.
This looks great, thanks for doing this. Besides shrinking the memory overhead of sabre's rust component and vf2 scoring, leveraging the type checker to validate we're reasoning about the correct type of qubit is a big win. Also I really like the VirtualQubit::to_phys() and PhysicalQubit::to_virt() syntax it's a lot nicer looking than the syntax before.
Just a couple small comments/questions inline.
| } | ||
| } | ||
|
|
||
| unsafe impl numpy::Element for $id { |
There was a problem hiding this comment.
Is this the first unsafe block we have in the rust code so far? Maybe it's time we start using miri (https://github.com/rust-lang/miri) in CI. This is super minor use now and unlikely to have an issue as we're just implementing the numpy traits which are unsafe because of their nature. But just something to think about moving forward.
There was a problem hiding this comment.
I think but am not 100% sure that unsafe impl doesn't implicitly allow unsafe code within function definitions - it's just a marker that the trait's contract can't be reasonably enforced by the compiler. In this case, the object has to be safe to be managed by Numpy (give or take, I think that means it needs to be a valid Numpy scalar and not implement Drop), but there's no actual unsafe Rust code needed.
This reverts the changes to the coupling-map generation and the indexing into `NeighborTable` that were designed to make it more type safe in favour of doing that in a separate commit.
Summary
This converts all of our Rust-space components that are concerned with virtual and physical qubits (via the
NLayoutclass) to represent those integers with newtypes wrapping them as half-size indices, and changes theNLayoutAPI to only allow access via these types and not by raw integers.The way this is done should have no overhead in time usage from Rust, and is actually a reduction in memory usage because all the qubits are stored at half the width they were previously (for most systems). This is done to add type safety to all our components that were concerned with the mixing of these two qubits. The implementation of this commit already turned up the logical bug fixed by gh-10756.
Details and comments
The biggest use of
.index()is in Sabre in the distance matrices. I considering adding a newtype wrapper around the distance matrix that implementsIndex<[PhysicalQubit; 2], Output = f64>to enforce type-safe access to that during Sabre, but that can potentially be done in a follow-up commit. I think some other changes I'd potentially like to make around the Sabre interfaces might invalidate that, though, so I've left it for this commit.I've not made the fix of #10756, so this PR will conflict with that one. I'll fix up whichever merges second; it's an easy fix.
This PR also takes the opportunity to standardise on
VirtualQubitthroughout the Rust components, which is more consistent with our Python-space usage, and doesn't have the naming clash with the QEC concept of a logical qubit.edit: In (relatively casual) benchmarks I ran of this, I saw no change in runtime of
SabreSwap, which is as expected.