Skip to content
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

Remove symbolic pulse subclasses #8261

Closed
nkanazawa1989 opened this issue Jun 28, 2022 · 0 comments · Fixed by #8278
Closed

Remove symbolic pulse subclasses #8261

nkanazawa1989 opened this issue Jun 28, 2022 · 0 comments · Fixed by #8278
Labels
type: feature request New feature or request

Comments

@nkanazawa1989
Copy link
Contributor

What should we add?

Background

We have four parametric pulse subclass in pulse library, namely, Gaussian, GaussianSquare, Drag, Constant.

from qiskit.pulse.library import Gaussian
instance = Gaussian(160, 0.1, 40)

This returns a class instance dedicated to each waveform kind.

In #7821 , we replaced the base class of them from ParametricPulse to SymbolicPulse. In contrast to parametric pulse, symbolic pulse can uniquely determine waveform with instance variables self._envelope and self._pulse_type which are a symbolic equation and string, respectively. This means, these subclasses are no longer necessary and we can turn everything into symbolic pulse instance. This can simplify the deserialization logic of QPY (actually type information is not stored).

Proposal

I want to convert Gaussian class into a symbolic pulse factory while keeping camel case :

def Gaussian(duration, amp, sigma, name, limit_amplitude):
    ...
    return SymbolicPulse(...)

so that user code works without deprecation, e.g.

with build() as schedule:
    # now the first arg is SymbolicPulse, rather than Gaussian
    play(Gaussian(160, 0.1, 40), DriveChannel(0))

Problems

However, sometimes type information is used in the codebase. For example, in the RZX transpiler pass:
https://github.com/Qiskit/qiskit-terra/blob/10822404936e98df01f2975ef43b8378006621ba/qiskit/transpiler/passes/calibration/builders.py#L163-L168
I can update such use case in our codebase. This can be written without type information like below.

if pulse_.pulse_type == "GaussianSquare":

However, because we have never prohibited users from using type information, we need proper communication with some warning mechanism. Adding deprecation warning in symbolic pulse constructor is very annoying since it will issue tones of warnings (e.g. loading each pulse in backend CmdDef). Probably overriding __instancecheck__ method to show deprecation warning is possible, however, this is not sensitive to, for example, __class__.__name__.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature request New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant