Command/Inst refactor: Pulses and Play #3936
Merged
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
…The Play instruction takes a Pulse and a PulseChannel. Pulse is a new class within the pulse library that is very close to the deprecated PulseCommand abstract class. The PulseCommand implementations (SamplePulse and the ParametricPulse subclasses) have been moved to the pulse library as well, subclassing from Pulse, and all valid arguments to Play.
lcapelluto
requested review from
DanPuzzuoli,
eggerdj and
taalexander
as code owners
lcapelluto
requested review from
1ucian0,
ajavadia,
diego-plan9,
jyu00,
kdk and
mtreinish
as code owners
…can't import Pulse from Play (for typehints) for the timebeing
taalexander
previously requested changes
Mar 18, 2020
| from ..instructions.play import Play | ||
|
|
||
|
|
||
| class Pulse(ABC): |
There was a problem hiding this comment.
This is more akin to a Type, do you think it should live in a types module along with Kernel and Discriminator?
There was a problem hiding this comment.
That's an interesting idea, I definitely like the idea of keeping simple, basic, "type-like" objects together and out of the way. For now, I think this does make the most sense. Once __call__ is removed (since it's sort of un-type-like) and this command->instruction thing is done, we should work on another organizational refactor, including this idea.
lcapelluto
changed the title
|
my mistake |
mtreinish
reviewed
Mar 25, 2020
releasenotes/notes/unify-instructions-and-commands-aaa6d8724b8a29d3.yaml
Outdated
Show resolved
Hide resolved
| description must subclass from :py:class:`~qiskit.pulse.pulse_lib.Pulse`. | ||
| py:class:`~qiskit.pulse.pulse_lib.SamplePulse` and | ||
| py:class:`~qiskit.pulse.pulse_lib.ParametricPulse` s (e.g. ``Gaussian``) | ||
| now subclass from ``Pulse`` and have been moved to the ``pulse_lib``. |
There was a problem hiding this comment.
This was a bit hard to follow, Originally I thought this should have been the pulse description should either be Pulse, SamplePulse, or ParametricPulse. But then I realized what it was actually saying. Maybe we should split this into 2 release notes. One saying that SamplePulse and ParametricPulse are now subclasses of Pulse and have been moved to pulse_lib (maybe as an upgrade note?) and the other about Play.
There was a problem hiding this comment.
This is a good idea, thank you!
…a29d3.yaml Co-Authored-By: Matthew Treinish <[email protected]>
…a into issue-3750-pulses
mtreinish
approved these changes
Mar 25, 2020
lcapelluto
added
API
faisaldebouni
pushed a commit
to faisaldebouni/qiskit-terra
that referenced
this pull request
Aug 5, 2020
* Deprecate PulseCommands, create new Play Instruction-implementation. The Play instruction takes a Pulse and a PulseChannel. Pulse is a new class within the pulse library that is very close to the deprecated PulseCommand abstract class. The PulseCommand implementations (SamplePulse and the ParametricPulse subclasses) have been moved to the pulse library as well, subclassing from Pulse, and all valid arguments to Play. * Update import statements in Pulse * Fill in the implementation of some IP work like __call__ * Fill in and update documentation, complete import paths * fixup cyclic imports * IP updates to assemble_schedules for supporting Play instruction * Give all pulses a name arg and update reno note * Continue pulse_instruction conversion support for Play * Add support for Play in assemble schedules, fix style * Attempt to fix build error * Add missing name field to Constant pulse * To support __call__ from Pulse, I need to import Play, which means I can't import Pulse from Play (for typehints) for the timebeing * Fixup bugs introduced in SamplePulse * Update tests with new API * Add tests and fixup missing name passing * Fixup tests, remove deprecation warning from assemble execution, add hash and eq methods to pulses * Fixup docs * Update functional_pulse import path and Instruction type ref * try again with the type hints * Was missing updates to PulseCommand that needed to be migrated with the new Pulse class * Was missing docstring type documentation for unresolvable type * Was missing removal of PulseStyle in parametric pulses * The changes from the sphinx warrnings pass hadn't been moved with the migrated ParametricPulses module. Moved the drag pulse docstring over finally * Fix spacing in drag pulse docstring * The docs should finally build now * Update qiskit/pulse/instructions/play.py Co-Authored-By: eggerdj <[email protected]> * Update qiskit/pulse/pulse_lib/sample_pulse.py Co-Authored-By: eggerdj <[email protected]> * Apply suggestions from code review Co-Authored-By: eggerdj <[email protected]> * Documentation improvements * Test change because linux python35 is failing * Update releasenotes/notes/unify-instructions-and-commands-aaa6d8724b8a29d3.yaml Co-Authored-By: eggerdj <[email protected]> * Fix plotting Play instruction * style, line length * Attempt to fix failing test by comparing names directly * Handle all instruction types in add_instruction * Remove unused import * Update releasenotes/notes/unify-instructions-and-commands-aaa6d8724b8a29d3.yaml Co-Authored-By: Matthew Treinish <[email protected]> * Separate note about Play feature into two notes: Play feature and Pulse movement Co-authored-by: eggerdj <[email protected]> Co-authored-by: Matthew Treinish <[email protected]> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.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
Part 6/7 for issue #3750
Migrate commands to instructions, migrate waveforms to
pulse_libDetails and comments
Deprecate PulseCommands, create new Play Instruction-implementation. The Play instruction takes a Pulse and a PulseChannel. Pulse is a new class within the pulse library that is very close to the deprecated PulseCommand abstract class. The PulseCommand implementations (SamplePulse and the ParametricPulse subclasses) have been moved to the pulse library as well, subclassing from Pulse, and all valid arguments to Play.
TODO
namesassemble_schedulesneeds to handlePlaypulse_instructionneeds to handlePlay