[RFC] Qiskit Pulse: Relative timing for Schedules #16

lcapelluto · 2020-02-20T21:02:19Z

Add an RFC proposing an implementation for relatively-timed instructions in pulse Schedules within Qiskit Pulse

…ons in pulse Schedules within Qiskit Pulse

… thoughts

0000-qiskit-pulse-schedule-relative-timing.md

taalexander · 2020-02-26T15:45:17Z

0000-qiskit-pulse-schedule-relative-timing.md

+
+
+## Summary
+Update the internal structure of `Schedule`s to track instructions sequentially. `Instruction`s will not be played at an explicit time; rather, they will begin immediately after the last is done. To schedule idle time between instructions, `Delay` instructions must be used.


Suggested change

Update the internal structure of `Schedule`s to track instructions sequentially. `Instruction`s will not be played at an explicit time; rather, they will begin immediately after the last is done. To schedule idle time between instructions, `Delay` instructions must be used.

Update the internal structure of `Schedule`s to track instructions sequentially. `Instruction`s will not be scheduled to be played at an explicit timestamp; rather, they will begin immediately after all channels on which the instruction effects are idle, ie., when instruction scheduling will be determined by the instruction dependency graph across channels. To schedule idle time between instructions, `Delay` instructions must be used.

0000-qiskit-pulse-schedule-relative-timing.md

taalexander · 2020-02-26T16:28:58Z

0000-qiskit-pulse-schedule-relative-timing.md

+| Method        | Runtime        |  Change          |
+|---------------|----------------|------------------|
+| `append(self, schedule)` | O(\|N<sub>new</sub>\|) |  |
+|`insert(self, time, schedule)`| O(\|N<sub>new</sub>\| * \|N<sub>C</sub>\|) |  |


How do you expect inserting in the middle of a schedule work? Verify that delays are present for all insertion locations?

yes. it's not great, since delays are supposed to be blocking, but I don't think there's any other way to do this

taalexander · 2020-02-26T16:43:02Z

0000-qiskit-pulse-schedule-relative-timing.md

+
+## Questions
+
+ 1. Should we create `insert(self, index, channel)`?


Isn't this handled by the normal insert, I don't see what is different about this?

the normal insert takes time, this takes index. I don't really see how this index method would be useful myself

0000-qiskit-pulse-schedule-relative-timing.md

CLAassistant · 2020-03-03T16:47:04Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

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

…com:lcapelluto/rfcs into terra-3749-relatively-timed-pulse-schedules

itoko · 2020-03-25T06:57:06Z

I realized we are using "relative time/timing" in the two meanings: "timing by channels (by-channels)" and "no start_time, no t=0", i.e. "absolute time/timing" means "timing across channels (across-channels)" and "having start_time from t=0".
For example,

`Instruction`s will not be played at an explicit time; rather, they will begin immediately after the last is done.

is talking about "timing by channels", while

explicit timing of instructions will become confusing with the introduction of control-flow to quantum programs as it would become difficult to program experiments containing loops that are not unrollable.

is talking about "no start_time". These two points may be considered independently.

Regarding "by-/across-channels",
I think we have two options:

Build and save with "time counting by channels" (as of the current RFC)
Build with "across-channels" and save with "by-channels"

I guess we are heading to "by-channels" because real devices do not support communication among channels (right?). That makes "timing across channels" too much for real device.
So I don't disagree with heading "timing by channels" for Schedule (as a data class to be passed to backend) if communication among channels would never be supported, but I think we need to be careful when thinking about schedule builder (as a class for users to build pulse schedule). At the time building schedules, we (often?) want to do insert(time, instruction). So we may need to have intermediate (mutable) class to build schedule supporting insert(time, instruction).
IMO, current Schedule class is doing two things, building and storing, as like QuantumCircuit. While QuantumCircuit perform well because it's mutable, it is difficult for Schedule to perform well because it's immutable. I thought that's why we're going to introduce "builder class" for Schedule as like "StringBuilder" and "String" class in Java. Am I misunderstanding schedule builder?

Regarding "start_time from t=0" considering control-flow,
we can think of another option (I don't say better option).
What if we introduce a kind of "(control-flow) block" and "no t=0" among blocks but having "t=0" for each block?

0000-qiskit-pulse-schedule-relative-timing.md

taalexander · 2020-03-31T18:54:38Z

0000-qiskit-pulse-schedule-relative-timing.md

-| `append(self, schedule)` | O(\|N<sub>new</sub>\|) |  |
-|`insert(self, time, schedule)`| O(\|N<sub>new</sub>\| * \|N<sub>C</sub>\|) |  |
+| `append(self, schedule)` | O(\|C\|) | Previously, `append` would add `schedule` to `self` with relative times preserved, at time set by `self.duration({schedule.channels}.intersection({self.channels}))`. The new method will simply extend the instructions in `self` with those in `schedule` on a per channel basis. |
+|`insert(self, time, schedule)`| O(N<sub>new</sub> * N<sub>C</sub>) | This will be slow generally, but it can be implemented with the same runtime as `append` when the `time` is greater than `self.duration(schedule.channels)`. |


Perhaps we should have both an insert_at_time and an insert (as the actual argument position in the schedule data structure) to properly differentiate these two operations?

sounds good to me

0000-qiskit-pulse-schedule-relative-timing.md

lcapelluto · 2020-04-02T00:42:40Z

@itoko I don't understand your comment; I don't understand what you mean by "cross channels"

0000-qiskit-pulse-schedule-relative-timing.md

ajavadia · 2020-04-02T02:38:03Z

0000-qiskit-pulse-schedule-relative-timing.md

+{
+    DriveChannel(0):
+        ('instructions'=[Delay(duration=10, ...),
+                         Play(duration=100, ...),
+                         Play(duration=20, ...)],
+         'duration'=130),
+     DriveChannel(1):
+        ('instructions'=[Delay(duration=110, ...),
+                         Play(duration=20, ...)],
+         'duration'=130),
+    ...
+}


This seems to be the main takeaway of this RFC, so we should settle on it.

So the 130 number is here for performance right? Even though it could be computed by summing all durations on that channel. I'm good with that.

However, we should think about what a conditional will look like. If I don't know an instruction's delay, due to a branch or conditional, how does this datastructure work?

A Schedule is a "basic block", so conditionals should live on top of this. An alt approach (which I should list in this RFC) would be to make this explicit and allow absolute timing within a basic block even when we have control flow. I find that misleading (too "special case"-y), but, at the heart of it, they're equivalent.

I think Thomas' new RFC touches on this

ajavadia · 2020-04-02T20:17:23Z

I'm not totally clear on this justification. Can you clarify the complexity of union with absolute and relative datastructures? I don't think it's different with the currently proposed structure, but I could be wrong.

It also encourages inefficient operations such as taking the union of two schedules. At first glance this operation should be efficient since all one needs to do is specifying the starting time of the instructions, but this requires verification that no instructions overlap in time in the two schedules resulting in expensive checks. The requirement for such a check to merge two schedules is obvious within the relatively schedules model, as one must look to make sure a delay exists in the first schedule for all instructions of the second schedule to merge.

ajavadia · 2020-04-03T00:35:02Z

Can we list the types of operations that we do in schedules (append, insert, delete/replace, union, find channel idle times) and the complexity of those in a relative vs. absolute data structure? I think for the basic blocks we should choose what makes compiler manipulation faster. I agree they can be converted to each other, but their manipulation is not the same.

…

On Apr 2, 2020, at 5:22 PM, Lauren Capelluto ***@***.***> wrote: @lcapelluto commented on this pull request. In 0000-qiskit-pulse-schedule-relative-timing.md: > +{ + DriveChannel(0): + ('instructions'=[Delay(duration=10, ...), + Play(duration=100, ...), + Play(duration=20, ...)], + 'duration'=130), + DriveChannel(1): + ('instructions'=[Delay(duration=110, ...), + Play(duration=20, ...)], + 'duration'=130), + ... +} I think Thomas' new RFC touches on this — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

lcapelluto · 2020-04-03T19:57:17Z

@ajavadia I agree. I'll do that 👍

remove ref to removed section Co-Authored-By: Ali Javadi-Abhari <ajavadia@users.noreply.github.com>

ajavadia · 2020-04-22T05:00:03Z

Ok so it based on this, append and shift are faster in relative, insert faster in absolute?

If I compare this with #18 it seems the findings are different in the case of insert. Can we come to an agreement on this @lcapelluto @itoko? Because I think the timed circuit and pulse case should be pretty identical in terms of datastructure and manipulation runtime.

In any case I just wanted to make sure we are basing this on some concrete runtime analysis, which will also help in the implementation. I think it is clear that relative wins in terms of the most common operation which is append.

Add an RFC proposing an implementation for relatively-timed instructi…

ab323ec

…ons in pulse Schedules within Qiskit Pulse

taalexander mentioned this pull request Feb 20, 2020

Pulse instructions should be scheduled with delays Qiskit/qiskit#3749

Closed

2 tasks

Fixup some formatting, rearrange a numbered list, write out some more…

8e9d16f

… thoughts

lcapelluto force-pushed the terra-3749-relatively-timed-pulse-schedules branch from 96d81f7 to 3cdf72f Compare February 21, 2020 21:21

Updates to notation

9876d18

lcapelluto force-pushed the terra-3749-relatively-timed-pulse-schedules branch from 3cdf72f to 9876d18 Compare February 21, 2020 21:23

Remove question I was wrong about

95ccc84

lcapelluto force-pushed the terra-3749-relatively-timed-pulse-schedules branch from 39d6653 to 95ccc84 Compare February 21, 2020 21:30

taalexander requested changes Feb 26, 2020

View reviewed changes

taalexander changed the title ~~Qiskit Pulse: Relative timing for Schedules~~ [WIP][RFC] Qiskit Pulse: Relative timing for Schedules Feb 26, 2020

taalexander changed the title ~~[WIP][RFC] Qiskit Pulse: Relative timing for Schedules~~ [WIP] [RFC] Qiskit Pulse: Relative timing for Schedules Feb 26, 2020

lcapelluto and others added 5 commits March 11, 2020 15:29

Fill in some more sections

ba587bb

small update

ae371ed

Update 0000-qiskit-pulse-schedule-relative-timing.md

1048410

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

Small updates

9239dd8

Merge branch 'terra-3749-relatively-timed-pulse-schedules' of github.…

be1c300

…com:lcapelluto/rfcs into terra-3749-relatively-timed-pulse-schedules

lcapelluto marked this pull request as ready for review March 11, 2020 19:40

lcapelluto changed the title ~~[WIP] [RFC] Qiskit Pulse: Relative timing for Schedules~~ [RFC] Qiskit Pulse: Relative timing for Schedules Mar 11, 2020

taalexander requested changes Mar 31, 2020

View reviewed changes

Remove additional schedule methods, because those belong to the builder

576c476

ajavadia reviewed Apr 2, 2020

View reviewed changes

lcapelluto and others added 3 commits April 10, 2020 14:56

Add runtimes for current implementation of append and insert

f06384c

Add shift runtimes

b8a84c0

Update 0000-qiskit-pulse-schedule-relative-timing.md

ef9a890

remove ref to removed section Co-Authored-By: Ali Javadi-Abhari <ajavadia@users.noreply.github.com>

lcapelluto closed this May 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] Qiskit Pulse: Relative timing for Schedules #16

[RFC] Qiskit Pulse: Relative timing for Schedules #16

lcapelluto commented Feb 20, 2020

taalexander Feb 26, 2020

taalexander Feb 26, 2020

lcapelluto Mar 11, 2020

taalexander Feb 26, 2020

lcapelluto Mar 11, 2020

CLAassistant commented Mar 3, 2020

itoko commented Mar 25, 2020 •

edited

Loading

taalexander Mar 31, 2020

lcapelluto Apr 2, 2020

lcapelluto commented Apr 2, 2020

ajavadia Apr 2, 2020

lcapelluto Apr 2, 2020

lcapelluto Apr 2, 2020

ajavadia commented Apr 2, 2020

ajavadia commented Apr 3, 2020 via email

lcapelluto commented Apr 3, 2020

ajavadia commented Apr 22, 2020



		## Summary
		Update the internal structure of `Schedule`s to track instructions sequentially. `Instruction`s will not be played at an explicit time; rather, they will begin immediately after the last is done. To schedule idle time between instructions, `Delay` instructions must be used.


		## Questions

		1. Should we create `insert(self, index, channel)`?

[RFC] Qiskit Pulse: Relative timing for Schedules #16

[RFC] Qiskit Pulse: Relative timing for Schedules #16

Conversation

lcapelluto commented Feb 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CLAassistant commented Mar 3, 2020

itoko commented Mar 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcapelluto commented Apr 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajavadia commented Apr 2, 2020

ajavadia commented Apr 3, 2020 via email

lcapelluto commented Apr 3, 2020

ajavadia commented Apr 22, 2020

itoko commented Mar 25, 2020 •

edited

Loading