Leverage native UnitaryGate from rust #13765
Conversation
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:
|
|
I ran a quick benchmark of
The release from PyPI will have PGO too which likely speeds it up a little bit. |
mtreinish
force-pushed
the
use-unitary-gate-rs
branch
from
2a151ec to
8c36ffd
Compare
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.
mtreinish
force-pushed
the
use-unitary-gate-rs
branch
from
8c36ffd to
8bdc469
Compare
|
This should be ready for review now, I rebased this on top on main now that #13759 has merged. |
Pull Request Test Coverage Report for Build 13168722272Details
💛 - Coveralls |
| .get_bound(py) | ||
| .call1((array, py.None(), false))?; | ||
| let array: Matrix2<Complex64> = | ||
| Matrix2::from_row_iterator(matrix.into_iter().flat_map(|x| x.into_iter())); |
There was a problem hiding this comment.
It seems we'll end up needing a lot of these converters... it's a bit a bummer that nalgebra doesn't implement a From<Array2> or something
| } | ||
|
|
||
| // Update self.op_names | ||
| self.decrement_op(op_name.as_str()); |
There was a problem hiding this comment.
These look like they might do a roundtrip &str -> String -> &str?
There was a problem hiding this comment.
I did just copy what was there before I split this out to a method for working with PackedInstruction directly. I'll play with it a little bit and see if I can remove the double conversion (I have a feeling it's acting as a copy to avoid a borrow checker failure).
There was a problem hiding this comment.
Yeah I just looked at it the to_string() is to copy the string. There is a shared reference issue if a copy isn't done here because everything is based on a references to dag and we're mutating it. So we need to copy the name string before we mutate. But the decrement_op() and increment_op() methods require &str as the input type.
Summary
This commit builds off of the native rust representation of a
UnitaryGate added in #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
ust add unecessary conversions and allocations. However, once #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.
Details and comments
This is based on top of #13759 and will need to be rebased once that merges. Until then you can see the contents of this PR by looking at the HEAD commit on the PR branch: 8c36ffd