Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support stabilised syntax for OpenQASM 3 switch #11417

Merged
merged 6 commits into from
Feb 1, 2024

Conversation

jakelishman
Copy link
Member

Summary

The OpenQASM 3 project made some changes to the preliminary syntax of the switch statement, merging the final version in openqasm/openqasm#463. This commit adds support for the new syntax, making it available by default while still supporting the experimental flag to enable the preliminary syntax. Support for the old flag is necessary to allow Qiskit to continue to interoperate with other tooling that still assumes the preliminary format (we know that at least the IBM QSS compiler is in this position right now), and in general should hopefully not be too onerous to continue to support.

Details and comments

The new syntax won't be supported by the QSS compiler yet, and there's no need to rush to that - it's easy for Qiskit to continue to support both, and having an easy flag switch over means that any deployments of Qiskit alongside other OpenQASM 3 parsers can easily keep them in sync without having to fall behind on Qiskit bugfixes.

The OpenQASM 3 project made some changes to the preliminary syntax of
the `switch` statement, merging the final version in
openqasm/openqasm#463.  This commit adds support for the new syntax,
making it available by default while still supporting the experimental
flag to enable the preliminary syntax.  Support for the old flag is
necessary to allow Qiskit to continue to interoperate with other tooling
that still assumes the preliminary format (we know that at least the IBM
QSS compiler is in this position right now), and in general should
hopefully not be too onerous to continue to support.
@jakelishman jakelishman added Changelog: New Feature Include in the "Added" section of the changelog mod: qasm3 Related to OpenQASM 3 import or export labels Dec 14, 2023
@jakelishman jakelishman added this to the 1.0.0 milestone Dec 14, 2023
@jakelishman jakelishman requested a review from a team as a code owner December 14, 2023 17:22
@qiskit-bot
Copy link
Collaborator

One or more of the the following people are requested to review this:

  • @Qiskit/terra-core

@coveralls
Copy link

coveralls commented Dec 14, 2023

Pull Request Test Coverage Report for Build 7732196831

Warning: This coverage report may be inaccurate.

We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
To ensure accuracy in future PRs, please see these guidelines.
A quick fix for this PR: rebase it; your next report should be accurate.

  • -1 of 50 (98.0%) changed or added relevant lines in 3 files are covered.
  • 129 unchanged lines in 18 files lost coverage.
  • Overall coverage decreased (-0.005%) to 89.581%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/qasm3/printer.py 24 25 96.0%
Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/expr.rs 1 93.81%
qiskit/circuit/library/standard_gates/rx.py 1 98.55%
qiskit/circuit/library/standard_gates/ry.py 1 98.51%
qiskit/circuit/library/standard_gates/rz.py 1 98.46%
qiskit/circuit/library/standard_gates/swap.py 1 98.08%
qiskit/circuit/library/standard_gates/u3.py 1 98.9%
qiskit/circuit/library/standard_gates/p.py 2 98.0%
qiskit/circuit/library/standard_gates/u.py 2 97.78%
qiskit/circuit/library/standard_gates/u1.py 3 96.7%
crates/qasm2/src/lex.rs 4 91.94%
Totals Coverage Status
Change from base Build 7731515567: -0.005%
Covered Lines: 59455
Relevant Lines: 66370

💛 - Coveralls

Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM, just one 🇬🇧 nit inline. The other thing is the name of the statement class for the experimental version. But both are fairly minor.

qiskit/qasm3/ast.py Outdated Show resolved Hide resolved
qiskit/qasm3/ast.py Outdated Show resolved Hide resolved
Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I caught some 🇬🇧 "stabilised" in the release note, but we can just fix that in the release note round-up for the final release. Thanks for the quick update.

Comment on lines +5 to +12
:mod:`qiskit.qasm3`) now supports the stabilised syntax of the ``switch`` statement in OpenQASM 3
by default. The pre-certification syntax of the ``switch`` statement is still available by
using the :attr:`.ExperimentalFeatures.SWITCH_CASE_V1` flag in the ``experimental`` argument of
the exporter. There is no feature flag required for the stabilised syntax, but if you are
interfacing with other tooling that is not yet updated, you may need to pass the experimental
flag.

The syntax of the stabilised form is slightly different with regards to terminating ``break``
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
:mod:`qiskit.qasm3`) now supports the stabilised syntax of the ``switch`` statement in OpenQASM 3
by default. The pre-certification syntax of the ``switch`` statement is still available by
using the :attr:`.ExperimentalFeatures.SWITCH_CASE_V1` flag in the ``experimental`` argument of
the exporter. There is no feature flag required for the stabilised syntax, but if you are
interfacing with other tooling that is not yet updated, you may need to pass the experimental
flag.
The syntax of the stabilised form is slightly different with regards to terminating ``break``
:mod:`qiskit.qasm3`) now supports the stabilized syntax of the ``switch`` statement in OpenQASM 3
by default. The pre-certification syntax of the ``switch`` statement is still available by
using the :attr:`.ExperimentalFeatures.SWITCH_CASE_V1` flag in the ``experimental`` argument of
the exporter. There is no feature flag required for the stabilized syntax, but if you are
interfacing with other tooling that is not yet updated, you may need to pass the experimental
flag.
The syntax of the stabilized form is slightly different with regards to terminating ``break``

class SwitchStatement(Statement):
"""AST node for the proposed 'switch-case' extension to OpenQASM 3."""
class SwitchStatementPreview(Statement):
"""AST node for the proposed 'switch-case' extension to OpenQASM 3, before the syntax was
Copy link
Contributor

@jlapeyre jlapeyre Jan 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to think of a better more useful name than ....Preview. I think contemporary software devs find the word "Legacy" distasteful. But it's probably widely understood by casual programmers. Maybe SwitchStatementOldStyle. In OQ3, "old style" refers to OQ2 legacy syntax. In this case it's not from a previous version. But OldStyle tells me that I probably don't want this, even if I am not familiar with its history.

Or even SwitchStatementObsolete. This really tells me I should not be using it.

@mtreinish mtreinish added this pull request to the merge queue Jan 31, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 1, 2024
@mtreinish mtreinish added this pull request to the merge queue Feb 1, 2024
Merged via the queue into Qiskit:main with commit 4a69a21 Feb 1, 2024
12 checks passed
@jakelishman jakelishman deleted the qasm3/switch-v2 branch February 1, 2024 02:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: New Feature Include in the "Added" section of the changelog mod: qasm3 Related to OpenQASM 3 import or export
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants