Add NormalizeRXAngle and RXCalibrationBuilder passes

[jaeunkim]

Summary

Two new transpiler passes are added to generate single-pulse RX gate calibrations on the fly. These single-pulse RX calibrations will reduce the gate time in half, as described in P.Gokhale et al, arXiv:2004.11205.

• qiskit.transpiler.passes.optimization.normalize_rx_angle.NormalizeRXAngle

  • Inherits TransformationPass
  • Performs three optimizations to reduce the amount of RX calibration data that needs to be generated:
    1. Wrapping RX gate rotation angles to [0, pi]: Replaces an RX gate with a negative rotation angle, RX(-theta), with a sequence: RZ(pi)-RX(theta)-RZ(-pi). This will help reduce the size of calibration data, as we won't have to keep separate, phase-flipped calibrations for negative rotation angles.
    2. Replacing RX(pi/2) and RX(pi) with SX and X gates: This will allow us to exploit the more accurate, hardware-calibrated pulses.
    3. Quantizing the rotation angles using a user-provided resolution: If the resolution is set to 0, this pass will not perform any quantization.
  • Required to be run before the qiskit.transpiler.passes.calibration.rx_builder.RXCalibrationBuilder

• qiskit.transpiler.passes.calibration.rx_builder.RXCalibrationBuilder

  • Inherits CalibrationBuilder
  • Generates RX calibrations on the fly. The pulse calibrations are bootstrapped from the SX gate calibration in the target. The amplitude is linearly scaled to achieve the desired arbitrary rotation angle.

[qiskit-bot]

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
• @nkanazawa1989

[nkanazawa1989]

Thanks @jaeunkim. I believe combining this with the existing RZX pass will further extend the available virtual circuit depth. Class structure and responsibility are well designed, and test is also written well. My comments are almost nitpick.

[nkanazawa1989]

I guess current logic causes an edge case. For example,

pass_ = NormalizeRXAngle(target, resolution_in_radian=0.1)
pass_.quantize_angles(0, 1.23)
pass_.quantize_angles(0, 1.24)
pass_.quantize_angles(0, 1.235)  # What happens here?

[jaeunkim]

In that case, more than one value will return True from np.isclose. In such a case, I will take only the first value.

similar_angle = angles[np.isclose(angles, original_angle, atol=self.resolution_in_radian/2)]
quantized_angle = float(similar_angle[0]) if len(similar_angle) > 1 else float(similar_angle)

(Note: the np.isclose test for 1.235 will return at least one True, because rtol is set to a small, nonzero default value. I will retain this setting because setting rtol to zero can lead to the np.isclose returning no True at all, due to uncertainties related to floating-point arithmetic.)

[jaeunkim]

Thank you @nkanazawa1989 for the detailed review. By "adding this pass to the existing RZX pass", do you mean merging the classes into one or setting these as required passes to the RZX pass? Should I include that change in this PR too?

[nkanazawa1989]

Please update class doc to show more practical example. Rest of code looks good to me!

(edit)

By "adding this pass to the existing RZX pass", do you mean merging the classes into one or setting these as required passes to the RZX pass? Should I include that change in this PR too?

No. I meant using this pass in combination with RZX pass will further reduce the actual circuit depth.

[wshanks]

I added a few small comments but this looks good to me.

One thing I wonder -- should the RXCalibrationBuilder fail/warn if there are no sx calibrations? Perhaps it will be clear enough when the user tries to run a job with rx gates that the backend/transpiler does not understand.

[wshanks]

This is probably fine, but it makes me a little nervous. I would do more checking to confirm that the default calibration is a single Drag pulse.

[jaeunkim]

I added two tests in the supported() method to check if the default calibration is a single pulse and it's a DRAG pulse.

 def supported(self, node_op: Instruction, qubits: list) -> bool:
        """
        Check if the calibration for SX gate exists and it's a single DRAG pulse.
        """
        return (
            isinstance(node_op, RXGate)
            and self.target.has_calibration("sx", tuple(qubits))
            and (len(self.target.get_calibration("sx", tuple(qubits)).instructions) == 1)
            and isinstance(
                self.target.get_calibration("sx", tuple(qubits)).instructions[0][1].pulse, Drag
            )
        )

However, I can't decide whether we should allow non-DRAG pulses too or not.

• Pros for allowing non-DRAG pulses: Flexibility. This pass simply scales the amplitude of the default calibration, based on preliminary experiments that indicated no need to alter the DRAG coefficient. Since all other types of pulses (Gaussian, square, etc) have the amplitude parameter, it's technically possible to scale the amplitude of any type of pulses.

• Cons for allowing non-DRAG pulses: The goal of this pass is to maximize the gate fidelity by decreasing gate time. The gate fidelity might not be optimal with non-DRAG pulses. Therefore, the user should probably try using DRAG pulses first, instead of using this pass.

[wshanks]

I think it would be okay to check for ScalableSymbolicPulse and scale the amp parameter in that. Since this is not part of a preset passmanager and is a somewhat advanced concept, I think it is okay to assume that the user has some understanding of the pulse to be scaled. I was more concerned about a using doing something unusual like using a schedule with two pulses or with a variant of Drag and not realizing the schedule was being replaced with a single Drag.

Also, isinstance(pulse, Drag) is going to be deprecated. The recommendation is to use isinstance(pulse, SymoblicPulse) and pulse.pulse_type == "Drag" because Drag and the other symbolic pulses are being converted into factory functions that produce SymbolicPulses instead of classes themselves. If you switch to checking for ScalableSymbolicPulse and scaling the amp this won't matter.

[jaeunkim]

Thank you @wshanks, I'm trying to cover the case where the original sx calibration is a ScheduleBlock with multiple types of instructions and multiple types of pulses, as you pointed out.
Could you help me building a new ScheduleBlock where everything else is the same but only the amplitude is halved?
I'm having trouble copying the alignment constraints and non-Play type of instructions.

This is an example of an unusually complicated sx schedule:

d0 = pulse.DriveChannel(0)
with pulse.build() as complicated_sx_cal:
    pulse.play(Drag(duration=100, amp=0.1, sigma=40, beta=0.2, angle=10), d0)
    pulse.delay(50, d0)
    pulse.play(
        GaussianSquare(duration=130, amp=0.3, sigma=30, risefall_sigma_ratio=2, angle=30), d0
    )

This is my attempt:

# goal: copy a ScheduleBlock, but halve the amplitude.

with pulse.build() as rx_cal:
    for i in range(len(complicated_sx_cal.instructions)):
        if isinstance(complicated_sx_cal.instructions[i][1], Play) and isinstance(
            complicated_sx_cal.instructions[i][1].pulse, ScalableSymbolicPulse
        ):
            # caveat: assumed that all the params are immutable (checked that Parameter is immutable)
            params = complicated_sx_cal.instructions[i][1].pulse.parameters.copy()
            halved_amp = params.pop("amp") * 0.5
            duration = params.pop("duration")
            angle = params.pop("angle", 0)
            pulse.play(
                ScalableSymbolicPulse(
                    complicated_sx_cal.instructions[i][1].pulse.pulse_type,
                    duration=duration,
                    amp=halved_amp,
                    angle=angle,
                    parameters=params,
                ),
                channel=complicated_sx_cal.channels[0]
            )  
        else:
            complicated_sx_cal.instructions[i][1]  #??

Questions:

• How do you copy a non-Play (ex. delay) instruction?
• How do you copy the alignment constraints in a ScheduleBlock? Or, would it be OK to assume that there won't be any alignment constraints for a sx gate calibration?

[jaeunkim]

When I print out the above rx_cal, the pulses seem to have the correct types (ex. Drag and GaussianSquare).
ScheduleBlock(Play(Drag(duration=100, sigma=40, beta=0.2, amp=0.05, angle=10), DriveChannel(0)), Play(GaussianSquare(duration=130, sigma=30, width=10.0, amp=0.15, angle=30), DriveChannel(0)), name="block25", transform=AlignLeft())

However, I get Pulse envelope expression is not assigned errors when I try to draw the schedule.

[wshanks]

Hmm, we can see what @nkanazawa1989 thinks. One option is not to try to handle such a complicated case, but to choose either to give an error or ignore it. A complicated schedule seems not too likely to come up, and either giving an error or ignoring the gate would avoid the case of assigning the wrong schedule (i.e. assigning a Drag with amplitude based on the first pulse amplitude if the schedule really has multiple instructions).

I think it's pretty tricky to walk through a ScheduleBlock and modify the pulses. I think I would not use the builder interface to do it. There is a ScheduleBlock.replace method that you could use like:

new_sched = copy.deepcopy(sched)
for block in sched.blocks:
    if isinstance(block, Play) and isinstance(block.pulse, ScalableSymbolicPulse):
        new = Play(ScalableSymbolicPulse(pulse_type=block.pulse.pulse_type, ...<a lot of options to copy>), block.channel)
        new_sched.replace(block, new)

but that only covers the top level blocks. You would also need to check isinstance(block, ScheduleBlock) and recurse into the nested ScheduleBlocks and replace pulses in them and call replace(block, new_scheduleblock).

[jaeunkim]

I see, I agree that it's reasonable to reject a schedule with multiple pulses.
The current implementation of supported() checks that the sx cal is a single pulse AND it's a Drag.
https://github.com/Qiskit/qiskit/pull/10634/files#diff-5c2525ef082142486a945b9138611abe282cc048803c89059207112f8db192b8R103-R106

To deal with any type of ScalableSymbolicPulse, I need to figure out why I get Pulse envelope expression is not assigned errors after instantiating a ScalableSymbolicPulse(pulse_type=some-string,...)
Thank you for the example on replace().

[wshanks]

Copying a ScalableSymbolicPulse is a bit awkward. It should work if you copy all the input parameters:

p = pulse.Drag(100, 0.5, 10, 0.1)
p2 = pulse.ScalableSymbolicPulse(
    pulse_type=p.pulse_type,
    duration=p.duration,
    amp=p.amp,
    angle=p.angle,
    parameters={k: v for k, v in p.parameters.items() if k not in ("amp", "angle", "duration")},
    limit_amplitude=p.limit_amplitude,
    envelope=p.envelope,
    constraints=p.constraints,
    valid_amp_conditions=p.valid_amp_conditions,
)

It would be nice to be able to copy the pulse and mutate the amp, but mutation is not part of the design. A SymbolicPulse.copy_with_new_parameters() method might be nice to have.

[wshanks]

Also, I agree that the current version is fine if you don't want to support ScalableSymbolicPulse but I would change the isinstance(<pulse>, Drag) to <pulse>.pulse_type == "Drag" to avoid the deprecation of isinstance support for the library pulses.

[jaeunkim]

Thank you for the discussion 😊 I updated the Drag check according to your suggestion. (In other words, the current version supports only the calibrations with a single Drag pulse)
As a next step, I will support calibrations with a single ScalableSymbolicPulse.
The final step would be to support calibrations with multiple instructions (nested ScheduleBlock, non-Play instructions, etc). However, such calibrations seem unlikely to occur. Moreover, I may need to introduce some arbitrary rules to reject edge cases. Therefore, I would like to require in supported() that sx calibration should consist of a single ScalableSymbolicPulse.

[jaeunkim]

Thank you for the review, @nkanazawa1989 and @wshanks. Please take a look at my comments above.

This branch seems to fail the test on GitHub with Python 3.8, although it passes all the test on my laptop with Python 3.11. Could you suggest me any solutions?
This is the result I get from running tox -epy311 -- test.python locally.

On the other hand, I get stestr: 'run test.python' is not a stestr command error when I run the test with Python 3.8, both on GitHub and locally.

[wshanks]

I tried restarting the tests now. That seems like a broken installation of stestr. If it is repeatable, we can look more closely at the logs and consult the experts.

[wshanks]

I tried restarting the tests now. That seems like a broken installation of stestr. If it is repeatable, we can look more closely at the logs and consult the experts.

It seems to be a problem with a dependency of stestr which was updated recently. Someone else will make a PR to pin the version of that dependency soon and then you can update this branch to use that.

[wshanks]

#10855 should fix the CI failure. After it is merged, you merge main into your branch to get the CI to pass again.

Edit: Actually, a new stestr version should be released soon to fix the issue as well, so rather than waiting for #10855 and merging in main it may be enough to re-run the tests again in a few hours after the stestr release.

[wshanks]

If you want to extend the set of pulses this works on, that is fine, but I think the current code (only working for Drag) is good in its current form.

[jaeunkim]

functools.lru_cache doesn’t seem to work with ScalableSymbolicPulse, so I will stick to the current version that only allows Drag.

[wshanks]

Yes, ScalableSymbolicPulse is unhashable because it can mutate if it has unassigned parameters that are later assigned. lru_cache only works on hashable objects since it saves the arguments as keys in a dictionary. You should be able to get lru_cache to work by splitting out all the ScalableSymbolicPulse slots as arguments to _create_rx_schedule but it would be getting complicated.

[jaeunkim]

I see. Thank you for the suggestion😊 I had also tried some workarounds, but the code indeed ended up being complicated. I think the new code would be difficult to maintain, especially if ScalableSymbolicPulse API changes. How about merging the code as it currently is, and then introducing the (caching of) ScalableSymbolicPulse support once we discover a case where it would be useful?

[wshanks]

It is good to merge as far as I am concerned. We need to wait for @nkanazawa1989. I think he might not be able to look at it again before next week.

[nkanazawa1989]

Thanks @jaeunkim @wshanks (and sorry about slow response). The PR looks good to go now. In my thoughts, if an advanced user wants to support non-DRAG case, probably they are using some trickier pulse that may violate our assumption, e.g. DRAG beta stay the same, so it's more reasonable to let them subclass RXCalibrationBuilder and we just offer the class design which is subclassing friendly. I think RXCalibrationBuilder.get_calibration is easy to overwrite, and current implementation is enough. I'll approve and add this to the queue. Congrats @jaeunkim for the first code contribution :)