-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Command/Inst refactor: Acquire (and Kernel, Discriminator) #3935
Command/Inst refactor: Acquire (and Kernel, Discriminator) #3935
Conversation
…int to the new one. Deprecate AcquireInstruction and make it return an Acquire instead
5ae572a
to
c284476
Compare
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 looks good!
…cqurie. Fix up some more tests. Fill in pulse_instructions converter for Acquire
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.
Looking good :)
Improve description of Acquire Co-Authored-By: Thomas Alexander <thomasalexander2718@gmail.com>
…o/qiskit-terra into issue-3750-acquire-instruction
…a test to also match
…o/qiskit-terra into issue-3750-acquire-instruction
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.
Minor comments regarding acquire operands and documentation.
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.
LGTM, just one question/suggestion in the release notes
releasenotes/notes/unify-instructions-and-commands-aaa6d8724b8a29d3.yaml
Outdated
Show resolved
Hide resolved
…o/qiskit-terra into issue-3750-acquire-instruction
releasenotes/notes/unify-instructions-and-commands-aaa6d8724b8a29d3.yaml
Outdated
Show resolved
Hide resolved
…a29d3.yaml Co-Authored-By: Matthew Treinish <mtreinish@kortar.org>
* Make new Acquire instruction and update path to Acquire command to point to the new one. Deprecate AcquireInstruction and make it return an Acquire instead * Fix some style and continue implementation * Fill in docs and fix tests * Fill in __repr__ and __eq__ as necessary for Kernel, Discriminator, Acqurie. Fix up some more tests. Fill in pulse_instructions converter for Acquire * Style fixes * Fix bug missing inclusion of Acquire type along with AcquireInstruction * Fixup some kernel instantiation in pulse instructions converter * Fixup style * Try to fix the docs build * Update qiskit/pulse/instructions/acquire.py Improve description of Acquire Co-Authored-By: Thomas Alexander <thomasalexander2718@gmail.com> * Respond to review * Update repr for Kernel and Discriminator to reflect **params. Update a test to also match * style * Complete release note * Complete reno note (was missing one line) * discriminator and kernel shouldn't be treated as instruction operands * Add link in releasenote * Update releasenotes/notes/unify-instructions-and-commands-aaa6d8724b8a29d3.yaml Co-Authored-By: Matthew Treinish <mtreinish@kortar.org> Co-authored-by: Thomas Alexander <thomasalexander2718@gmail.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
Summary
Part 5/7 for issue #3750
Make new Acquire instruction and update path to Acquire command to point to the new one. Deprecate AcquireInstruction and make it return an Acquire instead
Details and comments
This is going to look so much nicer after the deprecation period for the multiple layers of changes that have been made 😄