[Stretch] Add `stretch` variable support to `QuantumCircuit`. by kevinhartman · Pull Request #13852 · Qiskit/qiskit

kevinhartman · 2025-02-16T22:17:13Z

Summary

Adds support for adding new stretch variables to a circuit via QuantumCircuit::add_stretch. The returned variable follows the same scoping rules as other classical circuit variables. For now, we don't support introducing a lower bound when declaring new stretch variables, but this can be added in the future.

Details and comments

~~Based on #13850. See the readable diff here.~~

Stretch variables are represented with a new expression node, Stretch. It's type is always Duration, and it is always a constant expression.
Stretch variables are tracked separately from classical run-time variables.
Stretches are not allowed as QuantumCircuit "input" variables.
We can now declare stretch variables and have those be emitted to QASM, but we won't be able to do anything with them until Delay and Box start using them in their duration expressions.

To-do

Release note.

This reverts commit e2b3221.

Types that have some natural order no longer have an ordering when one of them is strictly greater but has an incompatible const-ness (i.e. when the greater type is const but the other type is not).

We need to reject types with const=True in QPY until it supports them. For now, I've also made the Index and shift operator constructors lift their RHS to the same const-ness as the target to make it less likely that existing users of expr run into issues when serializing to older QPY versions.

This is probably a better default in general, since we don't really have much use for const except for timing stuff.

Since we're going for using a Cast node when const-ness differs, this will be fine.

I wasn't going to have this, but since we have DANGEROUS Float => Int, and we have Int => Bool, I think this makes the most sense.

A Stretch can always represent a Duration (it's just an expression without any unresolved stretch variables, in this case), so we allow implicit conversion from Duration => Stretch. The reason for a separate Duration type is to support things like Duration / Duration => Float. This is not valid for stretches in OpenQASM (to my knowledge).

Also adds support to expr.lift to create a value expression of type types.Duration from an instance of qiskit.circuit.Duration.

jakelishman

Thanks for all this work - I know quite a lot of it is tedious duplication of Var-based stuff.

I'm not convinced I'd have spotted if there's stretch handling missing from anywhere, but I think that's par for the course for reviews like this.

One more point I didn't spot in the review (but maybe overlooked): could you add a test that QuantumCircuit equality requires the stretches to be declared in the same order?

jakelishman · 2025-03-05T16:54:14Z

+        if self.vars_info.contains_key(&var_name) {
+            return Err(DAGCircuitError::new_err(
+                "cannot add var as its name shadows an existing var",
+            ));
+        }
+        if let Some(previous) = self.declared_stretches.get(&var_name) {
+            if var.eq(previous)? {
+                return Err(DAGCircuitError::new_err("already present in the circuit"));
+            }
+            return Err(DAGCircuitError::new_err(
+                "cannot add var as its name shadows an existing var",
+            ));
+        }
+        if let Some(previous) = self.captured_stretches.get(&var_name) {
+            if var.eq(previous)? {
+                return Err(DAGCircuitError::new_err("already present in the circuit"));
+            }
+            return Err(DAGCircuitError::new_err(
+                "cannot add var as its name shadows an existing var",
+            ));
+        }


I think the "input var" should be referred to as stretch, and the "existing var" would now be an "existing identifier"? Should add_var have its terminology in the error messages updated too?

jakelishman · 2025-03-05T18:30:41Z

+    var: uuid.UUID
+    """A :class:`~uuid.UUID` to uniquely identify this stretch."""
+    name: str | None
+    """The name of the stretch variable."""


Two minor points:

would var be better named as uid? Var originally called its inner field var because the inner object was a Clbit or ClassicalRegister, then the field got co-opted to mean something more like "l-value ID". Stretch isn't wrapping anything else, so we might be better jumping straight to uid-based naming? (Strictly, we don't need the uid to be universally unique, just unique among stretches, so we might not want to hard commit to saying it's universally unique.)

name must always be set and be a str for Stretch, I think? name: None was for implicit Var nodes that wrap Clbit, etc.

jakelishman · 2025-03-05T18:33:04Z

+    def __getstate__(self):
+        return (self.var, self.name)
+
+    def __setstate__(self, state):
+        var, name = state
+        super().__setattr__("type", types.Duration())
+        super().__setattr__("const", True)
+        super().__setattr__("var", var)
+        super().__setattr__("name", name)


Potentially we should just swap to a __getnewargs__ form, but I'm assuming that's pre-existing based on how Var was written? Let's do both at once (in a different PR post 2.0), if so.

jakelishman · 2025-03-05T21:22:28Z

+      # Use the stretches as Delay duration.
+      qc.delay(a, [0, 1])
+      qc.delay(b, 2)
+      qc.delay(c, [3, 4])
+      qc.barrier()


Strictly I think this PR doesn't add this bit, right?

Correct. It'll come in the next PR.

jakelishman · 2025-03-05T21:22:39Z

+      qc.barrier()
+
+    For additional context and examples, refer to the 
+    `QASM language specification. <https://openqasm.com/language/delays.html#duration-and-stretch-types>`__


Suggested change

`QASM language specification. <https://openqasm.com/language/delays.html#duration-and-stretch-types>`__

`OpenQASM 3 language specification. <https://openqasm.com/language/delays.html#duration-and-stretch-types>`__

jakelishman · 2025-03-05T21:23:04Z

+    use :meth:`.QuantumCircuit.add_stretch`. The resulting expression is a constant
+    expression of type :class:`~.types.Duration`, which can currently be used as the ``duration``
+    argument of a :meth:`~.QuantumCircuit.delay` or :meth:`~.QuantumCircuit.box` instruction.


It probably won't be able to be used by box in time for 2.0, so likely best drop that line.

jakelishman · 2025-03-05T21:41:01Z

        copied = qc.copy_empty_like("copy")
        self.assertEqual(copied.name, "copy")

+    # pylint: disable=invalid-name


I think this comment was supposed to go inside the function so it scopes to only that, but I also don't care because I don't like this rule haha.

jakelishman · 2025-03-05T21:42:40Z

        copied.add_var(d, 0xFF)
        self.assertEqual({a, b}, set(copied.iter_input_vars()))
        self.assertEqual({c, d}, set(copied.iter_declared_vars()))
+        self.assertEqual({e}, set(copied.iter_declared_stretches()))
        self.assertEqual({a}, set(qc.iter_input_vars()))
        self.assertEqual({c}, set(qc.iter_declared_vars()))
+        self.assertEqual({e}, set(qc.iter_declared_stretches()))



iirc, the spirit of this test is to copied.add_stretch(f) and to assert that {e, f} is in copied, while qc is still {e} (to catch the case of a shared reference to a Python object).

That said, we then seem to repeat functionally the same test another 20ish times, so probably not a big deal lol

jakelishman

I'm fine leaving Stretch.var as var and not changing to uid - if nothing else it helps duck-typing over Stretch and Var, and they are used in similar contexts sometimes.

Last comment is about the equality assertions, but it's fine - let's just leave it til a follow up. At the moment we'll false negative some assertions, but we false negative loads of things in QuantumCircuit comparisons, so it's not the biggest deal ever (might matter for control-flow builders one day, so we ought to fix it, but still).

jakelishman · 2025-03-06T00:51:08Z

+        qc1 = QuantumCircuit(captures=[a, b, c])
+        self.assertEqual(qc1, QuantumCircuit(captures=[a, b, c]))
+        self.assertNotEqual(qc1, QuantumCircuit(captures=[c, b, a]))


Actually this particular one should compare equal in both cases - captures are orderless, it's just declarations for which the order matters (because captures don't introduce a new degree of freedom).

So in this test they should both be equal, but

qc1 = QuantumCircuit() qc1.add_stretch(a) qc1.add_stretch(b) qc2 = QuantumCircuit() qc2.add_stretch(b) qc2.add_stretch(a)

would have qc1 != qc2.

That said, at this point I'm fine leaving this til a follow-up anyway, though let's note to do it.

Added to my list, I'll sort it out in #13853

If there's no time, it can wait til post 2.0 - it's a valid bugfix.

jakelishman

Re-approving, assuming I didn't bork the merge conflict. (Just trying to get this in the queue before I got to bed.)

jakelishman

Thanks for the QPY docs, and for pointing out I'd missed them haha!

ElePT

LGTM!

kevinhartman added 30 commits February 7, 2025 16:19

WIP

af4ba52

Add try_const to lift.

e423bf9

Try multiple singletons, new one for const.

0a5917b

Revert "Try multiple singletons, new one for const."

a97434d

This reverts commit e2b3221.

Remove Bool singleton test.

1afc965

Add const handling for stores, fix test bugs.

86655f1

Fix formatting.

aaeae9b

Remove Duration and Stretch for now.

a2a444b

Cleanup, fix const bug in index.

8ac2dc3

Fix ordering issue for types with differing const-ness.

9f8313c

Types that have some natural order no longer have an ordering when one of them is strictly greater but has an incompatible const-ness (i.e. when the greater type is const but the other type is not).

Make expr.Lift default to non-const.

71b7e7a

This is probably a better default in general, since we don't really have much use for const except for timing stuff.

Revert to old test_expr_constructors.py.

2091557

Make binary_logical lift independent again.

7307be9

Since we're going for using a Cast node when const-ness differs, this will be fine.

Update tests, handle a few edge cases.

9b30284

Fix docstring.

ce1faf1

Remove now redundant arg from tests.

4fee48f

Add const testing for ordering.

88ab046

Add const tests for shifts.

c5a230f

Add release note.

7c88d88

Add const store tests.

c58a7b8

Address lint, minor cleanup.

d9e9a8c

Add Float type to classical expressions.

4a56150

Allow DANGEROUS conversion from Float to Bool.

23b5961

I wasn't going to have this, but since we have DANGEROUS Float => Int, and we have Int => Bool, I think this makes the most sense.

Test Float ordering.

8bf2e4f

Improve error messages for using Float with logical operators.

111eb32

Float tests for constructors.

a839d51

Add release note.

a19b39a

Add Duration type to qiskit.circuit.

55af327

Also adds support to expr.lift to create a value expression of type types.Duration from an instance of qiskit.circuit.Duration.

Add negative test for QPY stretch expr.

320b72d

mtreinish added the priority: high label Mar 4, 2025

Merge branch 'main' of github.com:Qiskit/qiskit into circuit-stretch

c66842b

kevinhartman added Changelog: Added Add an "Added" entry in the GitHub Release changelog. mod: circuit Related to the core of the `QuantumCircuit` class or the circuit library and removed on hold Can not fix yet labels Mar 5, 2025

kevinhartman commented Mar 5, 2025

View reviewed changes

Comment thread crates/circuit/src/dag_circuit.rs

Add more circuit testing.

827e619

jakelishman reviewed Mar 5, 2025

View reviewed changes

kevinhartman added 5 commits March 5, 2025 17:34

Add circuit equality testing for stretch.

4d7129e

Refer to 'stretches' instead of 'stretch variables'.

9d82809

Remove | None for Stretch.name

e84b48e

Address review comments.

62cf3ee

Merge branch 'main' of github.com:Qiskit/qiskit into circuit-stretch

e2044e6

jakelishman previously approved these changes Mar 6, 2025

View reviewed changes

jakelishman dismissed their stale review via aad2200 March 6, 2025 00:58

Update QPY desc.

7634b3e

jakelishman previously approved these changes Mar 6, 2025

View reviewed changes

kevinhartman dismissed jakelishman’s stale review via 7634b3e March 6, 2025 01:00

kevinhartman force-pushed the circuit-stretch branch from aad2200 to 7634b3e Compare March 6, 2025 01:00

Fix num_identifiers.

2223d12

jakelishman previously approved these changes Mar 6, 2025

View reviewed changes

jakelishman enabled auto-merge March 6, 2025 01:08

kevinhartman added 2 commits March 5, 2025 21:11

Merge branch 'main' of github.com:Qiskit/qiskit into circuit-stretch

823dd27

Fix merge for control flow tests.

a5ccc7b

kevinhartman dismissed jakelishman’s stale review via a5ccc7b March 6, 2025 02:30

ElePT approved these changes Mar 6, 2025

View reviewed changes

jakelishman added this pull request to the merge queue Mar 6, 2025

Merged via the queue into Qiskit:main with commit f82689f Mar 6, 2025

	`QASM language specification. <https://openqasm.com/language/delays.html#duration-and-stretch-types>`__
	`OpenQASM 3 language specification. <https://openqasm.com/language/delays.html#duration-and-stretch-types>`__

Conversation

kevinhartman commented Feb 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Details and comments

To-do

Uh oh!

Uh oh!

jakelishman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakelishman Mar 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakelishman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakelishman left a comment

Choose a reason for hiding this comment

Uh oh!

jakelishman left a comment

Choose a reason for hiding this comment

Uh oh!

ElePT left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

kevinhartman commented Feb 16, 2025 •

edited

Loading

jakelishman Mar 5, 2025 •

edited

Loading