-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat!: introduce ExecutionDependency::Scheduled #186
Conversation
Co-authored-by: Mark Skilbeck <mark.skilbeck@rigetti.com>
| Instruction::FrameDefinition(_) | ||
| Instruction::Gate(_) | ||
| Instruction::GateDefinition(_) | ||
| Instruction::Halt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A query on the spec; wouldn't a HALT have a precise timing? ie. you would know precisely when the program ends?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You make a good point, perhaps about the docs or choice of names.
Many instructions will have timing fixed at compile time, but whether a halt is fixed to time X or time Y does not materially affect the program, if we see its pulse sequence as essential to its purpose.
Put another way, depending on the semantics of a hardware provider, the HALT may be 10 ns or 10us or 10ms after the previous instruction, but that does not affect the readout results that one would expect to collect.
That’s in contrast to a PULSE, where durations and times may be statically (if parametrically) determined from the quil alone, and which do not depend on hardware provider implementation details.
How could I better communicate that, maybe in docs but ideally in the code itself?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the documentation. As a user, I never write HALT; it's always implicit, and if the hardware back end chooses to add it for some tracking of the type you describe, so be it. The place I would care is Quil-T based pulse timing analysis. One of the things we can do with Quil-T entirely on the user side is mimic pulse timing rules and determine the timing of all of the instructions ourselves. Here, we report total program duration as the end time of the last pulse. But, where HALT is in play (implicitly or explicitly) and has some real time duration as you describe, that real time should be accounted for (start time of HALT was the end time of the last pulse, but then there is some real end time to HALT that is greater?). Which brings me back to the point-- wouldn't you be able to tell me precisely what that time was? If the back end adds 10 microseconds, shouldn't I be able to see that somehow? Is this was "precise" means here, and what is quil-rs
's part to play in this workflow (it may be it is simply out of scope at this level).
This was a query of the semantics only, and not a practical issue, so I'm happy whatever you decide.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for that feedback, @mhodson-rigetti !
Following up from our discussion offline, I've renamed this method to is_scheduled
, and ExecutionDependency::Timing
to ExecutionDependency::Scheduled
.
For anyone following along - "precise timing" vs scheduling is a worthwhile semantic distinction here; only some instructions are considered for "scheduling" as part of the program, and their position within that schedule is essential to the program doing what the author intended. That is now what this function identifies.
All instructions, though, may have "precise times" fixed as part of compilation for a backend. The timing of non-scheduled instructions (like ADD
or MOVE
) is flexible within some bounds without affecting the output of the program.
Code looks neat and tidy; good test coverage. |
We're stuck with the CI failure until I'll ignore the failure for now and we can merge in the fix once available. It's a dev dependency in any case, so it does not impact consumers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional changes look good to me!
Closes #185
Introduces
ExecutionDependency::Timing
as described in the issue, with updated snapshot tests to reflect the change.