-
Notifications
You must be signed in to change notification settings - Fork 117
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
feature: Add delay and barrier for circuits #993
feature: Add delay and barrier for circuits #993
Conversation
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.
A few comments - also it looks like the unit tests are currently failing in CI, could you take a look and resolve this? Thank you!
Co-authored-by: Ryan Shaffer <[email protected]>
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.
Good start. left comments throughout the code. Feel free to ask any question if it does not make sense and give your opinion about the brainstorming parts (don't resolve them though, I'd like other Braket members to think about them).
@@ -3847,6 +3847,77 @@ def pulse_gate( | |||
Gate.register_gate(PulseGate) | |||
|
|||
|
|||
class Barrier(Gate): |
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 sold yet on whether it should be a Gate
or something else like a CompilerDirective
.
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.
@jcjaskula-aws , @rmshaffer , did you get a chance to brainstorm if its okay to proceed with Barrier as a Gate?
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 with @jcjaskula-aws - I think CompilerDirective
makes the most sense here, since these are not instructions which directly affect the state of the qubits.
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.
@Manvi-Agrawal were you able to try converting this to be a CompilerDirective
instead of a Gate
? Please let us know if you hit any issues with this.
matrices.append(gate.to_matrix()) | ||
identity = np.eye(2**gate.qubit_count) | ||
assert np.allclose(functools.reduce(lambda a, b: a @ b, matrices), identity) | ||
if NoMatrixGeneration not in irsubclasses: |
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.
This makes me think that we need to check if it works (or at least returns an understandable error message) with simulators.
Fixed. that was because I renamed a class using refactor->rename but in test framework we need to provide value as strings too. |
Co-authored-by: Jean-Christophe Jaskula <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## feature/delay-barrier-instructions #993 +/- ##
====================================================================
Coverage 100.00% 100.00%
====================================================================
Files 135 136 +1
Lines 8949 9038 +89
Branches 2011 2029 +18
====================================================================
+ Hits 8949 9038 +89 ☔ View full report in Codecov by Sentry. |
class DurationGate(Gate, Parameterizable): | ||
"""Class `DurationGate` represents a quantum gate that operates on N qubits and a duration.""" |
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.
Just a note, there is quite a bit of duplication between this DurationGate
class and the existing AngledGate
class. Did you consider refactoring to make a common base class for instructions with a single float
parameter? I guess this would also ideally extend to the DoubleAngledGate
and TripleAngledGate
classes - so maybe outside the scope of this PR and could be tracked by a separate issue.
@@ -3847,6 +3847,77 @@ def pulse_gate( | |||
Gate.register_gate(PulseGate) | |||
|
|||
|
|||
class Barrier(Gate): |
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.
@Manvi-Agrawal were you able to try converting this to be a CompilerDirective
instead of a Gate
? Please let us know if you hit any issues with this.
Hi @Manvi-Agrawal - we still have 2 days to wrap up unitaryHACK PRs and get them merged. Are you going to be able to address the open comments on this PR? Please let us know so that we know how to proceed with this. Thank you! |
Hi @rmshaffer , actually I wouldnt be able to get to this issue until a couple of weeks due to personal commitments. I am happy to take a look at the open comments after that, even if it doesn't count as progress towards Unitary Hack, if it's not a time pressing issue. Thank you for all your help with this issue and review so far. Past week had been a bit crazy due to finals and other stuff for quarter wrap up. |
@Manvi-Agrawal thank you for all of your work on this PR! We are going to merge the PR into a feature branch and close the unitaryHACK issue to ensure that you get credit for your work. We will then open a new PR to finish the remaining work for merging to |
b1c414a
into
amazon-braket:feature/delay-barrier-instructions
Issue , if available:
Fixes #974
Testing done:
OUTPUT
OUTPUT
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.