Refactor encapsulation of Bit storage #13377
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Co-authored-by: John Lapeyre <[email protected]>
By limiting its visibility to the `qiskit-circuit` crate, we can hide the storage type used by Qubit and Clbit as an implementation detail of the circuit crate (also done in this PR). As a result of this change, the accelerate crate (and future crates) are forced to use the `::new` and `::index` methods of `Qubit` and `Clbit`, which deal in terms of usize.
The BitData type was only ever intended as an implementation detail of CircuitData and DAGCircuit. As such, its visibility has been downgraded from pub to pub(crate). By limiting visibility of BitData to the circuit crate, we can fully encapsulate the storage type of Qubit, Clbit, and dagcircuit::Var. We also encourage the accelerate crate to avoid writing code which depends on conversion to and from the Python bit representations (there were only two existing places where BitData was being used in accelerate, and it'd be nice to avoid any more.
Also removes From<u32> and Into<u32> implementations for bit types (and DAGCircuit's Var type) to improve data encapsulation outside of the circuit crate. In its place, we now have WireIndex which is private to the circuit crate, which we implement for our wire types and use as a bound for types used with BitData going forward.
kevinhartman
changed the title
kevinhartman
requested review from
alexanderivrii,
ShellyGarion and
a team
as code owners
|
One or more of the following people are relevant to this code:
|
Pull Request Test Coverage Report for Build 11787261722Details
💛 - Coveralls |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR follows up #13284 (which added an interface for converting bit types into and out of
usize) with encapsulation of the internal representation used for bits. Previously,QubitandClbitwere transparent wrappers aroundu32. Their internal representation is now private to encourage users to deal with bits directly or asusizevia theirindex. As part of this, I've changed a lot of API surface inaccelerateto deal in terms ofusizefor bit indices as well.Details and comments
BitDatastruct has also been made private to the circuit crate. It was never intended as API surface for other crates, but rather as a short term way for us to track the association between native and Python bits. To support this, a new crate private typeWireIndexhas been added which is implemented byQubit,ClbitandVar. It serves as a bound for the typeTused in aBitData<T>and provides crate-private conversion between our wire types andusize. With these changes, it is (intentionally) also no longer possible forVarto be constructed outside of the circuit crate.CircuitData::from_dagfunction has also been added to support this encapsulation.From<Qubit>trait is also now implemented forPhysicalQubitandVirtualQubit(though these really also ought to move to the circuit crate soon...).PyInstructionandPyGatestill publicly store theirnum_qubitsandnum_clbitsasu32. We should probably consider making these fieldscrate(pub)as well.Needs a rebase on #13368.