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.
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
Pulse reference mechanism #8005
Pulse reference mechanism #8005
Changes from 47 commits
1ca9667
5618d83
cb8b832
3ccb1d1
2960d62
be48b5b
cf3c0d4
a66f3cd
3c99fa3
372b684
8cc3716
9d3c020
b4a90ad
7a08296
220a6f1
944076c
6d87410
021577e
44d4959
f92fa65
6871a4f
12d0870
b14ee6c
118004c
f24745a
670b333
d4ae163
04a656e
ad60789
620e67d
b59bbac
fe65c1a
329c6bc
ae506e8
f483456
5b7eeef
7a19c58
c973d61
f579889
1531480
fe6fd7f
cdefe16
90334dc
ffd2130
0fc4705
6ca8cfb
a16a596
ba531c7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
What is the status of Call and ScheduleBlock after this change? Before, one could use Call on a ScheduleBlock. I think this PR does not block that case but changes most of the convenient ways of a calling a ScheduleBlock to substitute in a reference instead.
I think the justification for doing this is that the reference mechanism allows scoping of parameters and deduplication of multiple calls of the same subroutine. I think that is reasonable, though I still wonder about leaving the call interface alone and making the reference interface separate, so one needs to choose to use it.
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'm thinking to remove the use case of calling block as a
Call
instruction. I don't think there are many users directly building a pulse program without builder syntax, so this will be kind of internal mechanism update.The reason
Call
is introduced is to improve memory efficiency at program construction and serialization. ACall
instance separately stores parameter-unassigned schedule and parameter table, and it is intended to reuse unassigned schedule among multiple instances. However, it is difficult to perform deduplication of schedules stored in separate instruction instances, on the other hand, reference mechanism easily realizes this because subroutine is now stored in the schedule block.Reference mechanism doesn't separately keep parameter-unassigned schedule, but I don't have any use case in mind that requires assignment of different parameters to the same parameterized sequence within the same schedule (i.e. Rabi experiment scans pulse amp, but each scan is different schedule instance). Indeed, current
Call
just degrades memory efficiency by having two schedule instances (one unassigned and other assigned one in cache) within the instance.If we keep calling the schedule block, then called schedule is not recognized as a reference. If a program has mixed representation of call and reference, it seems like we can easily run into some edge case.
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.
This seems somewhat unusual to me - it feels like you have name shadowing if the name is explicitly given, and not (and an unknowable lookup key for the user?) if it's not given. I admittedly don't fully understand the context, but it feels cleaner if shadowing either always happens, or is always an error?
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 always using shadowing is probably best. You need to use shadowing when the name is not given because a schedule could call multiple schedules with the same name (e.g. a two qubit schedule calling x on each qubit). While using the name as the key feels intuitive, it's not needed to preserve the way
call()
worked before.One issue with the current behavior is that if you use the same name for multiple calls with different schedules (which does not seem like a good thing to do) the last one overwrites the earlier ones whereas before the individual schedules would be kept.
Besides backwards compatibility, the nice thing about
call()
is that adds a reference and assigns it in a single step. Maybe the newreference()
method could optionally take aScheduleBlock
and assign it if given? Since it is a new method, it does not have to worry about backwards compatibility and can just always use the requiredname
parameter as the key.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 we used opposite meanings of "shadowing" here - I meant "overwrites" (so the previous definitions of "x" are inaccessible now), and I think you meant "adds to"? It doesn't matter too much if we did - I don't have any position on what's best for the API here, because I've never used the code. As long as you and Naoki come to an agreement on what's best, I can sign off on the PR. If that's the extra uuid bit of the key in all cases, or always making it an error, or always silently overriding, any of those are absolutely fine by me.
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.
Yes, you are right. I got it backwards from you. I was thinking of the function obscuring the subroutine's name as shadowing the name.
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.
This is good point. I'm not 100 percent confident on the current implementation. This at least guarantees the conventional behavior. The reference key is important only when a user assigns a subroutine to reference. Since,
.call
performs this operation in a single function, basically the reference key can be whatever. There is no reason to update explicitly providedname
, but I added uuid here to avoid local parameter collision since we can call.call
with parameter assignment. Remember that reference manager does NOT allow assigning multiple objects to a single reference. Calling the same schedule with different parameter assignments will generate different subroutines with the same name on the fly, and this operation usually fails without shadowing. This should be supported for backward compatibility. I wanted to generate unique name considering parameters, but I needed to give up because of mutability of.assign_parameters
(I needed unnecessarily heavy logic to realize). Another option would beprobably this makes more sense since
subroutine.name
is no longer the effective identifier due to uuid.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.
The key also factors into the new parameter scoping features. If you call a schedule without passing a name, you will get difficult to use scoped parameter names.
One thing I wanted to check -- do you want to support calling different schedules with the same name passed explicitly? It sounds like the reference manager will raise an exception for the second schedule now whereas before the PR it was allowed to use the same name since the name was just attached to the instruction and not inserted into a shared namespace.
The problem with this is that the first key gets used for
Instruction.name
so it is probably best to keep that a readable string.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.
Yes, this changes the behavior. I updated API doc and release note accordingly in ffd2130
These comments all make sense. I leave the current implementation.
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.
Is there a reason to allow the user to write a subroutine with a naming clash between its parameters? Seems a bit odd.
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 can't think of why you would set out to write a schedule like the example, but I could see it coming up in more complex code, like calling different functions to generate different parts of a schedule or using references to different schedules and then using
inline_subroutines
to merge them into one big schedule block.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.
That's totally fine by me, if there's a reason. I really don't know any of the context - I was basically just reviewing by "smell".
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.
Yeah basically this kind of problem occurs because the pulse module uses
Parameter
object designed for circuit as-is. Usually pulse parameter is scoped by pulse name (this allows user to safely have the same name in a schedule, i.e., sequence of a pulse), and thus UUID mechanism is not necessary. In pulse programming, it will never happen to have the same parameter name in the same pulse. So proper model should beParameter(scope, name)
, rather thanParameter(name, uuid)
.