Create qiskit_circuit::dag submodule
#13721
Conversation
This rearranges some of the files of the `qiskit_circuit` crate to localise a `dag`-specific module. This is primarily a code-organisation thing, but also intended to allow some better visibility scoping in follow-up commits, particularly around the boundaries where `unsafe` functions must be considered.
jakelishman
added
type: qa
|
One or more of the following people are relevant to this code:
|
Pull Request Test Coverage Report for Build 13134192504Details
💛 - Coveralls |
| @@ -20,8 +20,7 @@ use hashbrown::HashMap; | |||
| use pyo3::prelude::*; | |||
|
|
|||
| use pyo3::types::{PyDict, PyList}; | |||
| use qiskit_circuit::dag_circuit::{DAGCircuit, NodeType, Wire}; | |||
| use rustworkx_core::petgraph::stable_graph::NodeIndex; | |||
| use qiskit_circuit::dag::{DAGCircuit, NodeIndex, NodeType, Wire}; | |||
There was a problem hiding this comment.
It's nice to now have petgraph's NodeIndex reexported under dag :)
There was a problem hiding this comment.
ha yeah, mostly this is speculative because of the follow-up of #13696, where OperationIndex will be imported from there too
| @@ -18,7 +18,7 @@ | |||
| use std::collections::BTreeMap; | |||
There was a problem hiding this comment.
Probably out of scope, but I wonder if we should make dot_utils an inline submodule of circuit so that we can make DAGCircuit::dag() private (IIRC, dot_utils is the only external module that uses it).
There was a problem hiding this comment.
I was actually really surprised to learn that dot_utils is the only place that accesses the petgraph object directly - I could totally imagine more places where it would make sense to have mutable access to the raw DAG tbh. I'm almost sure we used to have more raw accessors of DAGCircuit._multi_graph from Python space.
Right now, I think having dot_utils hidden away in its own file is possibly better - I can't think of any way you can break anything with immutable access to the petgraph object, and I wouldn't be surprised if we start wanting it a bit more in the future?
| /// | ||
| /// These keys are [Eq], but this is semantically valid only for keys from the same owning object. | ||
| #[derive(Copy, Clone, Debug, Hash, Ord, PartialOrd, Eq, PartialEq)] | ||
| pub struct Var(pub BitType); |
There was a problem hiding this comment.
I suppose this is fine here, though I had intended to make it completely opaque outside of DAGCircuit (this I tried to do here, though I'm likely to close that PR in favor of another that changes ::new(usize) to ::new(u32) for our bit types).
The issue I have with it being transparent is that unlike index for Qubit and Clbit, its index doesn't mean anything—it's just an implementation detail of how DAGCircuit happens to store variables. But maybe hiding that is hairier than just letting it leak. Thoughts?
There was a problem hiding this comment.
DAG consumers have to be able to inspect wires, so they need to be able to inspect the type of the Var wires, so it'll never manage to be properly private, and I was thinking it makes most sense to be forcibly consistent with the other things it's like.
For bit types: I'm in the process of writing a series of PRs (of which #13734 is the first) that restrict the Qubit, Clbit and Var types to 30 bits, so that the DAG wire can become a u32 instead of being 64 bits (we talked about this in the team meeting on Thursday while you were out, but if you're strongly opposed we can reconsider). A side effect of that is that it divorces us from the idea that there's a natural type that consumers of the type can pass in that's provably statically lossless for conversions, so I wasn't really worrying about it in typing anymore, and typically having new be usize is most convenient. If nothing else, it's consistent with the petgraph indices, so we're already in that boat with other types.
(Fwiw, that patch series also very heavily touches BitData, though I'll probably reconsider exactly how much I touch it while Ray's #13686 is still in flight.)
There was a problem hiding this comment.
Oh, to finish the thought on it: my patch series also makes the internals of Qubit, Clbit and Var private, and you can only get at them through the index method that returns a usize; the type is logically supposed to be the indexer into a list, because there's a promise in both circuits that the values are 0..num_qubits.
(I also stuck in impls between all primitive unsigned types and Qubit, Clbit, Var for whichever of From and TryFrom made sense, but new and index are the only named constructor methods.)
This rearranges some of the files of the
qiskit_circuitcrate to localise adag-specific module. This is primarily a code-organisation thing, but also intended to allow some better visibility scoping in follow-up commits, particularly around the boundaries whereunsafefunctions must be considered.Summary
Details and comments
The follow-up I'm referring to in the commit message is the comment chain here: #13696 (comment).