-
Notifications
You must be signed in to change notification settings - Fork 15
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?
Changes from 7 commits
266ea34
9053611
ce9962e
1136144
d595f8a
419c4df
3687715
4578908
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,7 +18,17 @@ | |
| from __future__ import annotations | ||
|
|
||
| import contextlib | ||
| from typing import TYPE_CHECKING, Iterable, Iterator, Optional, TypeVar, overload | ||
| from typing import ( | ||
| TYPE_CHECKING, | ||
| Any, | ||
| ContextManager, | ||
| Iterable, | ||
| Iterator, | ||
| Literal, | ||
| Optional, | ||
| TypeVar, | ||
| overload, | ||
| ) | ||
|
|
||
| from openpulse import ast | ||
|
|
||
|
|
@@ -38,7 +48,7 @@ | |
| from oqpy.program import Program | ||
|
|
||
|
|
||
| __all__ = ["If", "Else", "ForIn", "While", "Range"] | ||
| __all__ = ["If", "Else", "ForIn", "While", "Range", "Switch", "Case", "Default"] | ||
|
|
||
|
|
||
| @contextlib.contextmanager | ||
|
|
@@ -176,3 +186,99 @@ def While(program: Program, condition: OQPyExpression) -> Iterator[None]: | |
| yield | ||
| state = program._pop() | ||
| program._add_statement(ast.WhileLoop(to_ast(program, condition), state.body)) | ||
|
|
||
|
|
||
| class Switch(ContextManager["Switch"]): | ||
| """Context manager for switch statement control flow. | ||
|
|
||
| .. code-block:: python | ||
|
|
||
| selector = IntVar(0) | ||
| with Switch(program, selector) as switch: | ||
| with Case(switch, 0): | ||
| program.increment(result, 1) | ||
| with Case(switch, 1, 2): # Multiple values in one case | ||
| program.increment(result, 2) | ||
| with Default(switch): | ||
| program.increment(result, 100) | ||
|
|
||
| """ | ||
|
|
||
| def __init__(self, program: "Program", target: OQPyExpression): | ||
| self.program = program | ||
| self.target = target | ||
| self.cases: list[tuple[list[ast.Expression], list[ast.Statement]]] = [] | ||
| self.default: list[ast.Statement] | None = None | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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)
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
|
|
||
| def __enter__(self) -> "Switch": | ||
| return self | ||
|
|
||
| def __exit__( | ||
| self, | ||
| exc_type: type[BaseException] | None, | ||
| exc_val: BaseException | None, | ||
| exc_tb: Any, | ||
| ) -> Literal[False]: | ||
| if exc_type is not None: | ||
| return False | ||
|
Comment on lines
+225
to
+227
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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 |
||
| # Build the case tuples as (list of expressions, CompoundStatement) | ||
| case_tuples = [(values, ast.CompoundStatement(body)) for values, body in self.cases] | ||
| default_stmt = ast.CompoundStatement(self.default) if self.default else None | ||
| stmt = ast.SwitchStatement( | ||
| to_ast(self.program, self.target), | ||
| case_tuples, | ||
| default_stmt, | ||
| ) | ||
| self.program._add_statement(stmt) | ||
| return False | ||
|
|
||
|
|
||
| @contextlib.contextmanager | ||
| def Case(switch: Switch, *values: AstConvertible) -> Iterator[None]: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
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 |
||
| """Context manager for a case within a switch statement. | ||
|
|
||
| Must be used inside a Switch context. Multiple values can be provided | ||
| for a single case block. | ||
|
|
||
| .. code-block:: python | ||
|
|
||
| with Switch(program, selector) as switch: | ||
| with Case(switch, 0): | ||
| # Handle case 0 | ||
| program.increment(result, 1) | ||
| with Case(switch, 1, 2): | ||
| # Handle cases 1 and 2 | ||
| program.increment(result, 2) | ||
|
|
||
| """ | ||
| if not values: | ||
| raise ValueError("Case requires at least one value") | ||
| switch.program._push() | ||
| yield | ||
| state = switch.program._pop() | ||
| case_values = [to_ast(switch.program, v) for v in values] | ||
| switch.cases.append((case_values, state.body)) | ||
|
|
||
|
|
||
| @contextlib.contextmanager | ||
| def Default(switch: Switch) -> Iterator[None]: | ||
| """Context manager for the default case within a switch statement. | ||
|
|
||
| Must be used inside a Switch context. | ||
|
|
||
| .. code-block:: python | ||
|
|
||
| with Switch(program, selector) as switch: | ||
| with Case(switch, 0): | ||
| program.increment(result, 1) | ||
| with Default(switch): | ||
| # Handle all other cases | ||
| program.increment(result, 100) | ||
|
|
||
| """ | ||
| if switch.default is not None: | ||
| raise RuntimeError("Switch statement can only have one default case") | ||
| switch.program._push() | ||
| yield | ||
| state = switch.program._pop() | ||
| switch.default = state.body | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -133,7 +133,9 @@ def __iadd__(self, other: Program) -> Program: | |
| self.defcals.update(other.defcals) | ||
| for name, subroutine_stmt in other.subroutines.items(): | ||
| self._add_subroutine( | ||
| name, subroutine_stmt, needs_declaration=name not in other.declared_subroutines | ||
| name, | ||
| subroutine_stmt, | ||
| needs_declaration=name not in other.declared_subroutines, | ||
| ) | ||
| for name, gate_stmt in other.gates.items(): | ||
| self._add_gate(name, gate_stmt, needs_declaration=name not in other.declared_gates) | ||
|
|
@@ -418,7 +420,9 @@ def declare( | |
| return self | ||
|
|
||
| def delay( | ||
| self, time: AstConvertible, qubits_or_frames: AstConvertible | Iterable[AstConvertible] = () | ||
| self, | ||
| time: AstConvertible, | ||
| qubits_or_frames: AstConvertible | Iterable[AstConvertible] = (), | ||
|
Comment on lines
+431
to
+433
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| ) -> Program: | ||
| """Apply a delay to a set of qubits or frames.""" | ||
| if not isinstance(qubits_or_frames, Iterable): | ||
|
|
@@ -608,7 +612,9 @@ def reset(self, qubit: quantum_types.Qubit) -> Program: | |
| return self | ||
|
|
||
| def measure( | ||
| self, qubit: quantum_types.Qubit, output_location: classical_types.BitVar | None = None | ||
| self, | ||
| qubit: quantum_types.Qubit, | ||
| output_location: classical_types.BitVar | None = None, | ||
| ) -> Program: | ||
| """Measure a particular qubit. | ||
|
|
||
|
|
@@ -709,6 +715,13 @@ def visit_BranchingStatement(self, node: ast.BranchingStatement, context: None = | |
| node.else_block = self.process_statement_list(node.else_block) | ||
| self.generic_visit(node, context) | ||
|
|
||
| def visit_SwitchStatement(self, node: ast.SwitchStatement, context: None = None) -> None: | ||
| for _, case_block in node.cases: | ||
| case_block.statements = self.process_statement_list(case_block.statements) | ||
| if node.default: | ||
| node.default.statements = self.process_statement_list(node.default.statements) | ||
| self.generic_visit(node, context) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Smallest nit (feel free to ignore) - 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. |
||
|
|
||
| def visit_CalibrationStatement( | ||
| self, node: ast.CalibrationStatement, context: None = None | ||
| ) -> None: | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
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.