feat!: #334: program stats

[BatmanAoD]

Closes #334

[github-actions]

PR Preview Action v1.4.7
Preview removed because the pull request was closed.
2024-03-02 00:29 UTC

[kalzoo]

• many of these methods only make sense within the context of a block. They only appear to make sense on a Program when that program consists of one block.

These should be implemented on a block, and then perhaps implemented as well on a program with an error about multiple blocks when present. Like ExecutionGraph, this can also be computationally expensive and could be done lazily and cached, or eagerly on construction.

• I see a todo about testing the path_fold. I’ll have to review that on my next pass. There’s also some commented-out code with unimplemented methods. Fine to defer those to later MR; there’s already an issue for the duration one.

[kalzoo]

I'll give this a self-review next week, but it's feature-complete and ready for review by others. Please pay particular attention to the python bindings.

[kalzoo]

added documentation to the python bindings with a usage example

[kalzoo]

self-review complete. ready for team review.

[BatmanAoD]

Why would zero-duration instructions cause "unexpectedly long" durations?

[kalzoo]

I've been trying to think of how to illustrate this in the docs.

namely, on calibration expansion, one instruction is decomposed into many. Those decomposed instructions will now interact with other decomposed instructions in a way that "mix" the two original gates in time. A FENCE "plays" at the first clock tick following any preceding instructions on its qubits; that may not immediately precede the PULSE instruction whose timing the user actually cares about (for example).

[BatmanAoD]

So zero-duration instructions can introduce new dependencies between previously-independent instructions?

[BatmanAoD]

Some questions and a few small suggestions, but I don't think anything is blocking (unless I've identified an actual bug in duration(), but I think I just don't understand what's happening there).

[mhodson-rigetti]

At a high level, I'd be looking for this facility to provide the following:

• For pulses, the start and end time and possibly the associated waveform (sharing the memory);
• For delays, the inferred start time of the delay, constraining the duration to the delay time specified by the user;
• For fences, the time at which the barrier applies; this is a point-in-time operation and should have zero duration.

I think there is more to unpack here but I don't want to block an MR that already provides users with helpful debugging information. I do think breaking changes are possible in subsequent versions so I defer to you @kalzoo on the merge priority in the short term.

[mhodson-rigetti]

@kalzoo I finished a second, more thorough review of the API.

[mhodson-rigetti]

While technically correct, there's some patterns in this example which don't make much semantic sense, so I'd like to suggest a revision.

• A 0 plays a pulse on qubit 1, not 0?
• B 0 is actually a 2Q gate underlying? It would normally take 2 qubits?
• The 2Q gate only fences the one qubit needed to obviate the example? It would normally fence both.

# One-qubit frame
DEFFRAME 0 "a":
    ATTRIBUTE: 1

# Two-qubit frame
DEFFRAME 0 1 "b":
    ATTRIBUTE: 1

DEFCAL A 0:
    PULSE 0 "a" flat(duration: 1.0)

DEFCAL B 0 1:
    FENCE 0 1
    PULSE 0 1 "b" flat(duration: 1.0)

A 0
B 0 1

I would evict the explanation from the comments- nothing plays really in the declaration, and its nuanced in a way that depends on the instructions as much as the calibrations. See later comment.

[kalzoo]

true, good points

[mhodson-rigetti]

Updated example noted. All good.

[mhodson-rigetti]

The example you added is great, because I hadn't realized the implications of the include_zero_duration_instructions flag. I think it is a misnomer to consider FENCE to have zero duration. When writing the updated example above, I went to move the comments onto the individual instructions inside DEFCAL B 0 1 and wanted to write above the FENCE: "This instruction starts when the program starts at time zero, and then because of the pulse on the fenced frame, ends at time 1". So it would have a duration of 1. Which is not zero. Yet it is being excluded by this flag?

I am also concerned as this facility really only provides utility in a case where a calibration is started and ended with FENCE, those are considered "zero duration", and so it basically shows you the time span of the inner pulses. That is of interest, and this could be a common gate construction, but any more complicated multi-pulse gate with intervening FENCEs or DELAYs really leads to me wanting to see the time-span associated with the individual Quil-T commands, but in the context of the expanded instruction. This feature doesn't provide that- it only does a roll up- so it partially meets the need.

It feels like this flag should be removed. The detail you need to properly ascertain how FENCEs and DELAYs interact is not possible, and the common use case met is really just "tell me the time from the start of the first pulse to the end of the last pulse". Though, what you have implemented may could DELAY as a non-zero instruction, so it could (and probably should) include DELAYs. I don't think the case of users seeing the "unusually long gate times" could make any useful debugging sense, and it changes with the context the gate plays in, whereas in most cases the intra-gate pulse/delay schedule would be fixed (note: I can think of some esoteric cases where its not, but the fact you report per-invocation timing means that's not an issue).

I would therefore suggest to remove the flag, treat it as always False, simplify or remove the documentation around unexpectedly long gates (which attracted its own review questions), and then make the ability to tease apart these less common cases part of a follow-on where we expose per-Quil-T instruction timing in the context of the original instruction.

[kalzoo]

So it would have a duration of 1.

I don't agree with this - nothing plays in between the FENCE and its paired PULSE, but it "ends" at the same time it starts, 0, and that's how Quil scheduling interprets it as well (for instructions which depend on it). By definition it has no duration - it's just a marker between other elements on the timeline.

I don't think the case of users seeing the "unusually long gate times" could make any useful debugging sense

I agree, this may not, but someone (can't recall if it was you) recently specifically asked to "see when their PULSEs, CAPTUREs, and FENCEs were scheduled"), and this flag also affects top-level FENCEs within the pre-expansion program. If those are missing, I'll probably hear about it.

I agree that this is a coarse implementation though. As far as seeing the "member" instructions, yeah,we could (in another PR) expose the source mapping used to actually compute the gate-model schedule.

[kalzoo]

@mhodson-rigetti I'm confused about something in this thread after re-reading your original review on this PR:

1. it was you who had said that one of your success criteria for this was specifically seeing the FENCE timings:

For fences, the time at which the barrier applies

2. Then, you had also called out that FENCE is zero-duration:

this is a point-in-time operation and should have zero duration

---

I think this can be resolved by, as you say, expanding calibrations and returning the source mapping so that the user can trace back "expanded instruction time" to "original unexpanded instruction" for all instructions, and then decide how to handle that information themselves. I'll save that extension for the next PR.

[mhodson-rigetti]

True, I wasn't being very consistent. I'm imagining what I want to visualize with this data; for a FENCE, I would draw a barrier - a vertical line on a timeline - at the latest time of any frame involved in the fence. This is "the time of the fence" if you wanted to assign it a single time. However, from a frame tracking perspective, a given frame affected by a fence goes from whatever time it is at (e.g. the time the previous pulse finished) up to this barrier time. This is basically a delay, and it might be handy to have captured that explicitly. However the data structures might not support this, and giving a FENCE a duration like this only came up when I was tracking how I would explain your new example to someone- I'd be saying that the FENCE caused "b" frame to wait from 0 to 1. I will need to deconflict this viewpoint when I write the needs statement.

[mhodson-rigetti]

I like the tweaks you made to the example and documentation- simple and effective.

[kalzoo]

@mhodson-rigetti after some more thought on this, I'd like to omit the flag (as you suggested and as it is now) but instead turn the default behavior to include all instructions (even zero-duration) in the rolled-up timespan, because:

• While both ways can be surprising, treating all instructions equally is probably less surprising
• It suits the common case and thus will be less frequently surprising as well. In most cases, FENCE shouldn't lead or lag the calibration body as in the pathological example here
• It means that pre-expansion FENCEs will show up in the overall schedule
• It means that SET- and SHIFT- instructions will as well, which lets us treat this data structure as an IR of sorts
• I intend to give the user more control over the details (as in this comment thread), so they'll have the ability to decide on a per-instruction basis what gets "rolled up" soon anyway
• Finally, what swayed me: it means that this will work as you might expect for an already-expanded pulse program; every instruction in the program will get a time span. if we skip zero-duration, some will just be missing, to no benefit, and there's no way to "ask" for them to be included.

I'd leave the illustration here to help users identify this behavior when it does happen.

Does that check out with you?

[mhodson-rigetti]

Yeah I'm totally comfortable with that. Good call.

[mhodson-rigetti]

I feel like a little more explanation is required here; it took me a moment to deconflict concepts from the Wikipedia article on CFGs to what is happening in Quil. Each basic block can have multiple output arcs- in Quil it's probably limited to 2 (or 1 and it continues to the next block) and this happens only on conditional jump instructions? It would be worth saying that here, and it would be more semantically rich if there was a Quil control flow subclass of Instruction that was the return type here that helped you see it was only jump instructions.

[mhodson-rigetti]

Also, is this not by definition self.instructions[-1]? Or is the terminator excluded from instructions? If included, I can see why you would still have this method, because the naming provides a sort of semantic richness that is valuable (ie. same argument for having the "end" method on the span; its sort of canonical to the definition).

[kalzoo]

Also, is this not by definition self.instructions[-1]

Not if that instruction is not a terminator. The BasicBlockTerminator on the Rust side makes its possibilities more clear: Jump*, Halt, or implicit Continue.

edit: let me fix what I said - it is never self.instructions[-1], if it is a terminator, it is excluded.

[mhodson-rigetti]

Great. I see this is documented. All good.

[MarquessV]

One small question about potentially missing an export, but looks good to me!