Prepare PauliEvolutionGate for Rustiq & port it to Rust
#13295
Conversation
Pull Request Test Coverage Report for Build 11667384085Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
--> expand should return float | ParameterExpression
some things are still upside down and it seems like it would be cleaner to do the time multiplication inside pauli_evolution
Cryoris
changed the title
PauliEvolutionGate plugin structure & rustiq integrationPauliEvolutionGate for Rustiq & port it to Rust
|
One or more of the following people are relevant to this code:
|
| time: Param, | ||
| phase_gate: bool, | ||
| do_fountain: bool, | ||
| ) -> Box<dyn Iterator<Item = StandardInstruction> + 'a> { |
There was a problem hiding this comment.
Why are we using a Box here? From what I understand, using a Box pointer can save memory by storing things in the heap rather than the stack. But is it that much more expensive than just returning a regular iterator? Standard gates aren't supposed to be too heavy either.
This isn't critical as performance wouldn't be impacted from what I can tell since StandardGate instances are very light. When it comes to Param instances, it might be a bit trickier. But it is something to think about.
There was a problem hiding this comment.
This is because this function returns different types of Iterator depending on the input (e.g. Chain<Map<...>> vs Empty). The dynamic type was the only way I got to work, but I'm happy to change it if there's a better way 🙂
There was a problem hiding this comment.
From a recent talk I had with @alexanderivrii it seems that you want to call the .rev method to be able to reverse the iterator. If so, perhaps you could change the return types to be DoubleEndedIterator instances instead of a Box<dyn Iterator<_>>.
So you could change some of the return types from:
pub fn foo() -> Box<dyn Iterator<Item = StandardInstruction> + 'a> {to:
pub fn foo() -> impl DoubleEndedIterator<Item = StandardInstruction> + 'a {This would allow you to use any iterator type as long as it can be reversed, which seems to be the case for many of the iterators used here.
| .clone() | ||
| .into_iter() |
There was a problem hiding this comment.
For this I think you might need to rewrite the logic quite a bit to avoid some cloning, since you're cloning the iterator's source each time. Maybe start by using iter().copied() here since both structures implement the Copy trait.
You might also be better off collecting some of them instead of cloning the source. At least for this one I think you could get away with collecting, and maybe the inverse_basis_change as well since you will re-append. Then you can call into-iter() at the end to consume them.
| /// Get an iterator that returns a barrier or an empty element. | ||
| pub fn maybe_barrier( | ||
| py: Python, | ||
| num_qubits: u32, | ||
| insert_barriers: bool, | ||
| ) -> Box<dyn Iterator<Item = PyResult<Instruction>>> { | ||
| // TODO could speed this up by only defining the barrier class once | ||
| if !insert_barriers { | ||
| Box::new(std::iter::empty()) | ||
| } else { | ||
| let barrier_cls = imports::BARRIER.get_bound(py); | ||
| let barrier = barrier_cls | ||
| .call1((num_qubits,)) | ||
| .expect("Could not create Barrier Python-side"); | ||
| let barrier_inst = PyInstruction { | ||
| qubits: num_qubits, | ||
| clbits: 0, | ||
| params: 0, | ||
| op_name: "barrier".to_string(), | ||
| control_flow: false, | ||
| instruction: barrier.into(), | ||
| }; | ||
| Box::new(std::iter::once(Ok(( | ||
| barrier_inst.into(), | ||
| smallvec![], | ||
| (0..num_qubits).map(Qubit).collect(), | ||
| vec![] as Vec<Clbit>, | ||
| )))) | ||
| } | ||
| } |
There was a problem hiding this comment.
This method is a little confusing:
- From what I can gather it returns a Barrier as an
Instructionbut then it returns aBox<Iterator<_>>instance, even though it is one item. Is this a memory saving measure? - If the purpose of this operation is to just return a
Barrierif a condition is met, you should probably have this labeled as#[inline]since it's only a conditional check. This prevents the compiler from seeing an extra call to this function. - Going back to @alexanderivrii's comment, could you maybe use
insert_barrier.then_some().
There was a problem hiding this comment.
I changed to then_some in cf7d8b0 but I think this has one disadvantage: when we write something like
instructions.chain(
insert_barrier.then_some(Ok(barrier.clone())
)Then even if insert_barrier is false, I need to make the Python call to construct the barrier object. To avoid that, what I would like to do is
let barrier = match insert_barrier {
true => Some(.... get barrier instruction...),
false => None,
}
// in my iteration loop ...
instructions.chain(
insert_barrier.then_some(Ok(barrier.clone().unwrap()))
)but rust won't allow this since barrier can be None and I'm not allowed to unwrap 🤔
So it seems we have a performance vs. code legibility issue here, which is why I had the maybe_barrier function. How about making this into a struct with some documentation? Like
struct MaybeBarrier {
barrier: Option<Instruction>
}
impl MaybeBarrier {
fn from_py(...) // construct the barrier object
fn get(condition) // return None or the barrier based on the condition
}
|
Hmm, after switching Other than that, I am very happy with this PR, thanks for the great work! |
There was a problem hiding this comment.
I had a bit of a discussion with @alexanderivrii about the usage of Box here, and we believe to have found a better alternative to what's being done here. I haven't tested it myself but it might be worth taking a look.
| time: Param, | ||
| phase_gate: bool, | ||
| do_fountain: bool, | ||
| ) -> Box<dyn Iterator<Item = StandardInstruction> + 'a> { |
There was a problem hiding this comment.
From a recent talk I had with @alexanderivrii it seems that you want to call the .rev method to be able to reverse the iterator. If so, perhaps you could change the return types to be DoubleEndedIterator instances instead of a Box<dyn Iterator<_>>.
So you could change some of the return types from:
pub fn foo() -> Box<dyn Iterator<Item = StandardInstruction> + 'a> {to:
pub fn foo() -> impl DoubleEndedIterator<Item = StandardInstruction> + 'a {This would allow you to use any iterator type as long as it can be reversed, which seems to be the case for many of the iterators used here.
|
For the record, I am no longer sure what was our conclusion with @raynelfss (Ray is so ahead of me when discussing Rust types). I know that @Cryoris used boxing to be able to return both the usual and the double-ended iterators (and maybe something else too), yet if I remember correctly the double-ended iterators are a bit heavier than the usual ones. Does it make sense for us to return the double-ended iterators everywhere? Another possibility is maybe we should return the |
|
In hindsight @Cryoris, we might not be able to use a simple
Even though both |
|
|
||
| // custom types for a more readable code | ||
| type StandardInstruction = (StandardGate, SmallVec<[Param; 3]>, SmallVec<[Qubit; 2]>); | ||
| type Instruction = ( |
There was a problem hiding this comment.
Perhaps the name "Instruction" can be confusing with Qiskit Instruction? Maybe call it something like "EvolutionIstruction" ?
There was a problem hiding this comment.
This typedef represents a packed instruction, which can be used as input to CircuitData.from_packed_instructions (so it's not specific to an evolution). The same is used in some other places (e.g. quantum volume or pauli feature map), so I'd prefer keeping as is for now and potentially adding a general typedef used across all of the rust code 🙂
| @@ -85,10 +86,10 @@ class PauliEvolutionGate(Gate): | |||
|
|
|||
| def __init__( | |||
| self, | |||
| operator, | |||
| time: Union[int, float, ParameterExpression] = 1.0, | |||
There was a problem hiding this comment.
why is ParamterExpression replaced by ParameterValueType ? (isn't this considered as an API change) ?
There was a problem hiding this comment.
It's the same, ParameterValueType is a typedef used across the circuit and the gates, it's defined as
ParameterValueType = Union[ParameterExpression, float](see the quantumcircuit.py file)
There was a problem hiding this comment.
Thanks @Cryoris! Let me approve the PR, but let's also give a chance to @ShellyGarion and @raynelfss to see if they have additional comments.
ShellyGarion
added
the
Changelog: New Feature
* py version for expand * starting to write some code * implementing * cleanup * cleanup * expand fully & simplify lie trotter * use examples that actually do not commute * add plugin structure * fixing global phase for all-I rotations * fixes * fixing plugin names * minor * removing a random print statement * additional improvements * improving rustiq plugin * merge with #13239 * Adding pauli evolution plugins to docstrings * adding documentation on rustiq plugin * fixes after refactoring * typo * more merges with #13295; adding more Rustiq tests * more efficient append_sx and append_sxdg gates for cliffords * review comments * moving the pauli network synthesis logic into a separate file * some code review suggestions * simplifying the code by merging the oredered and unorderd version of rotation injection * more review comments * adding python tests * more code review suggestions * more review comments * more review comments * test for preserve_order * lint * upgrading rustiq-core to 0.0.10 * clippy: removing mutable ref * Improving PauliEvolution synthesis tests. Making sure that the number of rotation gates in the synthesized circuit equals the number of non-trivial Pauli rotation gates. * documentation fixes after the merge --------- Co-authored-by: Julien Gacon <[email protected]>
Summary
Port the
PauliEvolutionGatesynthesis to Rust, plus, expose the Pauli network and allow other plugins to synthesize the gate. Also adds the plugin structure for the gate for #12789.Details and comments
The larger the Pauli network to synthesis, the better the speedup from the port to Rust. Here I measured a Heisenberg Hamiltonian (XX+YY+ZZ on interacting qubits, plus 1-qubit Z on each qubit) on a square lattice for different settings:
1 timestep for first order Trotter: speedup @ 100 qubits is 2.8
10 timestep for 4th order Trotter: speedup @ 100 qubits is 9.4
1 timestep for first order Trotter but with wrap=True: speedup @ 100 qubits is 3.6
Side effects of this PR:
SparsePauliOp.to_sparse_listto construct the sparse list format (i.e.op == SparsePauliOp.from_sparse_list(op.to_sparse_list()))LieTrotterto just be an alias ofSuzukiTrotterand reduce code duplication