Move the circuit library's entanglement logic to Rust #12950
Conversation
Cryoris
added
Changelog: Bugfix
|
One or more of the following people are relevant to this code:
|
Pull Request Test Coverage Report for Build 10510704837Warning: 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 |
Cryoris
added
stable backport potential
thanks @alexanderivrii for the hint!
There was a problem hiding this comment.
Thanks @Cryoris - I only have some minor comments.
BTW, does porting this code to rust improves its performance ?
| if let Ok(strategy) = entanglement.downcast::<PyString>() { | ||
| let as_str = strategy.to_string(); | ||
| return Ok(Box::new( | ||
| get_entanglement_from_str(num_qubits, block_size, as_str.as_str(), offset)?.map(Ok), | ||
| )); | ||
| } else if let Ok(list) = entanglement.downcast::<PyList>() { | ||
| let entanglement_iter = list.iter().map(move |el| { | ||
| let connections = el | ||
| .downcast::<PyTuple>() | ||
| .expect("Entanglement must be list of tuples") // clearer error message than `?` | ||
| .iter()? | ||
| .map(|index| index?.downcast::<PyInt>()?.extract()) | ||
| .collect::<Result<Vec<u32>, _>>()?; | ||
|
|
||
| if connections.len() != block_size as usize { | ||
| return Err(QiskitError::new_err(format!( | ||
| "Entanglement {:?} does not match block size {}", | ||
| connections, block_size | ||
| ))); | ||
| } | ||
| Ok(connections) | ||
| }); | ||
| return Ok(Box::new(entanglement_iter)); | ||
| } | ||
| Err(QiskitError::new_err( | ||
| "Entanglement must be a string or list of qubit indices.", | ||
| )) |
There was a problem hiding this comment.
To me this looks good, however I would appreciate if someone with more knowledge of Rust/Pyo3 would take a look as well (maybe @raynelfss)? In particular, is there a more ergonomic way to convert python lists of integers into a rust vector (avoiding some of the downcasting/extracting)?
There was a problem hiding this comment.
If there's a way I'd be glad to know it -- the NLocal refactoring (on my local branch, not here) currently has quadruple nested lists which need casting to PyList 😅
| if entanglement == "pairwise" && block_size > 2 { | ||
| return Err(QiskitError::new_err(format!( | ||
| "block_size ({}) can be at most 2 for pairwise entanglement", | ||
| block_size | ||
| ))); | ||
| } |
There was a problem hiding this comment.
Could we add this to the match code below (by including ("pairwise", _) after handling the other "pairwise" cases)?
Summary
Move the entanglement logic used in the circuit library's
NLocalcircuits & friends to Rust. This PR is also the basis to move frequently used circuits, such asEfficientSU2,TwoLocalorZZFeatureMapto Rust.This also fixes a bug in the
circularentanglement which missed some entanglement blocks for entangling gates with 3+ qubits.Details and comments
NLocalwill directly use this function, but there's also a series of tests to check the different errors and possible input types.The attentive reader might notice that handling elaborate constructs such as
list[list[list[int]]]are not handled, which does not affect the current code, but will change what we accept once the circuit library is moved to Rust. That's an intentional change as these types were used to allow different entanglements for different circuit layers (controlled byoffset), but this was redundantly covered by passing a callable. Moving forward, the suggestion is to only allow a callable in these cases to have only 1 method of doing 1 thing 🙂