Remove Python dependency for creating a blank dag #14069
Conversation
|
One or more of the following people are relevant to this code:
|
This commit removes the py token argument for the dag constructor which wasn't strictly needed. The only pieces of the dagcircuit struct which need python is the metadata atribute (which is an `Option<PyDict>`) and the var tracking sets. This commit rearranges the constructor so it doesn't actively need python. This is achieved by moving the var tracking sets inside a once lock which is only initialized in a context where vars are being used. The metadata is changed to None by default. The other use case was the control flow module which wasn't strictly required and is removed in this PR and using the import once cell mechanism that we use for this everywhere else.
448d1ee to
d7aefa3
Compare
Pull Request Test Coverage Report for Build 14039322959Warning: 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 |
| pub fn new() -> PyResult<Self> { | ||
| Ok(DAGCircuit { | ||
| name: None, | ||
| metadata: None, | ||
| dag: StableDiGraph::default(), | ||
| qregs: RegisterData::new(), | ||
| cregs: RegisterData::new(), | ||
| qargs_interner: Interner::new(), | ||
| cargs_interner: Interner::new(), | ||
| qubits: ObjectRegistry::new(), | ||
| clbits: ObjectRegistry::new(), | ||
| vars: ObjectRegistry::new(), | ||
| global_phase: Param::Float(0.), | ||
| duration: None, | ||
| unit: "dt".to_string(), | ||
| qubit_locations: BitLocator::new(), | ||
| clbit_locations: BitLocator::new(), | ||
| qubit_io_map: Vec::new(), | ||
| clbit_io_map: Vec::new(), | ||
| var_io_map: Vec::new(), | ||
| op_names: IndexMap::default(), | ||
| vars_info: HashMap::new(), | ||
| vars_by_type: VarsByType::new(), | ||
| captured_stretches: IndexMap::default(), | ||
| declared_stretches: IndexMap::default(), | ||
| }) | ||
| } | ||
|
|
There was a problem hiding this comment.
It might be better to have DAGCircuit implement (or derive) the Default trait. Although some of the types here would need to implement it as well so might be a little more effort than needed.
There was a problem hiding this comment.
It's more than just what a Default derive would provide, things like unit are not actually the default. I could do this as a custom default impl for DAGCircuit, but I'm not sure there is much benefit. Either way if we do need Default later, it's simple enough to add.
| pub fn new() -> PyResult<Self> { | ||
| Ok(DAGCircuit { | ||
| name: None, | ||
| metadata: None, |
There was a problem hiding this comment.
Is there any effect from having the metadata be None instead of an empty dict during initialization? I'm assuming the effect should be minimal. But this would mean that any DAGCircuit coming fom rust directly would have no metadata.
There was a problem hiding this comment.
There shouldn't be because we allowed None as a type for the metadata. If we are returning a rust generated DAG to python it is already able to handle this: https://github.com/Qiskit/qiskit/blob/main/qiskit/converters/dag_to_circuit.py#L75
The only reason I separated it out the python new though was because the default for Python's DAGCircuit() was intended to be an empty dict for metadata and I didn't want to change that expectation.
Summary
This commit removes the py token argument for the dag constructor which wasn't strictly needed. The only pieces of the dagcircuit struct which need python is the metadata atribute (which is an
Option<PyDict>) and the var tracking sets. This commit rearranges the constructor so it doesn't actively need python. This is achieved by moving the var tracking sets inside a once lock which is only initialized in a context where vars are being used. The metadata is changed to None by default.The other use case was the control flow module
which wasn't strictly required and is removed in this PR and using the import once cell mechanism that we use for this everywhere else.
Details and comments