Add unitary gate representation to rust #13759
Merged
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
mtreinish
added
on hold
mtreinish
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 13166968930Details
💛 - Coveralls |
mtreinish
added a commit
to mtreinish/qiskit-core
that referenced
this pull request
Jan 30, 2025
This commit builds off of the native rust representation of a UnitaryGate added in Qiskit#13759 and uses the native representation everywhere we were using UnitaryGate in rust via python previously: the quantum_volume() function and consolidate blocks. One future item is consolidate blocks can be updated to use nalgebra types internally instead of ndarray as for the 1 and 2q cases we know the fixed size of the array ahead of time. However the block consolidation code is built using ndarray currently and later synthesis code also works in ndarray so there isn't any real benefit yet, and we'd just add unecessary conversions and allocations. However, once Qiskit#13649 merges this will change and it would make more sense to add the unitary gate with a Matrix4. But this can be handled separately after this merges.
mtreinish
added a commit
to mtreinish/qiskit-core
that referenced
this pull request
Jan 30, 2025
This commit builds off of the native rust representation of a UnitaryGate added in Qiskit#13759 and uses the native representation everywhere we were using UnitaryGate in rust via python previously: the quantum_volume() function and consolidate blocks. One future item is consolidate blocks can be updated to use nalgebra types internally instead of ndarray as for the 1 and 2q cases we know the fixed size of the array ahead of time. However the block consolidation code is built using ndarray currently and later synthesis code also works in ndarray so there isn't any real benefit yet, and we'd just add unecessary conversions and allocations. However, once Qiskit#13649 merges this will change and it would make more sense to add the unitary gate with a Matrix4. But this can be handled separately after this merges.
This commit expands the rust circuit data model to include a definition of UnitaryGate. This is a more abstract operation than what we've defined so far in Rust, a gate represented solely by it's unitary matrix but during normal transpilation we create and interact with these operations for optimization purposes so having a native representation in rust is necessary to avoid calling to Python when working with them. This introduces a new UnitaryGate struct which represents the unpacked operation. It has 3 internal storage variants based on either an ndarray arbitrary sized array, a 2x2 nalgebra fixed sized array, or a 4x4 nalgebra fixed sized array. From the python perspective these all look the same, but being able to internally work with all 3 variants lets us optimize the code paths for 1q and 2q unitary gates (which are by far more common) and avoid one layer of pointer indirection. When stored in a circuit the packed representation is just a pointer to the actual UnitaryGate which we put in a Box. This is necessary because the struct size is too large to fit in our compact representation of operations. Even the ndarray which is a heap allocated type requires more than our allotted space in a PackedOperation so we need to reduce it's size by putting it in a Box. The one major difference here from the previous python based representation the unitary matrix was stored as an object type parameter in the PackedInstruction.params field, which now it is stored internally in the operation itself. There is arguably a behavior change around this because it's no longer possible to mutate the array of a UnitaryGate in place once it's inserted into the circuit. While doing this was horribly unsound, because there was no guardrails for doing it a release note is added because there is a small use case where it would have worked and it wasn't explicitly documented. Closes Qiskit#13272
mtreinish
force-pushed
the
unitary-gate-rs
branch
from
9bfe14b to
5b3b80b
Compare
|
I've rebased this now that #13486 has merged and it shouldn't be blocked anymore. |
One other small breaking change with this move in the data model is that UnitaryGate in rust doesn't store it's matrix as a parameter because it doesn't fit logically in the rust data model for circuits. This wasn't a problem before recent changes to BasicSimulator, but recent changes started assuming this. This commit updates this in the basic simulator and then adds a release note to document the change.
kevinhartman
reviewed
Feb 5, 2025
There was a problem hiding this comment.
This is so beautiful (lol). It's nice to see how easily it fits in with the rest of our PackedOperation data model.
Perhaps worth noting, the impl_packable_pointer! macro also implements From<UnitaryGate> for PackedOperation, and that will automatically move the unitary into a Box and pack it into PackedOperation. But, I think what you're doing in FromPyObject for OperationFromPython is more likely to be optimized to elide a copy from stack to heap, so no changes are requested.
Just a few small comments, otherwise!
| [OperationRef::Unitary(op_a), OperationRef::Unitary(op_b)] => { | ||
| match [&op_a.array, &op_b.array] { | ||
| [ArrayType::NDArray(a), ArrayType::NDArray(b)] => { | ||
| Ok(relative_eq!(a, b, max_relative = 1e-5, epsilon = 1e-8)) |
There was a problem hiding this comment.
Less of a comment on this PR and more of a general question, but what is max_relative vs epsilon, exactly?
I spent some time looking at the docs for the approx crate, but I could quite grok it.
There was a problem hiding this comment.
Hah, what docs? It's very sparse from what I remember.
The relative_eq! macro is basically checking something like: abs(a - b) <= epsilon + max_relative * abs(b). The equivalent in python land would be numpy.is_close() where epsilon is the atol argument and max_relative is rtol. You can look at the impl for the RelativeEq trait for f64 to get a feeling for how it works.
kevinhartman
approved these changes
Feb 5, 2025
mtreinish
added a commit
to mtreinish/qiskit-core
that referenced
this pull request
Feb 5, 2025
This commit builds off of the native rust representation of a UnitaryGate added in Qiskit#13759 and uses the native representation everywhere we were using UnitaryGate in rust via python previously: the quantum_volume() function, consolidate blocks, split2qunitaries, and unitary synthesis. One future item is consolidate blocks can be updated to use nalgebra types internally instead of ndarray as for the 1 and 2q cases we know the fixed size of the array ahead of time. However the block consolidation code is built using ndarray currently and later synthesis code also works in ndarray so there isn't any real benefit yet, and we'd just add unecessary conversions and allocations. However, once Qiskit#13649 merges this will change and it would make more sense to add the unitary gate with a Matrix4. But this can be handled separately after this merges.
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 commit expands the rust circuit data model to include a definition
of UnitaryGate. This is a more abstract operation than what we've
defined so far in Rust, a gate represented solely by it's unitary matrix
but during normal transpilation we create and interact with these
operations for optimization purposes so having a native representation
in rust is necessary to avoid calling to Python when working with them.
This introduces a new UnitaryGate struct which represents the unpacked
operation. It has 3 internal storage variants based on either an ndarray
arbitrary sized array, a 2x2 nalgebra fixed sized array, or a 4x4
nalgebra fixed sized array. From the python perspective these all look
the same, but being able to internally work with all 3 variants lets us
optimize the code paths for 1q and 2q unitary gates (which are by far
more common) and avoid one layer of pointer indirection.
When stored in a circuit the packed representation is just a pointer to
the actual UnitaryGate which we put in a Box. This is necessary because
the struct size is too large to fit in our compact representation of
operations. Even the ndarray which is a heap allocated type requires more
than our allotted space in a PackedOperation so we need to reduce it's
size by putting it in a Box.
The one major difference here from the previous python based
representation the unitary matrix was stored as an object type parameter
in the PackedInstruction.params field, which now it is stored internally
in the operation itself. There is arguably a behavior change around
this because it's no longer possible to mutate the array of a
UnitaryGate in place once it's inserted into the circuit. While doing
this was horribly unsound, because there was no guardrails for doing it
a release note is added because there is a small use case where it would
have worked and it wasn't explicitly documented.
Details and comments
Closes #13272
A follow-up PR to this one will start to use UnitaryGate directly in rust instead of using the Python version.
This is currently based on top of #13486 and will need to be rebased when that merges. To view the contents of just this PR without #13486 you can look at the HEAD commit: 9bfe14b