-
Notifications
You must be signed in to change notification settings - Fork 121
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
feat: Add gate modifiers and additional supported gates to AutoQASM #958
feat: Add gate modifiers and additional supported gates to AutoQASM #958
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## feature/autoqasm #958 +/- ##
==================================================
Coverage 100.00% 100.00%
==================================================
Files 173 172 -1
Lines 10589 10568 -21
Branches 2264 2261 -3
==================================================
- Hits 10589 10568 -21 ☔ View full report in Codecov by Sentry. |
t(1, control=0) | ||
t(2, control=[0, 1], control_state="10", power=0.123) |
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.
Could be interesting to think of alternatives, like
aq.ctrl([0, 1], "10") @ aq.pow(0.123) @ t(2)
or
# allows controlling logical chunks of a program
# (can we distribute the op effectively?)
with aq.ctrl(0):
t(1)
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 agree that there may be a more clever syntax for this. The approach in this PR is most consistent with what the Braket SDK does today (and also happens to be the easiest to implement, given how OQpy implements control modifiers). But we should keep thinking about this and consider whether there's something nicer to do.
control = [control] | ||
|
||
if control_state is None: | ||
control_state = [1] * len(control) |
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 this logic duplicated in the BDK? I suppose the risk of it changing / getting out of sync is pretty minuscule if there's no bugs
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.
Yes, it's a duplicate of the logic here:
amazon-braket-sdk-python/src/braket/circuits/instruction.py
Lines 98 to 101 in 395662e
self._control_state = BasisState( | |
(1,) * len(control_set) if control_state is None else control_state, | |
len(control_set), | |
) |
It would be difficult to re-use directly since we don't use the BDK Instruction
class in AutoQASM. I do agree that the duplication is not ideal, but it seems very low-risk.
Issue #, if available:
Gate modifier support in AutoQASM: #957
Support U, GPhase, PRx gates in AutoQASM
Description of changes:
Add support for three additional keyword arguments to AutoQASM gates, for consistency with the
Circuit
class and associated gates (see #562):control: QubitIdentifierType | Iterable[QubitIdentifierType] | None = None
control_state: BasisStateInput | None = None
power: float | None = None
Also add the
gphase
,u
, andprx
gate definitions for consistency with the main Braket SDK (see #799, #945).Testing done:
tox
passes. Tests added to cover new functionality.Merge Checklist
Put an
x
in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your pull request.General
Tests
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.