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:
|
arthurostrauss
force-pushed
the
dynamic_range_expr
branch
from
00e0de0 to
23f62bf
Compare
Pull Request Test Coverage Report for Build 20016330999Details
💛 - Coveralls |
arthurostrauss
force-pushed
the
dynamic_range_expr
branch
from
74d48b7 to
957f37f
Compare
arthurostrauss
force-pushed
the
dynamic_range_expr
branch
from
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: New Feature
|
@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. |
arthurostrauss
force-pushed
the
dynamic_range_expr
branch
from
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.
| return false; | ||
| } | ||
| } | ||
| (ExprRef::Range(a), ExprRef::Range(b)) => { |
There was a problem hiding this comment.
I didn't fully understand what this function does - and so, whether we should also compare the values of start and stop (and step).
There was a problem hiding this comment.
It recursively goes into the equivalence checks of the components of the Range, which are defined in other modules. I did not pay too much attention of what it was checking under the hood
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)
arthurostrauss
force-pushed
the
dynamic_range_expr
branch
from
7610270 to
435e6e6
Compare
| self.assertEqual(range_expr.type, types.Uint(8)) | ||
| self.assertTrue(range_expr.const) | ||
|
|
||
| def test_range_with_float_values(self): |
There was a problem hiding this comment.
I think this should just check start, just like the next test
There was a problem hiding this comment.
Not sure to understand that either. Which tests are you referring to?
There was a problem hiding this comment.
I don't think start should be the only one tested, because we want to test the type casting of the step when not explicitly provided.
|
I think there are still some unaddressed comments from @gadial |
1ucian0
moved this from Waiting for maintainer response
to Blocked by another issue
in Contributor Monitoring
|
There are some high-priority changes on control flow coming that mostly probably will conflict with this PR. So probably this PR wont make it in 2.3. Bumping it for 2.4. |
Extends the circuit representation to support range objects (both `expr.Range` and Python `range`) as index sets for for-loops, in addition to lists of integers. This change ensures that for-loops can iterate over dynamically-sized ranges defined by `expr.Range`, preserving the Python `range` object when provided as input. This allows users to use Python ranges as loop iterators instead of just lists of indices. The length of `expr.Range` objects can only be determined at runtime, a fallback mechanism is added to handle this case gracefully. Also introduces a dedicated enum `ForLoopIndexSet` to encapsulate the different types of index sets.
|
The latest commits aim to solve both the integration issue related to #14568 and to address the issue #15380 by doing a careful check of the Python range in Rust. The latter check is done by inspecting the name of the type, which might not appear robust at first glance if the Python range is bound to change its name but it could probably pass as a temporary solution to recover the expected behavior. Elsewhere, the indexset check is relaxed in such a way that one can either recover access to Python range or the expr.Range expression done by this PR. |
Enhances the visualization of `ForLoopOp` nodes in the circuit drawer. It now correctly displays the range parameters (start, stop, step) when the index set is an `expr.Range` object, extracting and displaying their values or names. This provides a clearer and more informative representation of loop indices in circuit diagrams, especially when using dynamic ranges.
|
@1ucian0, as this PR got updated with the recently merged internal change for control flow, and that it furthermore intends to solve another supported behavior for Python ranges, could this PR be reviewed in the expectation to be included in the next release? Thanks |
Streamlines the formatting of range expressions displayed in circuit visualizations, improving readability. Removes redundant conditional logic and simplifies the construction of the range string.
Refactors the logic for displaying range objects within circuit visualizations. Simplifies the handling of `range` and `IndexSet` objects to provide a more consistent and readable representation, especially when dealing with step values. The changes enhance the display of dynamic ranges to avoid fallback to string representations of the `IndexSet`.
Updated ForLoopIndexSet to match latest ForCollection definition
Addresses comments to reduce unnecessary cloning during the creation of range expressions. This change reduces clone statements in the core range.rs logic. It also introduces getter methods that are now used in the `__repr__` method to further reduce unnecessary cloning.
|
This PR has been updated with the latest modifications brought by #15404 and complies with the recent Rust porting done by #14568. It also addressed the remaining comments of @gadial on implementation details of the new range.rs file. All tests are passing and build on top of the previous changes of control-flow that were planned for Qiskit 2.3. @1ucian0, @jakelishman, it is ready for a last review round and could be reintegrated to the 2.3 milestone unless there are critical reasons not to do it (e.g. anything else on control-flow revamping that needs to be prioritized over adding this). To comply with the current model of ForCollection, new methods called is_empty() and len() have been implemented in the Range object, and return by default False and 1 as both those methods would not be relevant at compile time. |
6 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).