[WIP] symbolic expression for symbolic pulse equating

[TsafrirA]

Summary

Qiskit-Terra PR #9002, converted the library symbolic pulses from a complex amp representation to a real (float) amp,angle representation. To overcome the non-unique nature of the new representation (particularly, when negative amp is allowed), this PR adds an optional attribute params_comp_exp to store symbolic expression\s which are used to equate pulses.

Details and comments

Qiskit-Terra PR #9002, converted the library symbolic pulses from a complex amp representation to a real (float) amp,angle representation. To better support several experiments in Qiskit-Experiments, the amp parameter is allowed to take both positive and negative values. This decision creates a non-unique representation for the pulses, where a pi shift of angle and a sign flip of amp have the same effect (the issue obviously also exists for 2*pi shift of angle). For example, the following two pulses

from qiskit.pulse.library import Gaussian
import numpy as np

g1 = Gaussian(duration=100, sigma=50, amp=-1, angle=0)
g2 = Gaussian(duration=100, sigma=50, amp=1, angle=np.pi)

are not the same when equated (because their parameters are different):

g1 == g2 # False

but they represent the same waveform:

np.all(g1.get_waveform().samples == g2.get_waveform().samples) # True

This PR sets to correct this situation, by introducing a new attribute to the SymbolicPulse class - param_comp_exp. This new optional argument stores symbolic expression\s which are used when equating two pulses. Parameters appearing in these expressions are not equated individually, but only via the value of the total expression. For custom pulses, one can use any expression, but for the library pulses the expression in use is amp*sym.exp(sym.I*angle) which removes the problem. Returning to the example above, the new PR yields:

g1 == g2 # True

Discussion

The new addition is flexible and allows for a single expression or a list of expressions to be used. However, it is not obvious that such flexibility is needed. I can't think of an example where one would want two or more such expressions for a pulse.

Additionally, performance seems to be an issue. The new test added to test\python\pulse\test_pulse_lib.py is by far the slowest of the entire batch - despite testing a rather modest scenario. Furthermore, the new attribute will increase instances' size, for example, when saved to QPY.

• Correct QPY loader for backwards compatibility.

[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 @TsafrirA this is a good start to discuss implementation. Current implementation has a fundamental limitation, namely, there is always only one amplitude; F(t, theta) = A F'(t, theta'). To me this seems very strong assumption. For example, we may want to define geometric pulse consisting of two tones at different frequencies.

I also want to avoid adding new symbolic expression, since this increases the complexity and computational overhead to build a program. Indeed, current symbolic pulse implementation is already too complicated, i.e. expression for envelope, validation, and condition to invoke waveform generation.

Alternatively you can delegate canonicalize to ParameterExpression object to manage the case when amp or angle are given by parameter expression. For example,

from qiskit.circuit import Parameter, ParameterExpression
from qiskit.pulse.utils import format_parameter_value
import symengine as sym

def amplitude_formatter(amp, angle):
    _amp, _angle = sym.symbols("amp, angle")

    tmp_amp = Parameter("amp")
    tmp_angle = Parameter("angle")

    canonical_amp = ParameterExpression(
        symbol_map={tmp_amp: _amp},
        expr=sym.Abs(_amp),
    )
    canonical_amp = canonical_amp.assign(tmp_amp, amp)

    canonical_angle = ParameterExpression(
        symbol_map={tmp_amp: _amp, tmp_angle: _angle},
        expr=_angle + (1 - sym.sign(_amp)) / 2 * sym.pi,
    )
    canonical_angle = canonical_angle.assign(tmp_angle, angle)
    canonical_angle = canonical_angle.assign(tmp_amp, amp)

    params = {
        "amp": format_parameter_value(canonical_amp),
        "angle": format_parameter_value(canonical_angle),
    }
    return params

Edit; As a side effect, this implicitly canonicalize user parameter, i.e. amp=-1 will appear as amp=1 when the user access to parameters.

[TsafrirA]

@nkanazawa1989
Thanks, Naoki, for the insightful comments.
Correct me if I'm wrong, but it seems like your proposal hard codes the amp\angle conversion, thus assuming a role for these parameters even for custom built pulses? This is what we tried to avoid with PR #9002. To avoid role assumption, one has to somehow store in the specific instance the conversion mechanism from the "parameters" to the "canonical parameters". If we don't want it to be a symbolic expression, we need to find a different solution. (Storing a callable?)

I know we have only recently dropped the sub-classes (PR #8278), but it would seem like the easiest solution is to create subclasses where you can hard-code things whatever way you like (Note that the inability to differentiate library pulses from custom built ones also gave us a hard time in #9002). However, I am sure there was a good reason to drop out the subclasses.