Brand operation indices with `OperationIndex` #13696

jakelishman · 2025-01-20T18:36:13Z

Summary

Operations on the DAGCircuit that return NodeIndexs that are known to refer to Operation instances now return a transparent wrapper type, OperationIndex. DAGCircuit can be indexed with an OperationIndex, which returns the &PackedInstruction directly, rather than requiring the caller to indirect through the OperationType enum, which 99% of the time they don't care about.

DAGCircuit methods that are required to act on operations now take OperationIndex arguments, shifting the typing validation left; the compiler now effectively tracks provenance of indices in the type system. You can convert a NodeIndex into an OperationIndex as part of a DAG lookup by using DAGCircuit::get_operation, which returns the &PackedInstruction (if available), and upgrades the NodeIndex into an OperationIndex at the same time. Most of the time this is not necessary, as the methods that can only return operation nodes now return OperationIndex.

Most of the changes in this commit are simplifying relevant call sites, and removing if let NodeType::Operation(inst) = &dag[node_index] { } constructs, since they were automatically flattened into let inst = &dag[op_index].

Details and comments

This was the goal I had in mind at the end of my little sequence of patches late last week (#13680, #13681, #13682, #13683). I don't love the unsafe casting of the Vec for collect_1q_runs etc - perhaps we can extend the rustworkx API to allow a mapping function, so we can do it all in safe no-copy Rust.

A little over one third of the diff of this PR is whitespace-only changes where the level of indent is reduced. For example, elide_permutations.rs claims a diffstat of +49,-52, but with git diff -b it reduces to +2,-5.

Operations on the `DAGCircuit` that return `NodeIndex`s that are known to refer to `Operation` instances now return a transparent wrapper type, `OperationIndex`. `DAGCircuit` can be indexed with an `OperationIndex`, which returns the `&PackedInstruction` directly, rather than requiring the caller to indirect through the `OperationType` enum, which 99% of the time they don't care about. `DAGCircuit` methods that are required to act on operations now take `OperationIndex` arguments, shifting the typing validation left; the compiler now effectively tracks provenance of indices in the type system. You can convert a `NodeIndex` into an `OperationIndex` as part of a DAG lookup by using `DAGCircuit::get_operation`, which returns the `&PackedInstruction` (if available), and upgrades the `NodeIndex` into an `OperationIndex` at the same time. Most of the time this is not necessary, as the methods that can only return operation nodes now return `OperationIndex`. Most of the changes in this commit are simplifying relevant call sites, and removing `if let NodeType::Operation(inst) = &dag[node_index] { }` constructs, since they were automatically flattened into `let inst = &dag[op_index]`.

qiskit-bot · 2025-01-20T18:36:18Z

One or more of the following people are relevant to this code:

@Qiskit/terra-core

coveralls · 2025-01-20T19:09:03Z

Pull Request Test Coverage Report for Build 12873983166

Details

741 of 801 (92.51%) changed or added relevant lines in 19 files are covered.
12 unchanged lines in 2 files lost coverage.
Overall coverage increased (+0.03%) to 88.953%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
crates/accelerate/src/elide_permutations.rs	44	45	97.78%
crates/accelerate/src/split_2q_unitaries.rs	34	35	97.14%
crates/accelerate/src/consolidate_blocks.rs	26	29	89.66%
crates/circuit/src/dag_circuit.rs	488	543	89.87%

Files with Coverage Reduction	New Missed Lines	%
crates/qasm2/src/lex.rs	4	91.98%
crates/circuit/src/dag_circuit.rs	8	89.3%

Totals
Change from base Build 12868603604:	0.03%
Covered Lines:	79467
Relevant Lines:	89336

💛 - Coveralls

kevinhartman

This is a neat idea and makes working with the op node API surface of DAGCircuit much cleaner.

Just a few minor comments.

kevinhartman · 2025-01-21T20:36:44Z

crates/circuit/src/dag_circuit.rs

+///
+/// The safety guarantees of [OperationIndex::new] must be upheld for each of the values in `val`.
+#[inline]
+unsafe fn cast_node_vec_to_operation_vec(nodes: Vec<NodeIndex>) -> Vec<OperationIndex> {


Out of curiosity, is there any chance Rust is smart enough that we don't actually need this? I.e., if we do nodes.into_iter().map(|n| OperationIndex(n)).collect::<Vec<_>>(), does that in effect do this anyway?

It looks like Vec's FromIterator does something really similar: https://doc.rust-lang.org/beta/src/alloc/vec/spec_from_iter.rs.html#49-58

I don't know, honestly. Even if it does on one compiler, I'm not sure if it'd feel comfortable relying on the behaviour, though equally, I don't love this unsafe function at all either.

This function is one of my biggest problems with this PR - I think overall it's decent, but the end result isn't fully as nice as I'd hoped. Not sure what exactly I have a problem with, but it definitely didn't feel like a slam-dunk when I looked at the final diff.

I think overall it's decent, but the end result isn't fully as nice as I'd hoped.

I mean the entire PR, btw.

kevinhartman · 2025-01-21T22:26:27Z

crates/circuit/src/dag_circuit.rs

+#[repr(transparent)]
+pub struct OperationIndex(NodeIndex);
+impl OperationIndex {
+    /// Brand a [NodeIndex] as referring to an operation.


The word "brand" feels a little weird IMO (I think of cattle). Can we pick something else? Perhaps "designate", "mark", or even "downcast"?

Yeah, whatever you like, really. "Mark" is nice and short, "downcast" is consistent with type theory and PyO3.

kevinhartman · 2025-01-21T22:29:47Z

crates/circuit/src/dag_circuit.rs

    }
 }

-#[derive(Hash, Eq, PartialEq, Clone, Debug)]
+#[derive(Hash, Eq, PartialEq, Clone, Copy, Debug)]


Nice! I think I forgot to add that when doing the Var stuff.

Honestly, that bit probably didn't need to be in this same PR - I think I did it because one of the code collapses became a lot neater if Wire was copy, but really I could have done that separately.

kevinhartman · 2025-01-21T22:40:27Z

crates/circuit/src/dag_circuit.rs

+    #[inline]
+    pub fn get_operation(&self, index: NodeIndex) -> Option<(OperationIndex, &PackedInstruction)> {
+        match self.dag.node_weight(index) {
+            Some(NodeType::Operation(inst)) => Some((OperationIndex(index), inst)),


Should we put OperationIndex into its own inner module to force DAGCircuit implementors to also reason about using the unsafe OperationIndex::new?

Oh yeah, I think that's a really good idea.

Let's hold this PR for a sec, then, and I'll push up another one that makes qiskit_circuit::dag and moves the relevant existing files into that, since this change would mean we'll be up to four files that are specific to the DAG (dag_node.rs, dag_circuit.rs, rustworkx_core_vnext.rs and the new index file), and we can start making the imports in qiskit_accelerate a little cleaner too.

Ok, that PR is #13721, so this PR can wait for that one.

jakelishman added Changelog: None Rust labels Jan 20, 2025

jakelishman requested a review from a team as a code owner January 20, 2025 18:36

kevinhartman reviewed Jan 21, 2025

View reviewed changes

jakelishman mentioned this pull request Jan 22, 2025

Create qiskit_circuit::dag submodule #13721

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Brand operation indices with `OperationIndex` #13696

Brand operation indices with `OperationIndex` #13696

jakelishman commented Jan 20, 2025

qiskit-bot commented Jan 20, 2025

coveralls commented Jan 20, 2025

kevinhartman left a comment

kevinhartman Jan 21, 2025

jakelishman Jan 21, 2025

jakelishman Jan 21, 2025

kevinhartman Jan 21, 2025

jakelishman Jan 21, 2025

kevinhartman Jan 21, 2025

jakelishman Jan 21, 2025

kevinhartman Jan 21, 2025

jakelishman Jan 21, 2025

jakelishman Jan 22, 2025

Brand operation indices with OperationIndex #13696

Are you sure you want to change the base?

Brand operation indices with OperationIndex #13696

Conversation

jakelishman commented Jan 20, 2025

Summary

Details and comments

qiskit-bot commented Jan 20, 2025

coveralls commented Jan 20, 2025

Pull Request Test Coverage Report for Build 12873983166

Details

💛 - Coveralls

kevinhartman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Brand operation indices with `OperationIndex` #13696

Brand operation indices with `OperationIndex` #13696