Command/Instruction unification #3862

lcapelluto · 2020-02-18T21:29:51Z

Summary

First part of #3750

Begin command and instruction unification within Pulse. Instructions will migrate into the new instructions submodule of pulse. This begins by moving only the Instruction class. The Instruction command attribute is deprecated in favor of operands, and references to commands are updated.

This is the first of a series of 7 (anticipated) PRs to implement this migration. All other PRs depend on this one.

TODO:

Get details from Thomas about operands
Release notes

Details and comments

https://github.com/Qiskit/rfcs/pull/12/files?short_path=990963a#diff-990963ad1ab66f8d85311258e904239a

For the reviewer: I made comments near the functions in instructions.py that are altered from the original file.

…will migrate into the new instructions submodule of pulse. This begins by moving only the Instruction class. The Instruction ``command`` attribute is deprecated in favor of ``operands``, and references to commands are updated.

…ommands/__init__ can have a deprecation warning after the rest is implemented

qiskit/pulse/instructions/instruction.py

… now just 'duration', and any other operands only exist for an instruction instance

Fix up hash function

releasenotes/notes/unify-instructions-and-commands-aaa6d8724b8a29d3.yaml

qiskit/pulse/instructions/instruction.py

taalexander

This is looking good! I've noted a couple of things I might change, but I am of course open to discussion 😄 .

qiskit/pulse/instructions/__init__.py

qiskit/pulse/instructions/instruction.py

Co-Authored-By: Thomas Alexander <thomasalexander2718@gmail.com>

…e operands

taalexander

LGTM! A good start! Looking forward to seeing the rest.

mtreinish

LGTM, just some comments from a quick skimming of the release notes

mtreinish · 2020-02-25T15:01:13Z

releasenotes/notes/unify-instructions-and-commands-aaa6d8724b8a29d3.yaml

@@ -0,0 +1,22 @@
+---
+prelude: >


This should probably be either feature or upgrade (probably feature)

My idea was this one prelude message and then the following 6 PRs would be a list of upgrades with specific details, for example DelayInstruction(Delay(time), channel) => Delay(time, channel)

But I'll defer to your opinion

mtreinish · 2020-02-25T15:02:18Z

releasenotes/notes/unify-instructions-and-commands-aaa6d8724b8a29d3.yaml

+    There has been a significant simplification to the style in which Pulse
+    instructions are built.
+
+    With the previous style, ``Command`` s were called with channels to make


Do you want to use:

Suggested change

With the previous style, ``Command`` s were called with channels to make

With the previous style, :class:`qiskit.pulse.Command`s were called with channels to make

here and in other places were you reference a class?

Not for Command because it wants to disappear

Instruction should though

…om:lcapelluto/qiskit-terra into issue-3750-unify-commands-and-instructions

mtreinish

LGTM, thanks for updating the release note

…om:lcapelluto/qiskit-terra into issue-3750-unify-commands-and-instructions

* Begin command and instruction unification within Pulse. Instructions will migrate into the new instructions submodule of pulse. This begins by moving only the Instruction class. The Instruction ``command`` attribute is deprecated in favor of ``operands``, and references to commands are updated. * Remove deprecation warning from commands/instruction.py because the commands/__init__ can have a deprecation warning after the rest is implemented * Revert commands/__init__ because changes there are not necessary. * Fill in first positional argument in instruction. What was command is now just 'duration', and any other operands only exist for an instruction instance * Continue adding documentation and notes Fix up hash function * Fix type hint from List[Channel] to Channel * Update repr to match new init signature * Update qiskit/pulse/instructions/__init__.py Co-Authored-By: Thomas Alexander <thomasalexander2718@gmail.com> * Update init file for the instruction module to be more clear about the operands * Make Instruction an ABC * update instruction comment * Update docs * Update year in copyright header * Revert Instruction as ABC because the Instructions which haven't been migrated yet do not support the abstractmethods * Update __eq__ to support both styles until migration is complete * Update __hash__ to support both styles until migration is complete * Fixup style * Update TODO comment * Use self.command instead of init arg if the first arg is a command, for readability * Make repr unchanged for nonmigrated commands * Small bugfix * Update instructions documentation * Relax requirement for channel operand * API reference to Instruction in release notes * Fix releasenote * Silly reno style Co-authored-by: Thomas Alexander <thomasalexander2718@gmail.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

lcapelluto assigned taalexander Feb 18, 2020

lcapelluto requested a review from taalexander February 18, 2020 21:30

lcapelluto mentioned this pull request Feb 18, 2020

[RFC] Unify Pulse Commands and Instructions Qiskit/RFCs#12

Merged

lcapelluto force-pushed the issue-3750-unify-commands-and-instructions branch from ebccc75 to 409d94b Compare February 18, 2020 21:39

lcapelluto added 2 commits February 18, 2020 16:46

Remove deprecation warning from commands/instruction.py because the c…

7a26499

…ommands/__init__ can have a deprecation warning after the rest is implemented

Revert commands/__init__ because changes there are not necessary.

0744b6f

itoko reviewed Feb 19, 2020

View reviewed changes

qiskit/pulse/instructions/instruction.py Outdated Show resolved Hide resolved

itoko reviewed Feb 19, 2020

View reviewed changes

qiskit/pulse/instructions/instruction.py Outdated Show resolved Hide resolved

lcapelluto assigned ajavadia and unassigned ajavadia Feb 19, 2020

lcapelluto requested a review from ajavadia February 19, 2020 16:41

lcapelluto added 2 commits February 19, 2020 13:14

Fill in first positional argument in instruction. What was command is…

d8cd97e

… now just 'duration', and any other operands only exist for an instruction instance

Continue adding documentation and notes

205c493

Fix up hash function

lcapelluto marked this pull request as ready for review February 19, 2020 20:28

lcapelluto requested review from 1ucian0, DanPuzzuoli, eggerdj, kdk, mtreinish and nkanazawa1989 as code owners February 19, 2020 20:28

Fix type hint from List[Channel] to Channel

7f2137f

lcapelluto commented Feb 19, 2020

View reviewed changes

releasenotes/notes/unify-instructions-and-commands-aaa6d8724b8a29d3.yaml Show resolved Hide resolved

Update repr to match new init signature

4f0ffeb

lcapelluto commented Feb 19, 2020

View reviewed changes

qiskit/pulse/instructions/instruction.py Show resolved Hide resolved

lcapelluto commented Feb 19, 2020

View reviewed changes

qiskit/pulse/instructions/instruction.py Outdated Show resolved Hide resolved

lcapelluto commented Feb 19, 2020

View reviewed changes

qiskit/pulse/instructions/instruction.py Show resolved Hide resolved

taalexander requested changes Feb 20, 2020

View reviewed changes

lcapelluto and others added 2 commits February 20, 2020 09:02

Update qiskit/pulse/instructions/__init__.py

172d667

Co-Authored-By: Thomas Alexander <thomasalexander2718@gmail.com>

Update init file for the instruction module to be more clear about th…

94c962c

…e operands

lcapelluto added 3 commits February 24, 2020 09:02

Update instructions documentation

560ea27

Relax requirement for channel operand

715504b

Merge branch 'master' into issue-3750-unify-commands-and-instructions

3292e74

taalexander previously approved these changes Feb 25, 2020

View reviewed changes

mtreinish reviewed Feb 25, 2020

View reviewed changes

lcapelluto added 2 commits February 25, 2020 11:09

API reference to Instruction in release notes

c9584a3

Merge branch 'issue-3750-unify-commands-and-instructions' of github.c…

9465bf4

…om:lcapelluto/qiskit-terra into issue-3750-unify-commands-and-instructions

lcapelluto dismissed taalexander’s stale review via 9465bf4 February 25, 2020 16:50

Fix releasenote

aea611b

lcapelluto requested review from mtreinish and taalexander February 25, 2020 18:21

mtreinish previously approved these changes Feb 25, 2020

View reviewed changes

Merge branch 'master' into issue-3750-unify-commands-and-instructions

a29bb01

taalexander previously approved these changes Feb 25, 2020

View reviewed changes

taalexander added the automerge label Feb 25, 2020

taalexander added this to To do in Pulse via automation Feb 25, 2020

lcapelluto added 2 commits February 25, 2020 16:13

Silly reno style

1476d92

Merge branch 'issue-3750-unify-commands-and-instructions' of github.c…

0bc4163

…om:lcapelluto/qiskit-terra into issue-3750-unify-commands-and-instructions

lcapelluto dismissed stale reviews from taalexander and mtreinish via 0bc4163 February 25, 2020 21:14

mtreinish approved these changes Feb 25, 2020

View reviewed changes

mergify bot added 3 commits February 26, 2020 02:16

Merge branch 'master' into issue-3750-unify-commands-and-instructions

64cbaa4

Merge branch 'master' into issue-3750-unify-commands-and-instructions

db4f151

Merge branch 'master' into issue-3750-unify-commands-and-instructions

0b0e1a9

taalexander approved these changes Feb 26, 2020

View reviewed changes

mergify bot merged commit 12b07bc into Qiskit:master Feb 26, 2020

Pulse automation moved this from To do to Done Feb 26, 2020

lcapelluto added the Changelog: API Change Include in the "Changed" section of the changelog label Apr 1, 2020

1ucian0 added the mod: pulse Related to the Pulse module label Jan 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Command/Instruction unification #3862

Command/Instruction unification #3862

lcapelluto commented Feb 18, 2020 •

edited

Loading

taalexander left a comment

taalexander left a comment

mtreinish left a comment

mtreinish Feb 25, 2020

lcapelluto Feb 25, 2020

lcapelluto Feb 25, 2020

mtreinish Feb 25, 2020

lcapelluto Feb 25, 2020

lcapelluto Feb 25, 2020

mtreinish left a comment

	With the previous style, ``Command`` s were called with channels to make
	With the previous style, :class:`qiskit.pulse.Command`s were called with channels to make

		@@ -0,0 +1,22 @@
		---
		prelude: >

Command/Instruction unification #3862

Command/Instruction unification #3862

Conversation

lcapelluto commented Feb 18, 2020 • edited Loading

Summary

Details and comments

taalexander left a comment

Choose a reason for hiding this comment

taalexander left a comment

Choose a reason for hiding this comment

mtreinish left a comment

Choose a reason for hiding this comment

mtreinish Feb 25, 2020

Choose a reason for hiding this comment

lcapelluto Feb 25, 2020

Choose a reason for hiding this comment

lcapelluto Feb 25, 2020

Choose a reason for hiding this comment

mtreinish Feb 25, 2020

Choose a reason for hiding this comment

lcapelluto Feb 25, 2020

Choose a reason for hiding this comment

lcapelluto Feb 25, 2020

Choose a reason for hiding this comment

mtreinish left a comment

Choose a reason for hiding this comment

lcapelluto commented Feb 18, 2020 •

edited

Loading