-
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
Open
jmdewart
wants to merge
8
commits into
openqasm:main
Choose a base branch
from
jmdewart:mdewart/switch-support
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
266ea34
Add switch statement support
jmdewart 9053611
Add test for nested switch statements
jmdewart ce9962e
Add validation to prevent multiple default cases in switch statements
jmdewart 1136144
Fix style issues and update openpulse dependency for switch support
jmdewart d595f8a
fixes for 3.8 and linting
ajberdy 419c4df
Merge pull request #1 from ajberdy/38-fixes
jmdewart 3687715
Add coverage tests for switch statements and make mypy non-blocking
jmdewart 4578908
Address PR review feedback for switch statement support
jmdewart File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,7 +25,7 @@ | |
|
|
||
| import warnings | ||
| from copy import deepcopy | ||
| from typing import Any, Hashable, Iterable, Iterator, Optional | ||
| from typing import TYPE_CHECKING, Any, Hashable, Iterable, Iterator, Optional | ||
|
|
||
| from openpulse import ast | ||
| from openpulse.printer import dumps | ||
|
|
@@ -44,6 +44,9 @@ | |
| from oqpy.pulse import FrameVar, PortVar, WaveformVar | ||
| from oqpy.timing import convert_duration_to_float, convert_float_to_duration | ||
|
|
||
| if TYPE_CHECKING: | ||
| from oqpy.control_flow import Switch | ||
|
|
||
| __all__ = ["Program"] | ||
|
|
||
|
|
||
|
|
@@ -59,6 +62,7 @@ def __init__(self) -> None: | |
| self.body: list[ast.Statement | ast.Pragma] = [] | ||
| self.if_clause: Optional[ast.BranchingStatement] = None | ||
| self.annotations: list[ast.Annotation] = [] | ||
| self.active_switch: Optional["Switch"] = None # Set when inside a switch context | ||
|
|
||
| def add_if_clause(self, condition: ast.Expression, if_clause: list[ast.Statement]) -> None: | ||
| if_clause_annotations, self.annotations = self.annotations, [] | ||
|
|
@@ -82,6 +86,10 @@ def add_statement(self, stmt: ast.Statement | ast.Pragma) -> None: | |
| # it seems to conflict with the definition of ast.Program. | ||
| # Issue raised in https://github.com/openqasm/openqasm/issues/468 | ||
| assert isinstance(stmt, (ast.Statement, ast.Pragma)) | ||
| if self.active_switch is not None: | ||
| raise RuntimeError( | ||
| "Statements inside a Switch block must be within a Case or Default context" | ||
| ) | ||
| if isinstance(stmt, ast.Statement) and self.annotations: | ||
| stmt.annotations = self.annotations + list(stmt.annotations) | ||
| self.annotations = [] | ||
|
|
@@ -133,7 +141,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 +428,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 +620,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 +723,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 is not None: | ||
| node.default.statements = self.process_statement_list(node.default.statements) | ||
| self.generic_visit(node, context) | ||
|
|
||
| def visit_CalibrationStatement( | ||
| self, node: ast.CalibrationStatement, context: None = None | ||
| ) -> None: | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
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.