Pulse reference mechanism #8005

nkanazawa1989 · 2022-04-30T04:29:22Z

Summary

This PR adds reference mechanism to ScheduleBlock. This feature is necessary for template schedule construction in the Qiskit Experiments calibration module. This mechanism allows following workflow.

with pulse.build(name="main") as main_prog:
     pulse.reference("child", "q0")

with pulse.build() as child_prog:
   pulse.play(my_pulse, pulse.DriveChannel(0))

main_prog.assign_references({("child", "q0"): child_prog})

Now pulse programmers can reference an external program (subroutine) with arbitrary string tuples, i.e. reference keys. This tuple must be unique within the scope (i.e. same name cannot be used twice in the same builder context for calling different programs). Note that main_prog can be constructed without knowing realization of child. This allows us to decouple programs defined in the separate library. The main_prog can (will) be QPY serialized with/without reference assignment.

Details and comments

Clue for review

(1) Reference manager
New class ReferenceManager is introduced. The manager instance is stored in the schedule block instance. Referenced program is appended to the self._block as an instruction, and actual subroutine is added to the reference manager.

(2) Parameter management
In the schedule block ParameterManager instance is stored as self._parameter_manager. Parameter manager now only manages parameters defined in the current scope (i.e. it is not aware of parameters of called program). Instead, management of parameters is delegated to the manager of the referenced program. By recursively calling the manger of called program, it automatically propagates the parameter assignment through all referenced programs from the root program. New property self.scoped_parameters is added to return all parameters with scoped name, i.e. if parameter A is defined in the program B, parameter name becomes B.A. If this is further referenced from program C the name becomes C.B.A. Note that Parameter object is not sensitive to name (it is evaluated with UUID).

TODO

finalize design
add more tests
release note

…e of it with DAGSchedule class. Pulse builder call function is also updated to use reference mechanism. Now call function can be called without actual pulse program, which will be later assigned to the schedule through assign_reference method.

…reference dict. Note that channels can be parametrized, and such dynamic key may break reference during programming. Even though if we update key with parameter assignment, assigned key doesn't work because they are different object, i.e. key is evaluated with "is" thus object id is important. Rather than introducing complicated mechanism to manage this, static ref key provides robust and cleaner solution.

reference added to the program is managed with scope. this helps users to address parameters and reference to assign.

qiskit-bot · 2022-04-30T04:29:24Z

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

One or more of the the following people are requested to review this:

@Qiskit/terra-core

coveralls · 2022-04-30T04:53:16Z

Pull Request Test Coverage Report for Build 3053122782

334 of 377 (88.59%) changed or added relevant lines in 16 files are covered.
2 unchanged lines in 2 files lost coverage.
Overall coverage increased (+0.03%) to 84.376%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/pulse/instructions/instruction.py	12	13	92.31%
qiskit/pulse/instructions/play.py	6	7	85.71%
qiskit/pulse/instructions/snapshot.py	2	3	66.67%
qiskit/pulse/builder.py	32	34	94.12%
qiskit/pulse/parameter_manager.py	33	35	94.29%
qiskit/pulse/instructions/acquire.py	5	9	55.56%
qiskit/pulse/instructions/reference.py	28	32	87.5%
qiskit/pulse/reference_manager.py	14	28	50.0%
qiskit/pulse/schedule.py	143	157	91.08%

Files with Coverage Reduction	New Missed Lines	%
qiskit/pulse/builder.py	1	90.66%
qiskit/pulse/transforms/canonicalization.py	1	92.35%

Totals
Change from base Build 3053075546:	0.03%
Covered Lines:	58173
Relevant Lines:	68945

💛 - Coveralls

eggerdj · 2022-06-03T09:02:40Z

Here is some high level feedback based on a use case. The setting I am working with is the following:

from qiskit.circuit import Parameter
import qiskit.pulse as pulse

x_amp = Parameter("amp")
sigma = Parameter("s")
x_dur = Parameter("dur")
cr_amp = Parameter("amp")
cr_dur = Parameter("dur")
cr_width = Parameter("w")
c_chan_idx = Parameter("ch0")
u_chan_idx = Parameter("ch0.1")
c_chan = pulse.DriveChannel(c_chan_idx)
u_chan = pulse.ControlChannel(u_chan_idx)

with pulse.build(name="xp") as xp:
    pulse.play(pulse.Gaussian(x_dur, x_amp, sigma), c_chan)
    
with pulse.build(name="cr45p") as cr45p:
    pulse.play(pulse.GaussianSquare(cr_dur, cr_amp, width=cr_width, sigma=sigma), u_chan)
    
with pulse.build(name="cr45m") as cr45m:
    pulse.play(pulse.GaussianSquare(cr_dur, -cr_amp, width=cr_width, sigma=sigma), u_chan)
    
with pulse.build(name="ecr") as ecr:
    with pulse.align_sequential():
        pulse.call(name="cr45p", channels=[u_chan])
        pulse.call(name="xp", channels=[c_chan])
        pulse.call(cr45m)  # make things interesting by mixing ref. styles.
        pulse.call(name="xp", channels=[u_chan])

Standard assigning still works fine, e.g.

xp_bind_dict = {x_amp: 0.5, x_dur: 160, sigma: 40, c_chan_idx: 2}

xp.assign_parameters(xp_bind_dict, inplace=False).draw()

gives:

Things now start getting more complex when I try to resolve the references and assign parameters in the ECR schedule. Some extra complexity is expected but currently its seems very hard to resolve and assign. My strategy is to first assign parameters in the references (aside from channel indices) and then to assign the references in the ecr schedule.

xp_bind_dict = {x_amp: 0.5, x_dur: 160, sigma: 40}
cr_bind_dict = {cr_amp: 0.123, cr_dur: 540, cr_width: 320, sigma: 40}

ecr2 = ecr.assign_reference(ref_key="xp", schedule=xp.assign_parameters(xp_bind_dict, inplace=False), inplace=False)
ecr3 = ecr2.assign_reference(ref_key="cr45p", schedule=cr45p.assign_parameters(cr_bind_dict, inplace=False), inplace=False)

# After this we still need to assign parameters in cr45m because of our strange choice to mix ref styles.
bind_dict = {
    ecr3.get_parameters("s", scope="ecr.cr45m")[0]: 80,  # Also lose the coupling
    ecr3.get_parameters("dur", scope="ecr.cr45m")[0]: 540,
    ecr3.get_parameters("amp", scope="ecr.cr45m")[0]: -0.2,  # This means that we lose the coupling between cr45p and m
    ecr3.get_parameters("w", scope="ecr.cr45m")[0]: 320,
}

ecr4 = ecr3.assign_parameters(bind_dict, inplace=False)
try:
    ecr4.assign_parameters({c_chan_idx: 2, u_chan_idx: 14}).draw()
except pulse.exceptions.UnassignedDurationError as err:
    print(err.message)
    print(ecr4.parameters)  # Contradicts the error message.

produces:

All instruction durations should be assigned before creating `Schedule`.Please check `.parameters` to find unassigned parameter objects.
set()

I also have the following comments:

For consistency it would be nice to have assign_reference behave like the parameter assignment, i.e., assign_references({"xp": xp_sched, "cr45p": cr45p}, inplace=False)
This framework allows us to easily break links between parameter but I'm not sure there is a way around that.
It seems that the framework does not capture all unassigned parameters?

Note that assigning references and then parameters also seems to lead to a few complications

# Assign references
ecr2 = ecr.assign_reference(ref_key="xp", schedule=xp, inplace=False)
ecr3 = ecr2.assign_reference(ref_key="cr45p", schedule=cr45p, inplace=False)

# Build the binding dict
bind_dict = {
    ecr3.get_parameters("amp", scope="ecr.xp")[0]: 0.4,
    ecr3.get_parameters("dur", scope="ecr.xp")[0]: 160,  # no sigma in xp as it is duplicated with cr45p
    ecr3.get_parameters("s", scope="ecr.cr45p")[0]: 40,
    ecr3.get_parameters("dur", scope="ecr.cr45p")[0]: 540,
    ecr3.get_parameters("amp", scope="ecr.cr45p")[0]: 0.2,
    ecr3.get_parameters("w", scope="ecr.cr45p")[0]: 320,
    u_chan_idx: 14,
    c_chan_idx: 2
}

# This seems misleading as there are still parameters burried in the schedule
print(ecr3.assign_parameters(bind_dict, inplace=False).scoped_parameters)
print(ecr3.assign_parameters(bind_dict, inplace=False).parameters)

try:
    ecr3.assign_parameters(bind_dict, inplace=False).draw()
except pulse.exceptions.UnassignedDurationError as err:
    print(err.message)

gives

set()
set()
All instruction durations should be assigned before creating `Schedule`.Please check `.parameters` to find unassigned parameter objects.

Redefined ReferenceManager as a subclass of mutable mapping. This is the mapping to both schedule and channels. Parameter management is excluded from the reference and moved to the schedule block. All parameters including one in subroutines are exposed with .parameters property.

nkanazawa1989 · 2022-06-14T20:36:24Z

Thanks Daniel for nice test case! I updated and simplified the code to support your test case. As we discussed offline, management of parameter-link between different pulse instances is outside the scope of schedule block. This should be delegated to high-level management tools, such as calibrations. However, the scoping mechanism here should be useful to simplify the logic there; by using get_parameters with scope, calibrations doesn't need to be aware of parameters (and uuids), and it can manage parameters with scope + parameter name, given we assign unique name to every schedules.

Here is the functional updates:

assign_references takes a dictionary as in assign_parameters
.parameters return all parameters in nested subroutines

And there is an overhaul of ReferenceManager in a66f3cd. Now this object is a dict-like with pretty printing;

print(ecr4.references)

shows

ReferenceManager
  - scope: 'ecr'
  - # of references: 3
  - mappings:
    * 'cr45p': ScheduleBlock(Play(GaussianSquare(duration=540, am..., channels=[uch0.1]
    * 'xp': ScheduleBlock(Play(Gaussian(duration=160, amp=(0.5..., channels=[dch0]
    * 'cr45m': ScheduleBlock(Play(GaussianSquare(duration=540, am..., channels=[uch0.1]

which looks much better than before. In addition, you have direct access to subroutine with dict-access

ecr4.references["cr45p"]

I hope this alleviates the complexity.

eggerdj · 2022-06-16T13:26:01Z

Playing around with this a bit more. Feels already better after the update. One thing I noticed is the scoping delimiter is based on the . symbol and there is no protection against the case where a parameter name has a . in it. This leads to a bit of strange behavior. The case is the following (based on the test case above):

u_chan_idx = Parameter("ch0.1")
u_chan = pulse.ControlChannel(u_chan_idx)

...

with pulse.build(name="cr45p") as cr45p:
    pulse.play(pulse.GaussianSquare(cr_dur, cr_amp, width=cr_width, sigma=sigma), u_chan)

with pulse.build(name="ecr") as ecr:
    with pulse.align_sequential():
        pulse.call(name="cr45p", channels=[u_chan])
        ...

I can assign the references doing e.g.

ecr2 = ecr.assign_references({"xp": xp, "cr45p": cr45p}, inplace=False)

which seems to work fine. Now, I can print the scoped parameters with ecr2.scoped_parameters to get

{Parameter(ecr.ch0),
 Parameter(ecr.ch0.1),
 Parameter(ecr.cr45p.amp),
 Parameter(ecr.cr45p.dur),
 Parameter(ecr.cr45p.s),
 Parameter(ecr.cr45p.w),
 Parameter(ecr.xp.amp),
 Parameter(ecr.xp.dur)}

Here, we see that ecr.ch0.1 has a problem. What is the scope and what is the name. Indeed, I cannot get the parameter named ch0.1 if I do:

ecr2.get_parameters("ch0.1", scope="ecr")

instead I need to do:

ecr2.get_parameters("1", scope="ecr.ch0")

which is not really intended. Here, I see two ways out:

forbid . in parameter names which is unfortunate since we chose that for channels in calibrations.
Use lists to manage scopes.

I would prefer option 2. Here we could do

ecr2.get_parameters("amp", scope=("ecr", "cr45p"))

The only issue I see here is what parameter names do we use in ecr2.scoped_parameters? Or perhaps ecr2.scoped_parameters could return

{
 ("ecr", ): Parameter(ch0),
 ("ecr", ): Parameter(ch0.1),
 ("ecr", "cr45p"): Parameter(amp),
 ("ecr", "xp"): Parameter(amp),
 ...
}

eggerdj

This looks very good. From a usability perspective I have some concerns about the . in the scoping. Would tuples of names work? This would avoid having to reserve . as a special character.

qiskit/pulse/builder.py

qiskit/pulse/schedule.py

test/python/pulse/test_builder.py

wshanks

Here is some initial review.

I want to think a bit more about some of the big picture of what is going on here -- how the scoping system works and how it is likely to be used in practice. The changes here do a lot more than the placeholder system I had originally been thinking of (not a bad thing).

One thing I would still like to look at more is the requirement that the reference include channels. The use case I envisioned for a referenced schedule would work better without needing to specify channels. To be a full Schedule/ScheduleBlock channels are needed but for the early specification of a schedule block calling another schedule block I don't think they are. With the way you make the central data structure a DAG here they become necessary but when ScheduleBlock was fundamentally a list of blocks I think not having channels could work (some things would not work but you wouldn't want those things until you filled in the reference with something with channels which typically would happen very early in the lifecycle of a ScheduleBlock with a reference).

wshanks · 2022-06-17T18:24:47Z

qiskit/pulse/builder.py

+        sub_name = name or subroutine.name
+
+        if isinstance(subroutine, ScheduleBlock):
+            # If subroutine is schedule block, use reference mechanism.


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.

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. A Call 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.

qiskit/pulse/builder.py

qiskit/pulse/instructions/call.py

qiskit/pulse/reference_manager.py

qiskit/pulse/schedule.py

qiskit/pulse/utils.py

qiskit/pulse/schedule.py

Co-authored-by: Daniel Egger <38065505+eggerdj@users.noreply.github.com> Co-authored-by: Will Shanks <willshanks@us.ibm.com>

nkanazawa1989 · 2022-06-21T17:16:13Z

Thanks @eggerdj @wshanks for reviewing. I tend to agree with Will about channel-less representation of reference. We can define something like OpaqueChannel for DAG evaluation (and this makes reference manager much simpler). IIRC, the requirements for channel is based on Daniel's request. Having channel in reference makes cal data structure cleaner? @eggerdj

Regarding separator, I still prefer a single string rather than tuple of it. This makes serialization much easier. Also we can use regular expression for parameter search, for example, scope=\S.x1 will give you parameters defined within the scope x1 and its parent doesn't matter. I think this somewhat convenient.

Perhaps we can conform to SCPI-ish syntax? For example, x1:y1:amp rather than x1.y1.amp. This allows you to use . in parameter name.

- reference take multiple keys instead of channels - builder.refer command is newly added - schedule.blocks returns reference replaced with actual subroutine if assigned - remove dagschedule and revert to on the fly dag generation - move reference to own file - simplify reference manager - update docs and tests

jakelishman

I tried to read through this large PR pretty quickly so it could move to merge, and especially since I'm not familiar with the pulse builders, I was generally just looking for stuff that felt odd to me - please feel free to dismiss me if I'm wrong on it.

I'm largely trusting that if Will and Dan are happy with the interface as written, then it's good, my code-based comments notwithstanding. I can't offer any meaningful opinions on that.

qiskit/pulse/builder.py

jakelishman · 2022-09-08T15:59:30Z

qiskit/pulse/builder.py

+            if name is None:
+                # Add unique string, not to accidentally override existing reference entry.
+                keys = (subroutine.name, uuid.uuid4().hex)
+            else:
+                keys = (name,)


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?

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 new reference() method could optionally take a ScheduleBlock 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 required name parameter as the key.

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.

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.

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 provided name, 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 be

Suggested change

if name is None:

# Add unique string, not to accidentally override existing reference entry.

keys = (subroutine.name, uuid.uuid4().hex)

else:

keys = (name,)

keys = name or uuid.uuid4().hex

probably this makes more sense since subroutine.name is no longer the effective identifier due to uuid.

The reference key is important only when a user assigns a subroutine to reference.

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.

keys = name or uuid.uuid4().hex

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.

Yes, this changes the behavior. I updated API doc and release note accordingly in ffd2130

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.

These comments all make sense. I leave the current implementation.

jakelishman · 2022-09-08T16:01:49Z

qiskit/pulse/builder.py

+        If there is a name collision between parameters, you can distinguish them by specifying
+        each parameter object in a python dictionary. For example,
+
+        .. jupyter-execute::
+
+            amp1 = circuit.Parameter('amp')
+            amp2 = circuit.Parameter('amp')
+
+            with pulse.build() as subroutine:
+                pulse.play(pulse.Gaussian(160, amp1, 40), pulse.DriveChannel(0))
+                pulse.play(pulse.Gaussian(160, amp2, 40), pulse.DriveChannel(1))
+
+            with pulse.build() as pulse_prog:
+                pulse.call(subroutine, value_dict={amp1: 0.1, amp2: 0.3})


Is there a reason to allow the user to write a subroutine with a naming clash between its parameters? Seems a bit odd.

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.

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".

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 be Parameter(scope, name), rather than Parameter(name, uuid).

jakelishman · 2022-09-08T16:04:22Z

qiskit/pulse/instructions/instruction.py

+        try:
+            for channel in self.channels:
+                if not isinstance(channel, Channel):
+                    raise PulseError(f"Expected a channel, got {channel} instead.")
+        except UnassignedReferenceError:
+            pass


This looks surprising to me, without knowing the context - what part of the try block can throw an UnassignedReferenceError? Nothing looks like it ought to be doing any computation, which suggests to me there's performance surprises hiding in the API.

This is a bit awkward. The issue is that the Reference instruction raises this error in its channels property. There is no reasonable value to give for channels for an unassigned reference, but on the other hand there is no good reason to try to look at the channels of a ScheduleBlock with unassigned references. The approach taken here is to raise on accessing Reference.channels and have ScheduleBlock substitute in the assigned subroutine (when it exists) in place of the Reference in its channels and blocks methods. So nothing outside of the ScheduleBlock should be working directly with a Reference instruction.

Perhaps there is a more elegant way to deal with this. For this specific change to Instruction.__init__, an alternative would be to have Reference.channels return an empty list until a flag is set, and after the flag is set it would raise an exception instead of returning an empty list. Then Reference.__init__ could call super().__init__ and then set this flag. Reference.channels could always return an empty list but I think raising an exception is better because any code trying to work with the channels of a ScheduleBlock with unassigned references is probably doing something wrong -- for example, you would not want code to infer that a schedule doing nothing but calling three unassigned subroutines actually operated on no channels.

By the way, I don't know if it is clear reading the PR now, but the original motivation for the Reference instruction was to support defining a schedule like an echoed cross-resonance gate in a separate function (or even a separate package) from the one defining the single qubit x gate. The intent was that consumer would basically define the ecr and x gates and then assign x to ecr before doing anything else.

Ah, so channels is a property that does a little bit of work? I have no problems with raising the exception if there's nothing valid to return.

I personally don't like properties doing work (I much prefer having the explicit method call) because I find it's easy to introduce massive performance hits accidentally in consumer code (people assume attribute accesses are free) and if the return value is mutable, consumers often assume mutations will propagate back to the object. But that's not my call to make, and if the work is only very slight anyway (and the return type is immutable), I wouldn't think it's a big deal.

(I honestly didn't even read the PR closely enough to know what it was trying to add - I trust you and Daniel were better reviewers of that.)

The only work channels is doing at the moment is raising an exception, so it shouldn't have a performance impact. I am curious what @nkanazawa1989 thinks about having it return [] until after calling super().__init__() so that we don't have to have a special case for Reference in the base Instrument.__init__ method.

This is good idea but I don't have reasonable implementation, because Instruction doesn't have channels values. They are just a part of self._operantds and all subclasses report channel with known index (this is to reduce parameter assignment overhead). This means we need to dynamically rewrite method.

I came up with another implementation in cdefe16. I added explicit _validate method so that every subclass can override. Indeed there was redundant instance check in subclasses.

cdefe16 looks nice to me (but might be the source of the new test failure?).

Oh I didn't notice the test was also here. Actually this test is a bug and the code here will be never accepted by real hardware. The OpenPulse simulator in Aer doesn't really simulate the measurement dynamics and thus this random channel is accepted by it.

qiskit/pulse/instructions/instruction.py

qiskit/pulse/schedule.py

jakelishman · 2022-09-08T16:41:30Z

qiskit/pulse/schedule.py

+def _scope_parameter(param: Parameter, scope: str, delimiter: str = ":") -> Parameter:
+    """Override parameter object with program scope information."""
+    new_name = f"{scope}{delimiter}{param.name}"
+    scoped_param = Parameter.__new__(Parameter, new_name, uuid=getattr(param, "_uuid"))
+    scoped_param.__init__(new_name)
+
+    return scoped_param


This looks a bit terrifying to me, to be honest. I get what you're doing, but it feels potentially very fragile. I don't have a sensible alternative suggestion, though, so it's probably best to leave it.

This is identical trick used in qpy loader, but I agree this is bit hackey. I really wanted to introduce dedicated parameter subclass for pulse module.

jakelishman · 2022-09-08T16:45:45Z

qiskit/pulse/transforms/dag.py

+        node_id = dag.add_node(elm)
+        if dag.num_nodes() == 1:
+            continue
+        prev_id = dag.node_indices()[-1]
+        edges.append((prev_id, node_id))


I haven't tested it, but this feels very like prev_id is just going to be the same as node_id every time? If so, this isn't make a DAG - it's making a graph where every node points only to itself.

Hah, you are right. It should be making a DAG that is just a linear chain of all the elements but it is instead making a set of nodes pointing to themselves. I think this indicates a missing test -- if you make two schedule blocks with the same elements but in different orders, a test that checks that the two schedules are not equal should fail.

It turns out that there is good test coverage of the different equality cases in test_block.py. Here is the test I was suggesting:

https://github.com/Qiskit/qiskit-terra/blob/fe65c1a2cff8b1061425d92fb03ccc9534c5b1b4/test/python/pulse/test_block.py#L474-L484

However, there is a quirk that makes this test pass. The if dag.num_nodes() == 1 condition causes the first node not to get an edge to itself. So for two-element schedules, the two elements end up unique -- the first one has no edges while the second one has an edge. This variant of the test fails for me though:

block1 = pulse.ScheduleBlock(alignment_context=self.sequential_context) block1 += pulse.Play(self.test_waveform0, self.d0) block1 += pulse.Play(self.test_waveform0, self.d0) block1 += pulse.Play(self.test_waveform0, self.d1) block2 = pulse.ScheduleBlock(alignment_context=self.sequential_context) block2 += pulse.Play(self.test_waveform0, self.d0) block2 += pulse.Play(self.test_waveform0, self.d1) block2 += pulse.Play(self.test_waveform0, self.d0) self.assertNotEqual(block1, block2)

In this case, the first element of each schedule is the same so that they compare okay as the no-edge parts of the graphs, and then the remaining elements all point to themselves. is_isomorphic_node_match can then reorder the remaining elements to match them since there are no edges between them.

In fairness, your second Python block there has a block1 += hidden among the block2 mutations. I don't know if that affects anything.

Oops, that was me redoing the modification in the comment instead of copy-pasting from the test file I modified. Fixed in the code above now.

This can accidentally pass this test. Indeed this always creates closed edges, but this doesn't appear in the first node. So test can still pass up to two nodes. Code is fixed in c973d61

Test from @wshanks looks effective to detect the bug. Added in 1531480.

Co-authored-by: Will Shanks <willshanks@us.ibm.com>

…bclasses

nkanazawa1989 · 2022-09-12T09:00:14Z

Thanks @jakelishman you suggestions and comments were really useful to improve the code more reasonably and fix some bugs. I think your main concern is the hidden works at property methods, and this has been somewhat alleviated by turning .scoped_parameters into a method 5b7eeef. The .parameters property still does the recursive call inside, but it's hard to change this without breaking API change.

jakelishman · 2022-09-12T12:23:04Z

I'm not too bothered about the extra work done in properties, if you think it's appropriate here. I personally don't like it (for the performance-concern reasons I cited above), but it's a valid and deliberate part of Python, and if you want to use it in your sections of the code that I don't maintain, that's totally fine and I wasn't at all planning to block review on it.

Using property to be able to avoid API breaks is one of the clearest good things about it, really.

Looks like there's a couple of test failures - I can re-review / approve once that's squashed.

nkanazawa1989 · 2022-09-13T04:39:54Z

I plan to do major refactoring of the pulse module and we can convert these heavy-compute properties into methods with it. I think all of your comments have been addressed and the PR is good to go.

jakelishman

Thanks for the changes! As long as all the pulse-specific people are happy with the interface, I'm happy for us to move this to merge.

nkanazawa1989 added 3 commits April 28, 2022 03:03

Add scope to reference manager

cb8b832

reference added to the program is managed with scope. this helps users to address parameters and reference to assign.

nkanazawa1989 added Changelog: New Feature Include in the "Added" section of the changelog mod: pulse Related to the Pulse module labels Apr 30, 2022

nkanazawa1989 requested review from a team, eggerdj and DanPuzzuoli as code owners April 30, 2022 04:29

nkanazawa1989 added 4 commits May 1, 2022 11:35

fix parameter scope bug

3ccb1d1

add more test

2960d62

documentation update

be48b5b

support regex in get_parameters

cf3c0d4

nkanazawa1989 force-pushed the feature/call_with_name branch from 2a6f93d to cf3c0d4 Compare May 1, 2022 03:19

nkanazawa1989 force-pushed the feature/call_with_name branch from 40d983b to a66f3cd Compare June 14, 2022 20:30

eggerdj requested changes Jun 16, 2022

View reviewed changes

nkanazawa1989 mentioned this pull request Jun 17, 2022

QPY schedule serialization #7300

Merged

8 tasks

wshanks reviewed Jun 19, 2022

View reviewed changes

wording edits

3c99fa3

Co-authored-by: Daniel Egger <38065505+eggerdj@users.noreply.github.com> Co-authored-by: Will Shanks <willshanks@us.ibm.com>

nkanazawa1989 force-pushed the feature/call_with_name branch from 5e7d65c to 3c99fa3 Compare June 20, 2022 22:23

nkanazawa1989 added 2 commits June 21, 2022 07:43

add details of returned order

372b684

fix bug in reference management

8cc3716

nkanazawa1989 force-pushed the feature/call_with_name branch from 64f6622 to 8cc3716 Compare June 21, 2022 17:04

nkanazawa1989 dismissed stale reviews from wshanks and eggerdj via fe65c1a September 8, 2022 15:15

jakelishman reviewed Sep 8, 2022

View reviewed changes

nkanazawa1989 added 7 commits September 9, 2022 17:41

Update union of parameters, add some test for expression.

329c6bc

Switch baseclass to UserDict

ae506e8

Remove .name setter

f483456

Update parameter logic. Turn scoped_parameters into a method.

5b7eeef

update _get_references logic

7a19c58

Fix sequential dag generation logic

c973d61

add warning for parameter collision

f579889

nkanazawa1989 force-pushed the feature/call_with_name branch from dcf59a9 to f579889 Compare September 12, 2022 05:21

nkanazawa1989 added 3 commits September 12, 2022 14:38

add test of sequential two edges

1531480

Co-authored-by: Will Shanks <willshanks@us.ibm.com>

fix replace logic

fe6fd7f

add explicit validate logic and cleanup constructor of instruction su…

cdefe16

…bclasses

nkanazawa1989 added 4 commits September 13, 2022 01:04

fix test

90334dc

add upgrade notice and API doc

ffd2130

revert return type of parameters

0fc4705

fix documentation

6ca8cfb

nkanazawa1989 force-pushed the feature/call_with_name branch from d387e1a to 6ca8cfb Compare September 12, 2022 18:05

Merge branch 'main' into feature/call_with_name

a16a596

jakelishman approved these changes Sep 14, 2022

View reviewed changes

jakelishman added Changelog: API Change Include in the "Changed" section of the changelog automerge labels Sep 14, 2022

Merge branch 'main' into feature/call_with_name

ba531c7

mergify bot merged commit 69d2994 into Qiskit:main Sep 14, 2022

nkanazawa1989 deleted the feature/call_with_name branch November 25, 2022 02:55

nkanazawa1989 mentioned this pull request May 11, 2023

Deprecate pulse Call instruction #10103

Closed

Pulse reference mechanism #8005

Pulse reference mechanism #8005

Conversation

nkanazawa1989 commented Apr 30, 2022 • edited Loading

Summary

Details and comments

qiskit-bot commented Apr 30, 2022

coveralls commented Apr 30, 2022 • edited Loading

Pull Request Test Coverage Report for Build 3053122782

💛 - Coveralls

eggerdj commented Jun 3, 2022 • edited Loading

nkanazawa1989 commented Jun 14, 2022 • edited Loading

eggerdj commented Jun 16, 2022

eggerdj left a comment

Choose a reason for hiding this comment

wshanks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nkanazawa1989 Jun 20, 2022 • edited Loading

Choose a reason for hiding this comment

nkanazawa1989 commented Jun 21, 2022 • edited Loading

jakelishman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakelishman Sep 8, 2022 • edited Loading

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

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

jakelishman Sep 8, 2022 • edited Loading

Choose a reason for hiding this comment

wshanks Sep 8, 2022 • edited Loading

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

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

Choose a reason for hiding this comment

wshanks Sep 9, 2022 • edited Loading

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

nkanazawa1989 commented Sep 12, 2022

jakelishman commented Sep 12, 2022

nkanazawa1989 commented Sep 13, 2022

jakelishman left a comment

Choose a reason for hiding this comment

nkanazawa1989 commented Apr 30, 2022 •

edited

Loading

coveralls commented Apr 30, 2022 •

edited

Loading

eggerdj commented Jun 3, 2022 •

edited

Loading

nkanazawa1989 commented Jun 14, 2022 •

edited

Loading

nkanazawa1989 Jun 20, 2022 •

edited

Loading

nkanazawa1989 commented Jun 21, 2022 •

edited

Loading

jakelishman Sep 8, 2022 •

edited

Loading

jakelishman Sep 8, 2022 •

edited

Loading

wshanks Sep 8, 2022 •

edited

Loading

wshanks Sep 9, 2022 •

edited

Loading