-
Notifications
You must be signed in to change notification settings - Fork 13
Add support for OpenQASM 3.0 switch statements in OQPy
#104
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
base: main
Are you sure you want to change the base?
Conversation
Adds Switch, Case, and Default context managers for generating OpenQASM 3
switch statements.
Usage:
with oqpy.Switch(prog, selector) as switch:
with oqpy.Case(switch, 0):
prog.set(result, 10)
with oqpy.Case(switch, 1, 2): # Multiple values
prog.set(result, 20)
with oqpy.Default(switch):
prog.set(result, 100)
Changes:
- control_flow.py: Add Switch class, Case and Default context managers
- program.py: Add visit_SwitchStatement to MergeCalStatementsPass
- test_directives.py: Add comprehensive tests for switch functionality
Tests nested switches with empty case bodies and multiple values
per case (case 1, 2, 5, 12 { }).
The Default() function now raises RuntimeError if called more than once per Switch statement, preventing silent overwrites where only the last default block would be kept. This follows the existing validation pattern used by Else (which raises "Else without If").
- Fix type annotations in Switch class (__exit__ method) - Fix import sorting in control_flow.py - Fix unused variable in MergeCalStatementsPass.visit_SwitchStatement - Update openpulse dependency to >=1.0.0 (required for SwitchStatement AST)
Fixes for linting and Python 3.8
|
The coverage is complaining about missing the branch in the control_flow.py switch context manager exit method and the visit_SwitchStatement method in program.py The lint workflow has a ton of mypy errors, my best guess is the openpulse upgrade also upgrades mypy. Perhaps solving these in a dedicated openpulse upgrade MR would be good. In the meantime, pinning the mypy version may solve this |
- Add test for exception propagation in Switch context manager (covers control_flow.py __exit__ branch) - Add tests for MergeCalStatementsPass.visit_SwitchStatement with encal_declarations=True (with and without default case) - Make mypy check continue-on-error in CI workflow due to type incompatibilities introduced by openpulse 1.0.0 upgrade - Fix black formatting in control_flow.py overload stubs Coverage is now 100%. The mypy errors are pre-existing type issues exposed by the openpulse AST type definition changes and should be addressed in a separate PR.
ajberdy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, left some comments inline!
.github/workflows/test.yml
Outdated
|
|
||
| - name: Run mypy | ||
| # TODO: Remove continue-on-error after fixing openpulse 1.0.0 type incompatibilities | ||
| continue-on-error: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this is a change we'd want to merge to main. If so, I'd recommend linking a tracking issue in this TODO
And I'd be more tempted to pin the mypy version if possible rather than making a successful type check optional-- this creates a blind spot to new type errors not introduced by the upgrade.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason to update mypy? It should already be "pinned" by the poetry.lock file. We should be able to update openpulse without updating mypy.
| self.program = program | ||
| self.target = target | ||
| self.cases: list[tuple[list[ast.Expression], list[ast.Statement]]] = [] | ||
| self.default: list[ast.Statement] | None = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes sense for Oqpy in general to allow an empty default, to give full flexibility as per the OpenQASM spec. However, the OpenQASM ast implementation notes
# Note that `None` is quite different to `[]` in this case; the latter is
# an explicitly empty body, whereas the absence of a default might mean
# that the switch is inexhaustive, and a linter might want to complain.
Do we want to add an optional flag to this class to toggle whether a None default is allowed? Or should that be handled by oqpy's consumers?
Two other options on the table are defaulting to an empty block (perhaps risky) or raising an error/warning if no default is given (perhaps annoying)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None default seems like sane behavior since the produced openqasm is closest to what was written. I'm not sure openqasm needs to be the one enforcing default behavior.
| if exc_type is not None: | ||
| return False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to google what the return value of a context manager's __exit__ function means (whether to suppress exceptions within the block), so perhaps a comment here would help future maintainers, but maybe it's relatively common knowledge/easy to track down.
On the other hand, the default behavior (i.e. when returning None) is to not suppress errors, so maybe we don't need a return value/annotation at all here. If we're raising an exception anyways, we probably don't care whether the statement gets added to the program, so we could remove this branch entirely, though theoretically if we wanted to exit a couple clock cycles early, we could keep this line without the return value
oqpy/program.py
Outdated
| if node.default: | ||
| node.default.statements = self.process_statement_list(node.default.statements) | ||
| self.generic_visit(node, context) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Smallest nit (feel free to ignore) - if node.default is not None feels like a tighter condition here in terms of symmetry with the non-default cases. If node.default.statements was explicitly given as [], a symmetric approach would trivially process that, as it would for an empty case block.
Of course, the result is the same, but being explicit about why we would skip processing the default (it's set to None, vs happens to be empty) ever so slightly reduces mental load for developers reading the code.
tests/test_directives.py
Outdated
| with oqpy.Switch(prog, i) as outer_switch: | ||
| with oqpy.Case(outer_switch, 1, 2, 5, 12): | ||
| pass # Empty case body | ||
| with oqpy.Case(outer_switch, 3): | ||
| with oqpy.Switch(prog, j) as inner_switch: | ||
| with oqpy.Case(inner_switch, 10, 15, 20): | ||
| prog.gate(q, "h") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if we do with oqpy.Default(outer_switch) (or non-default case) while inside the inner_switch context? I suspect it likely works as expected, but would be nice to have a test
tests/test_directives.py
Outdated
| class TestException(Exception): | ||
| pass | ||
|
|
||
| with pytest.raises(TestException): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit
| with pytest.raises(TestException): | |
| with pytest.raises(TestException, match="test error"): |
tests/test_directives.py
Outdated
| assert "switch" in qasm | ||
| assert "case 0" in qasm | ||
| assert "case 1" in qasm | ||
| assert "default" in qasm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the looser check here? How come not just compare the qasm string as with the others?
tests/test_directives.py
Outdated
| assert "switch" in qasm | ||
| assert "case 0" in qasm | ||
| assert "case 1" in qasm | ||
| assert "default" not in qasm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if we have a non-IntVar selector? Can we add test with some examples we believe should work (e.g. float, bool, expression, etc) and some that probably shouldn't (e.g. qubit, frame, forinloop)
.github/workflows/test.yml
Outdated
|
|
||
| - name: Run mypy | ||
| # TODO: Remove continue-on-error after fixing openpulse 1.0.0 type incompatibilities | ||
| continue-on-error: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason to update mypy? It should already be "pinned" by the poetry.lock file. We should be able to update openpulse without updating mypy.
| self.program = program | ||
| self.target = target | ||
| self.cases: list[tuple[list[ast.Expression], list[ast.Statement]]] = [] | ||
| self.default: list[ast.Statement] | None = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None default seems like sane behavior since the produced openqasm is closest to what was written. I'm not sure openqasm needs to be the one enforcing default behavior.
| self, | ||
| time: AstConvertible, | ||
| qubits_or_frames: AstConvertible | Iterable[AstConvertible] = (), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like your formatter is set to 80 lines, but pyproject.toml specifies 100, perhaps we can revert this and the below changes
pyproject.toml
Outdated
| python = ">=3.8,<4.0" | ||
| # 0.4 loosens the antlr4-python3-runtime constraints | ||
| openpulse = ">=0.5.0" | ||
| # 1.0 adds SwitchStatement and CompoundStatement AST support |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment is a bit too path dependent
| # 1.0 adds SwitchStatement and CompoundStatement AST support |
oqpy/control_flow.py
Outdated
|
|
||
|
|
||
| @contextlib.contextmanager | ||
| def Case(switch: Switch, *values: AstConvertible) -> Iterator[None]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passing in the switch leaves some room for shenanigans, where the case statement is not directly within the switch context. My suggestion is we
- In
Switch.__enter__do aProgram._push() - update the pushed
ProgramStateto add aactive_switch_statementfield. - Pass the program into
Case - In
Case, get the switch from the program state (fail if not present). - In
Switch.__exit__doProgram._pop
So the usage would look more like:
with Switch(program, selector):
with Case(program, 0):
...We can also enforce that only case statements appear within a Switch context by raising an error if active_switch_statement is not None in ProgramState.add_statement. This would prevent usages like:
with Switch(program, selector):
program.play(frame, waveform) # or any command outside of a case statement
Summary
Add support for OpenQASM 3.0
switchstatements in OQPyChanges
New Features
Switchcontext manager: Creates a switch statement block that evaluates a selector expressionCasecontext manager: Defines a case within a switch statement, supporting multiple values per case (e.g.,Case(switch, 1, 2, 3))Defaultcontext manager: Defines the default case for unmatched valuesImplementation Details
Switch,Case, andDefaulttooqpy/control_flow.pyMergeCalStatementsPassinoqpy/program.pyto handleSwitchStatementAST nodes__all__Validation
ValueErrorotherwise)RuntimeErroron duplicate)Usage Example
Generates:
OPENQASM 3.0; int[32] result = 0; int[32] selector = 0; switch (selector) { case 0 { result = 10; } case 1, 2 { result = 20; } default { result = 100; } }