Unify identifer handling of `Var` and `Stretch` in `DAGCircuit` #14000

kevinhartman · 2025-03-11T21:57:49Z

Summary

During testing, I've found two bugs with DAGCircuit's handling of stretches:

They are fixed here.

Details and comments

I've also reworked DAGCircuit's tracking of stretches to be the same as what we do for vars. Namely, there's now just an identifier_info field (which replaces var_info) which makes it easier to detect name collisions between all identifiers in tracking, and also should make it easier to support native handling of stretches in the future.

To make things a bit cleaner, I've moved Var to lib.rs and defined a macro impl_circuit_identifier to reduce boilerplate between our Qubit, Clbit, Var, and now Stretch identifier types.

I've also moved the logic of pickling identifier info tracking from __getstate__ and __setstate__ to FromPyObject and IntoPyObject implementations.

To-do

It looks to me like we don't have any testing of pickling / unpickling a DAGCircuit. Can that possibly be true?

This is only really a user-facing error message when working with DAGCircuit, since QuantumCircuit first checks if the bits being added to it are duplicates. And, in the case of DAGCircuit, the previous error message was already unfriendly: ValueError: Existing bit ShareableQubit(Owned { register: OwningRegisterInfo { name: "q16", size: 2, subclass: QUBIT }, index: 0 }) cannot be re-added in strict mode.

Tracks stretches the same way we track vars. Also happens to fix a bug in DAG equality where order mattered between stretch captures (it should never have). And, fixes a serialization bug with stretches.

…ure-eq

qiskit-bot · 2025-03-11T21:57:54Z

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

@Qiskit/terra-core

coveralls · 2025-03-11T22:22:53Z

Pull Request Test Coverage Report for Build 14337982611

Details

206 of 231 (89.18%) changed or added relevant lines in 3 files are covered.
5 unchanged lines in 2 files lost coverage.
Overall coverage increased (+0.06%) to 88.218%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
crates/circuit/src/lib.rs	13	17	76.47%
crates/circuit/src/dag_circuit.rs	191	212	90.09%

Files with Coverage Reduction	New Missed Lines	%
crates/circuit/src/dag_circuit.rs	1	86.69%
crates/qasm2/src/lex.rs	4	91.73%

Totals
Change from base Build 14322076502:	0.06%
Covered Lines:	73590
Relevant Lines:	83418

💛 - Coveralls

mtreinish · 2025-03-12T12:14:38Z

It looks to me like we don't have any testing of pickling / unpickling a DAGCircuit. Can that possibly be true?

If we have any we get it indirectly through tests that are using multiprocessing (like transpiling > 1 circuit) because pickle will be used for IPC or deep copy which also internally uses pickle. That being said see #13980 for some explicit tests I had to add for another pickle bug that's been around since 1.3 (when we ported DAGCircuit to rust).

Also fix lint.

…ure-eq

kevinhartman · 2025-03-12T19:12:33Z

see #13980 for some explicit tests I had to add for another pickle bug that's been around since 1.3 (when we ported DAGCircuit to rust).

Great, thanks! I've added a few tests based on those here to validate that stretches are preserved after pickling and deepcopy.

I also tested vars locally, but I found that there's a bug there that predates 2.0 so those tests aren't added in this PR (we ought to address that separately):

DAGCircuit with vars does not compare equal after pickling #14007

mtreinish

I know you've got a pending rebase coming so let me just leave my comments from part way through the review and I'll look again after the rebase is up. That being said I'm wondering if we should concentrate on a dedicated 2.0 fix in a direct PR on stable/2.0 and leave the larger changes in this PR for just 2.1?

crates/circuit/src/dag_circuit.rs

mtreinish · 2025-03-27T19:08:13Z

crates/circuit/src/dag_circuit.rs

@@ -2206,8 +2269,8 @@ impl DAGCircuit {
    /// but was changed by issue #2564 to return number of qubits + clbits
    /// with the new function DAGCircuit.num_qubits replacing the former
    /// semantic of DAGCircuit.width().
-    fn width(&self) -> usize {
-        self.qubits.len() + self.clbits.len() + self.vars_info.len()
+    fn width(&self, py: Python) -> usize {


I assume this is something that won't be required after #14068 is done?

Should be possible, though it's not clear to me if it should happen in that PR or a follow-up. That PR (so far) ports the Python Expr and Type hierarchies to Rust, but does not yet change over how we track things in the circuit.

Yeah as we discussed offline I think it's fine to do this in a follow up after #14068.

I'm just mildly concerned that this is adding a deeper dependency on Python for something we should be able to figure out structurally from the dag without Python.

Agreed. We do have this information in a way that doesn't need Python as it is, but it'd be inefficient since we'd have to walk over all of the DAGIdentifierInfos and count which ones are variables.

I will find some faster solution in a follow-up that ditches use of the Python token.

mtreinish · 2025-03-27T19:11:48Z

crates/circuit/src/dag_circuit.rs

+    ///     stretch: the stretch to add.
+    fn add_captured_stretch(&mut self, py: Python, stretch: &Bound<PyAny>) -> PyResult<()> {


This is actually a breaking api change right? Python's arguments by default are both positional and by name, and we didn't declare the signature of this pymethod to just deal with this positionally the name can be used to do dag.add_captured_stretch(var=stretch) which would be broken by this change. So I don't think we can do this change until 3.0 since we released 2.0.0rc1 and rc2 with a fixed API surface we're stuck with this name for now.

I was originally hoping this PR would land earlier, but I've renamed stretch to var in both signatures for now: 7bba590

mtreinish · 2025-03-27T19:12:24Z

crates/circuit/src/dag_circuit.rs

-        if let Some(previous) = self.declared_stretches.get(&var_name) {
-            if var.eq(previous)? {
+    ///     stretch: the stretch to add.
+    fn add_declared_stretch(&mut self, py: Python, stretch: &Bound<PyAny>) -> PyResult<()> {


This is the same problem with this being a breaking change

mtreinish · 2025-03-27T19:13:52Z

crates/circuit/src/dag_circuit.rs

+    #[getter]
+    fn num_stretches(&self, py: Python) -> usize {
+        self.num_captured_stretches(py) + self.num_declared_stretches(py)
+    }
+
+    /// Number of captured stretches tracked by the circuit.
+    #[getter]
+    fn num_captured_stretches(&self, py: Python) -> usize {
+        self.captured_stretches.bind(py).len()
+    }
+
+    /// Number of declared local stretches tracked by the circuit.
+    #[getter]
+    fn num_declared_stretches(&self, py: Python) -> usize {
+        self.declared_stretches.bind(py).len()
+    }


If this is for 2.0 I don't think we can backport this since these are new attributes for the dag. This would only be for 2.1. They make total sense to add, but we might want to split this PR out into multiple parts.

Makes sense. I've removed the backport label and will add a release note for this PR for 2.1.

…ure-eq

kevinhartman · 2025-03-27T19:51:06Z

@mtreinish

That being said I'm wondering if we should concentrate on a dedicated 2.0 fix in a direct PR on stable/2.0 and leave the larger changes in this PR for just 2.1?

That sounds good. I've removed the backport label from this PR and will open a new one against stable/2.0 that takes the tests from here and fixes things via other means.

crates/circuit/src/dag_circuit.rs

releasenotes/notes/dag-stretch-counters-c26f39e3ea9b85a5.yaml

crates/circuit/src/dag_circuit.rs

mtreinish · 2025-04-01T20:17:00Z

crates/circuit/src/dag_circuit.rs

@@ -2206,8 +2269,8 @@ impl DAGCircuit {
    /// but was changed by issue #2564 to return number of qubits + clbits
    /// with the new function DAGCircuit.num_qubits replacing the former
    /// semantic of DAGCircuit.width().
-    fn width(&self) -> usize {
-        self.qubits.len() + self.clbits.len() + self.vars_info.len()
+    fn width(&self, py: Python) -> usize {


Yeah as we discussed offline I think it's fine to do this in a follow up after #14068.

I'm just mildly concerned that this is adding a deeper dependency on Python for something we should be able to figure out structurally from the dag without Python.

…ure-eq

mtreinish · 2025-04-08T20:21:17Z

crates/circuit/src/dag_circuit.rs

+    fn from_pickle(ob: &Bound<PyAny>) -> PyResult<Self> {
+        let val_tuple = ob.downcast::<PyTuple>()?;


Is there a reason to not just do?:

Suggested change

fn from_pickle(ob: &Bound<PyAny>) -> PyResult<Self> {

let val_tuple = ob.downcast::<PyTuple>()?;

fn from_pickle(val_tuple: &Bound<PyTuple>) -> PyResult<Self> {

I think just to be consistent with the other from_pickle methods, really. There isn't any auto-extraction possible here anyway, since we store this all in a PyDict which, when iterated over, yields PyAny pairs.

mtreinish · 2025-04-08T20:29:42Z

crates/circuit/src/lib.rs

+pub struct Var(u32);
+
+#[derive(Copy, Clone, Debug, Hash, Eq, PartialEq)]
+pub struct Stretch(u32);


Is there a reason to not make the inner u32 pub here?

I didn't make it public since it doesn't have real meaning, at least compared to the int of a Qubit or Clbit.

mtreinish · 2025-04-08T20:30:02Z

crates/circuit/src/lib.rs

@@ -37,53 +37,60 @@ use pyo3::prelude::*;
 use pyo3::types::{PySequence, PyTuple};
 use pyo3::PyTypeInfo;

-pub type BitType = u32;


Is the reason to remove this because it's used for more than bits now?

Pretty much, yeah.

I'd introduced it originally, but it doesn't feel very Rust-y. I think it wasn't doing much for us.

mtreinish · 2025-04-08T20:30:31Z

test/python/dagcircuit/test_dagcircuit.py

@@ -10,6 +10,8 @@
 # copyright notice, and modified files need to carry a notice indicating
 # that they have been altered from the originals.

+# pylint: disable=invalid-name


What's the invalid name that pylint was complaining about?

This is, I believe, because of the additional letter-only names I used. Like "c" and "d".

There were already letter-only names, so I figured this should be allowed. Let me know if you have other suggestions :)

kevinhartman added 11 commits March 10, 2025 12:14

s

6e519ec

Rename BitData to ObjectRegistry.

8a98dc6

s

4d5fbec

Fix comment in commutation checker.

f04aa81

Fix lint.

6d1f9f8

Address review comments.

39ed097

Unify identifier handling in DAG.

d118c44

Tracks stretches the same way we track vars. Also happens to fix a bug in DAG equality where order mattered between stretch captures (it should never have). And, fixes a serialization bug with stretches.

Fix tests.

4b6f400

Improve testing and fix bug.

46f1a36

Merge branch 'main' of github.com:Qiskit/qiskit into fix-stretch-capt…

d916fec

…ure-eq

kevinhartman added the Changelog: None Do not include in changelog label Mar 11, 2025

kevinhartman added this to the 2.0.0 milestone Mar 11, 2025

kevinhartman self-assigned this Mar 11, 2025

kevinhartman requested a review from a team as a code owner March 11, 2025 21:57

This was linked to issues Mar 11, 2025

DAGCircuit does not properly pickle stretches #13999

Closed

Stretch capture order should not matter in circuit equality #13998

Closed

mtreinish added the stable backport potential The bug might be minimal and/or import enough to be port to stable label Mar 11, 2025

kevinhartman added 2 commits March 12, 2025 15:07

Add pickle and deepcopy tests for stretches.

38fb72f

Also fix lint.

Merge branch 'main' of github.com:Qiskit/qiskit into fix-stretch-capt…

1813701

…ure-eq

mtreinish reviewed Mar 27, 2025

View reviewed changes

kevinhartman added 2 commits March 27, 2025 15:28

Merge branch 'main' of github.com:Qiskit/qiskit into fix-stretch-capt…

0c9c271

…ure-eq

Undo breaking name change.

7bba590

kevinhartman removed the stable backport potential The bug might be minimal and/or import enough to be port to stable label Mar 27, 2025

Add release note.

af4da55

kevinhartman added Changelog: New Feature Include in the "Added" section of the changelog mod: transpiler Issues and PRs related to Transpiler labels Mar 27, 2025

mtreinish modified the milestones: 2.0.0, 2.1.0 Mar 27, 2025

kevinhartman mentioned this pull request Mar 27, 2025

[Backport] Fix equality and pickling of DAGCircuit with stretches #14114

Merged

kevinhartman added 2 commits March 27, 2025 17:16

Fix lint.

2fab627

Fix Python lint.

6a852d9

kevinhartman changed the title ~~Fix equality and pickling of DAGCircuit with stretches~~ Unify identifer handling of Var and Stretch in DAGCircuit Apr 1, 2025

mtreinish reviewed Apr 1, 2025

View reviewed changes

Address review comments.

02cbc96

kevinhartman requested a review from mtreinish April 3, 2025 21:27

kevinhartman mentioned this pull request Apr 4, 2025

Oxidize classical expression use in Rust code. #14176

Open

Merge branch 'main' of github.com:Qiskit/qiskit into fix-stretch-capt…

eec52e5

…ure-eq

mtreinish reviewed Apr 8, 2025

View reviewed changes

kevinhartman requested a review from mtreinish April 10, 2025 18:37

mtreinish approved these changes Apr 10, 2025

View reviewed changes

mtreinish added this pull request to the merge queue Apr 10, 2025

Merged via the queue into Qiskit:main with commit 8ec4c9c Apr 10, 2025
24 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify identifer handling of `Var` and `Stretch` in `DAGCircuit` #14000

Unify identifer handling of `Var` and `Stretch` in `DAGCircuit` #14000

kevinhartman commented Mar 11, 2025 •

edited

Loading

qiskit-bot commented Mar 11, 2025

coveralls commented Mar 11, 2025 •

edited

Loading

mtreinish commented Mar 12, 2025

kevinhartman commented Mar 12, 2025 •

edited

Loading

mtreinish left a comment

mtreinish Mar 27, 2025

kevinhartman Mar 27, 2025

mtreinish Apr 1, 2025

kevinhartman Apr 3, 2025

mtreinish Mar 27, 2025

kevinhartman Mar 27, 2025

mtreinish Mar 27, 2025

mtreinish Mar 27, 2025

kevinhartman Mar 27, 2025

kevinhartman commented Mar 27, 2025

mtreinish Apr 1, 2025

mtreinish Apr 8, 2025

kevinhartman Apr 10, 2025

mtreinish Apr 8, 2025

kevinhartman Apr 8, 2025

mtreinish Apr 8, 2025

kevinhartman Apr 8, 2025

mtreinish Apr 8, 2025

kevinhartman Apr 8, 2025

		/// stretch: the stretch to add.
		fn add_captured_stretch(&mut self, py: Python, stretch: &Bound<PyAny>) -> PyResult<()> {

		fn from_pickle(ob: &Bound<PyAny>) -> PyResult<Self> {
		let val_tuple = ob.downcast::<PyTuple>()?;

	fn from_pickle(ob: &Bound<PyAny>) -> PyResult<Self> {
	let val_tuple = ob.downcast::<PyTuple>()?;
	fn from_pickle(val_tuple: &Bound<PyTuple>) -> PyResult<Self> {

Unify identifer handling of Var and Stretch in DAGCircuit #14000

Unify identifer handling of Var and Stretch in DAGCircuit #14000

Conversation

kevinhartman commented Mar 11, 2025 • edited Loading

Summary

Details and comments

To-do

qiskit-bot commented Mar 11, 2025

coveralls commented Mar 11, 2025 • edited Loading

Pull Request Test Coverage Report for Build 14337982611

Details

💛 - Coveralls

mtreinish commented Mar 12, 2025

kevinhartman commented Mar 12, 2025 • edited Loading

mtreinish 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

kevinhartman commented Mar 27, 2025

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

Unify identifer handling of `Var` and `Stretch` in `DAGCircuit` #14000

Unify identifer handling of `Var` and `Stretch` in `DAGCircuit` #14000

kevinhartman commented Mar 11, 2025 •

edited

Loading

coveralls commented Mar 11, 2025 •

edited

Loading

kevinhartman commented Mar 12, 2025 •

edited

Loading