Introducing DynamicRange for ForLoop + Calibrations support for control flow + Update of OpenQASM3 Exporter #14277
Conversation
This PR introduces two changes aimed at complementing the compatibility of Qiskit 1.x with all the features proposed through custom calibrations with Qiskit Pulse, that have now been ported within control flow bodies to let the transpiler know of their existence. Also, the OpenQASM3 exporter, printer and AST have been slightly modified to support the latest OpenQASM3 specification of a for loop, that requires the specification of the looping variable type (openqasm3 == 1.0.1). Finally, we introduce a new feature for the ForLoopOp called DynamicRange, which enables the specification of ranges (start, stop, step), that can contain real-time variables and not only integer literals. As many control stacks enable the dynamic adjustment of variable, scenarios such as dynamic error amplification could motivate such feature. The OpenQASM3 exporter was also updated based on this new feature and is able to generate dynamic ranges (where original specifications ported it to generic Expressions and not only Literals). However, as Qiskit 1.x does not support real-time floating points, we constrain the classical type of the looping variable to be an IntType.
qiskit-bot
added
the
Community PR
|
Thank you for opening a new pull request. Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient. While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone. One or more of the following people are relevant to this code:
|
Updated indexset argument typehint to indicate the possible new DynamicRange specification
|
It seems that some tests are failing because of the deprecation warnings that are triggered due to Qiskit Pulse deprecation. Since every control flow test summons the calibrations attribute of QuantumCircuit, those tests will raise a warning. |
There was a problem hiding this comment.
Thanks very much for the interest. Unfortunately, Qiskit 1.4 is not open for new features, so we can accept fixing the calibration tracking alone as a bugfix on 1.x, and the new dynamic-range stuff as a feature on the 2.x branch, but we can't merge new features into the feature-frozen 1.x branch. You're welcome to maintain an out-of-tree fork adding it, though if you need both - the chance of conflicts as we backport other fixes to the 1.x series is rather low. We can accept suitable split PRs to the relevant branches.
I left a few comments throughout. An extra top-level one is that there's no testing of the calibrations bug fix at the moment - bugfix PRs typically should include a regression test.
| The step size is an integer or a variable that defines the increment | ||
| """ | ||
|
|
||
| def __init__(self, start: int | Var, stop: Optional[int | Var] = None, step: int | Var = 1): |
There was a problem hiding this comment.
It's probably easiest/most correct to have this take start, stop and step all as Expr, plus Optional wherever needed, and then type-check that the types are consistent and/or valid.
I'd also suggest that step is taken as None by default in that form.
|
|
||
| if TYPE_CHECKING: | ||
| from qiskit.circuit import QuantumCircuit | ||
|
|
||
|
|
||
| class DynamicRange: |
There was a problem hiding this comment.
Overall, I'd suggest that Range should be a sort of expression or object in the classical.expr module, because the concept is more general than "for loops", even though that's the only place you're adding support for.
We're not intending to have wider support any time in the near term, but in principle the concept of a "range" is not limited to for-loops - it's also relevant for the Index expression.
| for value, name in [(start, "start"), (stop, "stop"), (step, "step")]: | ||
| if not isinstance(value, (int, Var)): | ||
| raise CircuitError( | ||
| f"DynamicRange {name} value must be an int or Var, but got {type(value)}." | ||
| ) |
There was a problem hiding this comment.
In general, it's best not to do this in the default constructor - in Python, the accepted types are documented in the method, and doing manual runtime type-checking slows down the happy paths for the vast majority of users, without being an adequate prevention of typing errors. I know we do it a lot in Qiskit, but it's a pattern we're trying to wind down, because in practice while each individual one is "not much", the whole effect is a library that's sluggish.
Certainly the expr.Expr subclass tree treats the classes as the "fast-path constructors", and leaves all the type-checking for the type-inferring/lifting function calls (where it is fine to include).
| "if_else", len(self.__resources.qubits), len(self.__resources.clbits), [], label=label | ||
| "if_else", | ||
| len(self.__resources.qubits), | ||
| len(self.__resources.clbits), | ||
| [], | ||
| label=label, |
There was a problem hiding this comment.
I think you might have your black set to default to too-narrow line length. Can you delete the trailing comma on label=label here (and everywhere else relevant) so the formatting goes back to what it was?
| body = scope.build(scope.qubits(), scope.clbits()) | ||
| body.calibrations = self._circuit.calibrations |
There was a problem hiding this comment.
Sharing a pointer to the calibrations data in loads of places is really likely to cause it to get out-of-sync in weird ways. Besides this, the calibrations dictionary doesn't mean the same thing in circuit scopes; the relative qubit indices are stored in the dictionary as part of the lookup, and those relative indices (in general) change when you go into a control-flow block.
If your concern is lookup of calibrations, I'd say that the answer for look-up is that things that both recursive into control-flow ops and care about calibrations should be passing down the top-level calibrations dictionary and handling it themselves.
If your concern is about causing QuantumCircuit.add_calibration to work while in a builder block, I'd mostly just suggest that the method should raise an exception if you attempt to set a "scoped" calibration. Calibrations are global and we shouldn't hide that.
There was a problem hiding this comment.
I thought the qubit indices present in the calibrations represented the physical qubits on which, after transpilation with a trivial layout pass, the calibrations would be considered available. The concern was about the fact that when passing through a transpiler pass, the basis translator would wrongly decompose the gates within the body of the control flow ops despite a calibration having been introduced at the wider level. That is why I thought of this referencing.
So I definitely care more about your point 1, so would you suggest I fix this in the transpiler directly?
| def test_for_loop_dynamic_range_instantiation(self): | ||
| """Verify creation and properties of a ForLoopOp using a dynamic range indexset.""" | ||
| body = QuantumCircuit(3, 1) | ||
| loop_parameter = Parameter("foo") | ||
| start = expr.Var.new("start", types.Uint(3)) | ||
| stop = expr.Var.new("stop", types.Uint(3)) | ||
| step = 1 | ||
| indexset = DynamicRange(start, stop, step) |
There was a problem hiding this comment.
We could do with a couple of more thorough tests that exercise more of the new code. In particular, the new OQ3 paths aren't tested right now.
| def _visit_sequence( | ||
| self, nodes: Sequence[ast.ASTNode], *, start: str = "", end: str = "", separator: str | ||
| self, | ||
| nodes: Sequence[ast.ASTNode], | ||
| *, | ||
| start: str = "", | ||
| end: str = "", | ||
| separator: str, | ||
| ) -> None: |
There was a problem hiding this comment.
Similar to other places - please can you undo the formatting changes?
| elif isinstance(indexset, DynamicRange): | ||
| # OpenQASM 3 uses inclusive ranges on both ends, unlike Python. | ||
| if isinstance(indexset.start, int): | ||
| start = self.build_integer(indexset.start) | ||
| else: | ||
| start = self.build_expression(indexset.start) | ||
| if isinstance(indexset.stop, int): | ||
| stop = self.build_integer(indexset.stop - 1) | ||
| else: | ||
| stop = self.build_expression(indexset.stop) | ||
| if isinstance(indexset.step, int): | ||
| step = self.build_integer(indexset.step) if indexset.step != 1 else None | ||
| else: | ||
| step = self.build_expression(indexset.step) | ||
| indexset_ast = ast.Range(start=start, end=stop, step=step) |
There was a problem hiding this comment.
Assuming we do pivot to DynamicRange getting called expr.Range and taking all its arguments as type-checked expr.Expr values, this would all just simplify to start = None if start is None else self.build_expression(start) etc.
In general, the library is privileged and allowed to use the private That said, I left a comment in the review above around how I don't think that sharing the |
Summary
This PR introduces several improvements to enhance the compatibility of Qiskit 1.x with dynamic quantum-classical workflows, particularly in the context of control flow, pulse-level calibrations, and OpenQASM3 export.
Highlights
• Control-Flow Compatible Calibrations
Custom calibrations defined with Qiskit Pulse are now fully supported within control flow bodies (if_else, for_loop, while_loop, switch_case). This enables correct visibility of these calibrated gates by the transpiler, aligning with the hybrid execution model enabled by Qiskit 1.x.
✅ Closes #13728
• OpenQASM3 for Loop Compliance
The QASM3 exporter, printer, and AST have been updated to match the latest OpenQASM3 specification (v1.0.1), which now requires explicit typing of loop variables in for constructs.
✅ Closes #13725
• Dynamic Range Support for ForLoopOp
A new feature, DynamicRange, has been added to ForLoopOp, allowing loop bounds (start, stop, step) to include runtime variables or classical register references instead of just static literals. This enables flexible use cases such as dynamic error amplification.
While the exporter supports dynamic expressions, Qiskit 1.x currently limits loop variables to IntType due to the lack of real-time floating point support. The OpenQASM3 Exporter has been updated accordingly to handle this new DynamicRange object.
✅ Closes #13729