-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Issue 3750 wrap up #4034
Merged
Merged
Issue 3750 wrap up #4034
Changes from 26 commits
Commits
Show all changes
34 commits
Select commit
Hold shift + click to select a range
ebaf14a
Clean up instructions and update documentation
lcapelluto 0a1c1f0
Documentation improvements, add 'name=' to Instruction repr
lcapelluto a5d8ad6
Fixup description of operands (does not return a list now, returns a …
lcapelluto 8d4a2e1
Clean up magic methods for Pulse and its subclasses
lcapelluto d66d9c6
Add more tests for instructions and move Pulse tests into a new pulse…
lcapelluto 9ddbe40
Give instructions an ID. Give instructions a simple default name if o…
lcapelluto 304c613
Give pulses ids too
lcapelluto 1ed82cd
fixup some documentation and style
lcapelluto 1baf335
Fixup name printing of pulses: Display names are not auto generated. …
lcapelluto 3c1e2f2
Remove deprecation warnings from reschedule
lcapelluto aa68680
Try to fix missing docs links
lcapelluto 90c05b8
Hide full path of module in autosum
lcapelluto d93defb
Add an autosummary to the pulse lib init file for api ref
lcapelluto 489ee64
Apply suggestions from code review
lcapelluto 2e5bfb9
Fixup style errors that I introduced during review
lcapelluto cbbc4dc
Cannot use 'Pulse' type hint from 'Play' due to cyclic import while d…
lcapelluto 8f98e51
Respond to feedback: improve documentation about instruction operands
lcapelluto 7f2571c
Upgrade to threadsafe ids for Pulse and Instruction: no longer is the…
lcapelluto 57f5c17
Fixup style (ignore id is not a valid name)
lcapelluto e1775e3
No display name if it is not provided by the user
lcapelluto 6342468
Update qiskit/pulse/instructions/play.py
lcapelluto c13c144
Update tests and instruction repr for None default name
lcapelluto 8bc38ae
Style fix
lcapelluto f07d177
Update qiskit/pulse/instructions/play.py
lcapelluto 3563220
Merge branch 'master' into issue-3750-wrap-up
lcapelluto 5f790b5
Remove danp's trailing whitespace :D
lcapelluto 731f639
PulseCommand is not used anywhere now and can be removed
lcapelluto 70dae97
Improve deprecation messaging for pulses being called
lcapelluto c266541
Include operands as the first argument to Instruction
lcapelluto 2ab1b35
Merge branch 'master' into issue-3750-wrap-up
lcapelluto 136fc2b
Fixup docs error
lcapelluto 97beb28
Merge branch 'issue-3750-wrap-up' of github.com:lcapelluto/qiskit-ter…
lcapelluto e3c7a48
Make Instruction init arg take a tuple rather than a list
lcapelluto d3209ce
Merge branch 'master' into issue-3750-wrap-up
lcapelluto 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
There are no files selected for viewing
This file contains 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 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 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 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 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 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 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 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
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.
This should be limited to one of MemorySlot or RegisterSlot.
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 haven't seen that restriction anywhere else, what does that stem from? I've seen tests with acquire taking memslot and reg slot. No one uses register slots yet, so it's possible they're not handled correctly in a couple places.
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.
Can users not use readout results for fast feedback and also get that result?
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.
It will be a practical restriction in the hardware. In reality it would be something like
Storing directly into a memory_slot is currently supported but is a convenience that masks hardware implementation. I would like to get away from multi-purpose instructions and I think this is a good first step that can be made while we are already changing the interface.
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.
so Acquire should really only take a register slot. This seems like an implementation change that is out of scope for this PR. Maybe next sprint?
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.
Will we be able to make the change before release?