qiskit
qiskit copied to clipboard
Qiskit
New classical expr.Range specification
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).
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:
- @Eric-Arellano
- @abbycross
- @beckykd
@Cryoris@Qiskit/terra-core@ajavadia
Pull Request Test Coverage Report for Build 19261729294
Details
- 149 of 243 (61.32%) changed or added relevant lines in 9 files are covered.
- 22 unchanged lines in 2 files lost coverage.
- Overall coverage decreased (-0.06%) to 88.138%
| Changes Missing Coverage | Covered Lines | Changed/Added Lines | % |
|---|---|---|---|
| qiskit/qasm3/ast.py | 4 | 5 | 80.0% |
| qiskit/qasm3/exporter.py | 4 | 6 | 66.67% |
| qiskit/circuit/classical/expr/visitors.py | 6 | 10 | 60.0% |
| crates/circuit/src/classical/expr/expr.rs | 0 | 30 | 0.0% |
| crates/circuit/src/classical/expr/range.rs | 128 | 185 | 69.19% |
| <!-- | Total: | 149 | 243 |
| Files with Coverage Reduction | New Missed Lines | % |
|---|---|---|
| crates/qasm2/src/lex.rs | 4 | 92.54% |
| crates/qasm2/src/parse.rs | 18 | 96.62% |
| <!-- | Total: | 22 |
| Totals | |
|---|---|
| Change from base Build 19258642560: | -0.06% |
| Covered Lines: | 94381 |
| Relevant Lines: | 107083 |
💛 - Coveralls
@1ucian0, the current tests that are failing have a priori no connection with the implemented features of this PR: 2 tests, related to high level synthesis and layoutTransformation. In both cases, the error seems to come from two circuits that do not look equivalent. Would it be possible that the seeding might have changed the way the circuit is transformed ? I can't see why my feature would make those tests fail. Any assistance would be appreciated.
@1ucian0, ready for another review round. Not too sure what is going on the docs side for the tests.
@1ucian0 All the tests are now passing. Ready for another review round.
Not too sure what is going on the docs side for the tests.
It seems to be related with an issue in the release notes.
Not too sure what is going on the docs side for the tests.
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.
@1ucian0
Still waiting for another review round on this PR.
Thank you.
@1ucian0
@gadial
Ready for another review. Waiting for your approval.
I think there are still some unaddressed comments from @gadial
I resolved the conversations that were left unresolved, but were addressed in my previous changes. @gadial Feel free to add anything if needed.
@1ucian0, care to give another look? I think I addressed all the prior comments and requests.