Fully port GatesInBasis to Rust.
#13034
Conversation
|
One or more of the following people are relevant to this code:
|
Pull Request Test Coverage Report for Build 11058624835Warning: 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 |
There was a problem hiding this comment.
Thanks @kevinhartman for this PR. I know it's currently blocked by #13056 but I wanted to take a look now to get more familiar with the latest contributions to our Rust crates. From my (limited) point of view, the changes looks good, I really like the additions you made beyond the GatesInBasis pass. I did get confused by some changes but then realized that the branch is a bit out of date so I can take a second look once it's updated.
crates/circuit/src/dag_circuit.rs
Outdated
| impl NodeType { | ||
| pub fn unwrap_operation(&self) -> &PackedInstruction { | ||
| match self { | ||
| NodeType::Operation(instr) => instr, | ||
| _ => panic!("Node is not an operation!"), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I may just go ahead and borrow this in my UnitarySynthesis WIP PR now because it's so much nicer than having to unwrap nodes manually (this will be merged earlier anyways).
kevinhartman
force-pushed
the
ox-gates-in-basis
branch
from
479feff
to
a3b9650
Compare
kevinhartman
force-pushed
the
ox-gates-in-basis
branch
from
28a86e5
to
5ecf22e
Compare
| // In the outer DAG, virtual and physical bits are the same thing. | ||
| let wire_map: HashMap<Qubit, PhysicalQubit> = | ||
| HashMap::from_iter((0..dag.num_qubits()).map(|i| { | ||
| ( | ||
| Qubit(i.try_into().unwrap()), | ||
| PhysicalQubit::new(i.try_into().unwrap()), | ||
| ) | ||
| })); | ||
|
|
There was a problem hiding this comment.
Is this mapping here mainly to reduce tha cost of re-building Qubits or PhysicalQubit instances from each other? Not that it needs any correction I just wanted to understand if this was the case.
There was a problem hiding this comment.
The purpose is actually to map qubits within control flow blocks from the indexing scheme used within the current block back to the indexing of the root circuit.
For example, if the root circuit has 5 qubits and has an if-else operation that uses qubits 3 and 4, then the inner circuits corresponding to the if and else blocks of that operation will have 2 qubits, indexed 0 and 1. To get from 0 and 1 each inner block's indexing scheme to 3 and 4 in the root circuit's indexing scheme, we use this map.
* Fully port gates_in_basis to Rust. * Revert visibility to no longer public. * Add docstring for unwrap_operation. * Remove unused py token.
Summary
Ports
GatesInBasisto Rust. The implementation avoids the prior conversion toDAGCircuitfor each control flow block, which should help with performance.Details and comments
Additional changes:
PyInstruction::blocks()for passes which need to get the blocks of a control flow operation as nativeCircuitData.NodeType::unwrap_operation()for callers who know that a DAG node is an operation.Targetinterface public for use in other crates (needed to accept a&Targetin apyfunction).DAGCircuit, naming makingdag,qubits, andclbitsprivate again.Includes some changes from #13056, #13013 and
#13006which ought to merge first.Resolves #12275