Issue 3750 wrap up #4034
Merged
Issue 3750 wrap up #4034
Conversation
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
Clean up magic methods (move most into Instructions), operands returns a tuple rather than a list, fix bug in kernel and discriminator repr, general clean up of Instructions Cannot deprecate commands attribute on Instructions directly, since it must be used in the assembler. It will be easy to deprecate and remove in a few months Update documentation and fixup an operands test fix indentation in instructions init
…_lib testing file
lcapelluto
requested review from
DanPuzzuoli,
eggerdj,
nkanazawa1989 and
taalexander
as code owners
…ne isn't provided. Add tests
…Test that deprecation warnings are printed for command tests
lcapelluto
requested review from
1ucian0,
ajavadia,
kdk,
mtreinish and
jyu00
as code owners
Remove deprecation warnings from schedule tests Remove warnings from assembler Remove or catch deprecation warnings from qobj tests Fixup style in assembler tests Migrate scheduler tests to new pulse style
lcapelluto
force-pushed
the
issue-3750-wrap-up
branch
from
2c56338 to
3c1e2f2
Compare
DanPuzzuoli
requested changes
Mar 31, 2020
taalexander
requested changes
Mar 31, 2020
qiskit/pulse/instructions/acquire.py
Outdated
| """Return a list of instruction operands.""" | ||
| return [self.duration, self.channel, | ||
| self.mem_slot, self.reg_slot] | ||
| def operands(self) -> Tuple[int, AcquireChannel, MemorySlot, RegisterSlot]: |
There was a problem hiding this comment.
This should be limited to one of MemorySlot or RegisterSlot.
There was a problem hiding this comment.
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.
Can users not use readout results for fast feedback and also get that result?
There was a problem hiding this comment.
It will be a practical restriction in the hardware. In reality it would be something like
- acquire qubit into register
- store register into memory
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.
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.
Will we be able to make the change before release?
lcapelluto
commented
Mar 31, 2020
lcapelluto
commented
Mar 31, 2020
taalexander
requested changes
Apr 1, 2020
Co-Authored-By: Thomas Alexander <thomasalexander2718@gmail.com>
Co-Authored-By: Daniel Puzzuoli <dan.puzzuoli@gmail.com>
DanPuzzuoli
previously approved these changes
Apr 1, 2020
taalexander
previously approved these changes
Apr 2, 2020
…ra into issue-3750-wrap-up
faisaldebouni
pushed a commit
to faisaldebouni/qiskit-terra
that referenced
this pull request
Aug 5, 2020
* Clean up instructions and update documentation Clean up magic methods (move most into Instructions), operands returns a tuple rather than a list, fix bug in kernel and discriminator repr, general clean up of Instructions Cannot deprecate commands attribute on Instructions directly, since it must be used in the assembler. It will be easy to deprecate and remove in a few months Update documentation and fixup an operands test fix indentation in instructions init * Documentation improvements, add 'name=' to Instruction repr * Fixup description of operands (does not return a list now, returns a tuple) * Clean up magic methods for Pulse and its subclasses * Add more tests for instructions and move Pulse tests into a new pulse_lib testing file * Give instructions an ID. Give instructions a simple default name if one isn't provided. Add tests * Give pulses ids too * fixup some documentation and style * Fixup name printing of pulses: Display names are not auto generated. Test that deprecation warnings are printed for command tests * Remove deprecation warnings from reschedule Remove deprecation warnings from schedule tests Remove warnings from assembler Remove or catch deprecation warnings from qobj tests Fixup style in assembler tests Migrate scheduler tests to new pulse style * Try to fix missing docs links * Hide full path of module in autosum * Add an autosummary to the pulse lib init file for api ref * Apply suggestions from code review Co-Authored-By: Thomas Alexander <thomasalexander2718@gmail.com> Co-Authored-By: Daniel Puzzuoli <dan.puzzuoli@gmail.com> * Fixup style errors that I introduced during review * Cannot use 'Pulse' type hint from 'Play' due to cyclic import while deprecating __call__, so use docstring type hinting instead * Respond to feedback: improve documentation about instruction operands * Upgrade to threadsafe ids for Pulse and Instruction: no longer is the id the order that they were instantiated (which is not necessary for this purpose) * Fixup style (ignore id is not a valid name) * No display name if it is not provided by the user * Update qiskit/pulse/instructions/play.py Co-Authored-By: Thomas Alexander <thomasalexander2718@gmail.com> * Update tests and instruction repr for None default name * Style fix * Update qiskit/pulse/instructions/play.py Co-Authored-By: Daniel Puzzuoli <dan.puzzuoli@gmail.com> * Remove danp's trailing whitespace :D * PulseCommand is not used anywhere now and can be removed * Improve deprecation messaging for pulses being called * Include operands as the first argument to Instruction * Fixup docs error * Make Instruction init arg take a tuple rather than a list Co-authored-by: Thomas Alexander <thomasalexander2718@gmail.com> Co-authored-by: Daniel Puzzuoli <dan.puzzuoli@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Closes #3750
Details and comments
Minor API changes
operandsreturns a tuple rather than a list (better type hints)idfor insts and pulses and make defaultnameNoneBug fix
__repr__bug forDiscriminatorandKernelconfigDocs
Misc / implementation changes
Pulse+subclasses andInstruction+subclassesPulseCommandclass wasn't used anywhere so I removed the file :DTests
test_instructions.py: New instruction tests and migrate Acquire teststest_pulse_lib.py: Move tests forParametricPulsesandSamplePulseto this since they are no longerCommandsself.assertWarns(DeprecationWarning)to catch test warnings