New classical expr.Range specification - #14618
Conversation
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:
|
00e0de0 to
23f62bf
Compare
Coverage Report for CI Build 28173681343Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage increased (+0.03%) to 87.671%Details
Uncovered Changes
Coverage Regressions7 previously-covered lines in 3 files lost coverage.
Coverage Stats
💛 - Coveralls |
74d48b7 to
957f37f
Compare
88057d9 to
e7b47f2
Compare
|
@1ucian0, the current tests that are failing have a priori no connection with the implemented features of this PR: |
|
Seems to be related to #14888 |
1ucian0
left a comment
1ucian0
left a comment
There was a problem hiding this comment.
Thanks Arthur! I took a look to the python part and added few comments.
1ucian0
added
the
Changelog: Added
|
@1ucian0, ready for another review round. Not too sure what is going on the docs side for the tests. |
it was indeed. solved now. |
|
@1ucian0 |
It seems to be related with an issue in the release notes. |
It was indeed, I found an alternative phrasing to make it pass. Let me know what you think. |
|
Still waiting for another review round on this PR. Thank you. |
a979634 to
ae41d4b
Compare
gadial
left a comment
gadial
left a comment
There was a problem hiding this comment.
Overall looks very good. I left specific remarks and questions about things I did not understand.
My main concern is the way step is handled. I don't see the need to allow it to be None and why unwrap is used every time it's accessed, which might lead to panic.
gadial
left a comment
gadial
left a comment
There was a problem hiding this comment.
The changes look good to me, I approve the part I've looked at (the rust code)
7610270 to
435e6e6
Compare
|
I think there are still some unaddressed comments from @gadial |
Conform expr.Range / UnrollForLoops work to upstream Qiskit#16273's input-var loop counter (re-inferred in QPY), drop the branch's 'w' LOOP_VARIABLE element, relax validation so range+Var is allowed, and fix the pyo3-0.29 subclass-init deprecation. Known limitation (Var-counter body cannot capture outer vars) deferred to a follow-up PR.
… Var loop param on QPY read When substituting variables in a ForLoopOp, exclude the loop's parameter (the body's input variable) from substitutions so the body's input var isn't removed or mismatched with the ForLoopOp.loop_parameter. This prevents accidental capture/removal of the loop counter during substitution. Also adjust QPY reading to handle ForLoopOp entries where the loop-parameter slot is serialized as Null for runtime expr.Var counters: if the body circuit has exactly one input variable, treat that as the loop parameter (matching the data model and the Rust reader behavior). Update tests and release notes to reflect the loop-parameter semantics (Var loop counters are the body's input variable and are re-inferred on QPY read). No public API signature changes, but this behavior change affects substitution semantics and QPY deserialization for ForLoopOp.
5ea0e7e to
a8c1e06
Compare
|
@gadial I have updated the PR to conform to your recent support for LoopParam. One use case that is currently not working and that was flagged by @jakelishman in your PR is that there is currently no way in this new data model that has been introduced to insert an I however decided to leave the substitute_vars within the scope of this PR as it directly unfolds the use case of the |
…/dynamic ranges Change the ForCollection::List variant from Vec to Vec so list-based loop collections can represent negative indices or other signed integer ranges. This enables expressing dynamic/range-like collections that include negative values. Breaking change: the ForCollection::List type in the public API now contains Vec instead of Vec; callers and pattern matches must be updated accordingly.
Clean up test/python/qpy/test_roundtrip.py by removing duplicated manual write/read sequences and related assertions that duplicated what assert_roundtrip_equal already verifies. Also remove an unused QpyError import and correct a docstring to more precisely describe the limitation on dumping expr.Range for-loops below QPY v18. This is a test-only change (no runtime/API behavior change). It reduces noise and duplication in the tests and makes expectations rely on the central assert_roundtrip_equal helper.
…nd consolidate roundtrip tests Introduce assert_dump_raises_unsupported to centralize checks that dumping a circuit with a given writer/version raises UnsupportedFeatureForVersion. Replace duplicated manual write_circuit/io.BytesIO blocks with calls to this helper across QPY roundtrip tests, and adjust writer iteration logic for versions that support the Rust writer. This is a test-only refactor that reduces duplication and improves clarity; it does not change runtime or public API behavior.
gadial
left a comment
gadial
left a comment
There was a problem hiding this comment.
I think this PR can be merged as part of the QPY18 update. Note the changes in the python QPY component are not required as python QPY does not support QPY18, but you can leave them.
7 participants
Summary
This PR introduces a new feature enabling the specification of Range objects that can contain real-time expressions for start, stop and step. This provides a Qiskit entrypoint for OpenQASM3 RangeDefinition specification, and opens new ways of performing dynamically evolving for loops that can now depend on a wider set of expressions than the compile time specification of index set.
✅ Closes #13725
✅ Closes #13729
Details and comments
The Range expression is now one of the core expressions available in the expr module, as suggested in the feedback of #14277. This specification (written in Rust to comply with other expr specifications) also involves the rewriting of the OpenQASM3 exporter to include special handling of this new object, and also updates the QIskit internal OpenQASM3 AST to comply with the latest AST of the official OpenQASM3 specification (notably by specifying the type of the looping variable).