Bug fix sample pulse name conflict in qobj assembly #2431
Conversation
|
|
8cb34e3 to
41fd65f
Compare
41fd65f to
85b39ce
Compare
There was a problem hiding this comment.
Thank you very much for tackling this @lcapelluto. Congrats on your first PR! Just a couple of small implementation details that I would recommend, the most crucial is that I think we should avoid mutating the pulse elements when possible as the underlying private methods could always change in the future.
| if isinstance(instruction, PulseInstruction): | ||
| name = instruction.command._name |
There was a problem hiding this comment.
We should try not to access the SamplePulse by its private name, but rather its public command.name.
| if isinstance(instruction, PulseInstruction): | ||
| name = instruction.command._name | ||
| if name in user_pulselib and not np.allclose(instruction.command.samples, |
There was a problem hiding this comment.
Perhaps just check for equality instead on just the SamplePulse object?
| # add samples to pulse library | ||
| user_pulselib.add(instruction.command) | ||
| user_pulselib[name] = instruction.command.samples |
There was a problem hiding this comment.
If you take the approach above, I guess you would need to first create a user_pulselib of name: SamplePulse pairs, and then unwrap this later to a new pulse library of name: array pairs. I think you should do what you feel is best.
| # add samples to pulse library | ||
| user_pulselib.add(instruction.command) | ||
| user_pulselib[name] = instruction.command.samples | ||
| instruction.command._name = name |
There was a problem hiding this comment.
I think it would be better to create a new SamplePulse here with a new name and the same samples, rather than mutate the existing SamplePulse.
There was a problem hiding this comment.
Are we guaranteed to always have a Sample Pulse command and no other kinds of instructions?
There was a problem hiding this comment.
(as the command within a PulseInstruction)
There was a problem hiding this comment.
Running into another issue: I can't set instruction.command either, so I tried to make a new PulseInstruction, which complained because timeslots isn't an initarg... but I'm guessing it's important to carry that information forward. I will look further into this tomorrow, but lmk what you'd do if you know offhand.
There was a problem hiding this comment.
Also, instruction = ... should give qobj_instructions.append(instruction_converter(shift, instruction)) the right behavior, but do I have to worry about the fact that the original schedule isn't going to have the correct name for that PulseInstruction?
There was a problem hiding this comment.
I think you're on the right track, will need to create a new PulseInstruction and SamplePulse due to immutability. Should be something like
PulseInstruction(SamplePulse(old_samples, name=new_name), old_instruction.channels[0])
| @@ -80,8 +87,8 @@ def assemble_schedules(schedules, qobj_id, qobj_header, run_config): | |||
| }) | |||
|
|
|||
| # setup pulse_library | |||
| qobj_config['pulse_library'] = [PulseLibraryItem(name=pulse.name, samples=pulse.samples) | |||
| for pulse in user_pulselib] | |||
| qobj_config['pulse_library'] = [PulseLibraryItem(name=name, samples=samples) | |||
There was a problem hiding this comment.
If the above change is made to the pulse lib dictionary representation, this change would no longer be required.
… github.com:lcapelluto/qiskit-terra into bug-fix-sample-pulse-name-conflict-in-qobj-assembly
* Append sample hash to pulse sample names if there is a conflict * Fixup * Create new SamplePulse rather than mutating existing
* Append sample hash to pulse sample names if there is a conflict * Fixup * Create new SamplePulse rather than mutating existing
Summary
Fixes #2404
Details and comments
Previously, if a user created a pulse schedule with two different pulses of the same name and assembled the schedule into a Qobj, the first pulse would be played for both instructions. Now, the name conflict is resolved.* The first pulse remains named
pulse.name, and later pulses are namedf"{pulse.name}-{hash(pulse.samples.tostring())}"* Except when >2 pulses share the same name, the pulses are long enough that their str repr is abbreviated, and the abbreviated samples (usually the first and last few) are the same. I'd love recommendations for fixing this! Do we have qubit and time info? Then we can be sure we wouldn't have more conflicts (can't play two pulses at the same time on the same channel)