Port ElidePermutations transpiler pass to Rust#13094
Port ElidePermutations transpiler pass to Rust#13094kevinhartman merged 20 commits intoQiskit:mainfrom
ElidePermutations transpiler pass to Rust#13094Conversation
|
One or more of the following people are relevant to this code:
|
Pull Request Test Coverage Report for Build 11158897921Details
💛 - Coveralls |
kevinhartman
left a comment
kevinhartman
left a comment
There was a problem hiding this comment.
Just a few small comments.
| let mapped_qubits = new_dag.set_qargs(&mapped_qargs); | ||
| mapped_inst.qubits = mapped_qubits; | ||
| new_dag.push_back(py, mapped_inst)?; | ||
| let params = inst.params.clone(); |
There was a problem hiding this comment.
Is this still needed? Or can we directly do inst.params.as_deref().cloned() in the stuct initializer?
There was a problem hiding this comment.
Good catch, this is a remnant of my own failed attempts to clone params correctly. Fixed in 8209757.
There was a problem hiding this comment.
Sounds good. We should probably (in a separate PR) expose a method on PackedInstruction as well which just returns self.params.as_deref() for convenience.
There was a problem hiding this comment.
There was a problem hiding this comment.
Hmm. Perhaps what I'm suggesting would be more confusing, then. The params_view method returns a &[Param] slice, but what we'd need here is the owned representation of parameters stored by DAGCircuit (Option<SmallVec<[Param; 3]>>).
That makes me think we may want to consider changing DAGCircuit::apply_operation_{back,front} to take &[Param] instead of owned data. It'd be less efficient for callers that happen to already have owned parameters to move in during the apply, but it's certainly a cleaner interface and hides what is otherwise an implementation detail for how DAGs store parameters.
kevinhartman
left a comment
kevinhartman
left a comment
There was a problem hiding this comment.
This looks good to me. Thanks for the changes (and for trying out the new Rust-facing DAGCircuit::apply_operation_back!).
| @@ -0,0 +1,5 @@ | |||
| --- | |||
| features_transpiler: | |||
| - | | |||
There was a problem hiding this comment.
I think we haven't actually written release notes like this for any (most of?) the other passes being ported, have we? IMO, we can leave this in for now and the release manager (whoever that ends up being) can just remove this if it's out of place.
* initial commit * Rust docstring improvements * lint * restoring elided comment * explicitlt setting dtype=int for permutation gates * another attempt * fmt after merge * update after merge + comment from code review * switching to apply_operation_back * Comments from code review * fix using params --------- Co-authored-by: Kevin Hartman <kevin@hart.mn>
ShellyGarion
added
the
Changelog: Added
7 participants
Summary
Closes #12336.
This PR reimplements most of the logic of the
ElidePermutationstranspiler pass in Rust (the only thing missing is updating the passmanager's property set to account for the new mapping, this is still done in the Python space).The Rust code also corrects a problem in the Python code where the qubit mapping was not updated correctly in the presence of permutation gates over a subset of qubits -- it's really scary how broken it was and that none of our tests were catching this (and I have added an extra python test that would exhibit the bug).
On one simple test with 100,000 CX-gates and 99,800 SWAP-gates (multiple iterating layers of CX-gates and SWAP-gates), the runtime is improved from
1.388seconds to0.094seconds -- about 10x speedup; and on another simple quantum-volume-like test (multiple iterating layers of random unitary and permutation gates) the runtime is improved from0.128seconds to0.020seconds -- about 5x speedup.Details and comments
I have needed to expose some of the functionality from
DAGCircuit,First, I needed the pure Rust version of the functionSecond, I needed to make some of the functions public, namely:count_ops()which I have copied from some other PR, most probably from #13013 (thus it's probably best to wait till this merges and rebase the code on top of it).push_back,topological_op_nodesandcopy_empty_like.Third, I needed the setter variantset_qargs(and we already hadget_qargs), and I also addedset_cargsfor consistency.Thanks to @jakelishman and @Cryoris for help and suggestions!