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 subclass implementation #8278

Merged
merged 13 commits into from
Sep 27, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion qiskit/assembler/assemble_schedules.py
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ def _assemble_instructions(
if isinstance(instruction.pulse, (library.ParametricPulse, library.SymbolicPulse)):
is_backend_supported = True
try:
pulse_shape = ParametricPulseShapes(type(instruction.pulse)).name
pulse_shape = ParametricPulseShapes.from_instance(instruction.pulse).name
if pulse_shape not in run_config.parametric_pulses:
is_backend_supported = False
except ValueError:
Expand Down
122 changes: 90 additions & 32 deletions qiskit/pulse/library/symbolic_pulses.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
"""

import functools
import warnings
from typing import Any, Dict, List, Optional, Union, Callable

import numpy as np
Expand Down Expand Up @@ -579,7 +580,42 @@ def __repr__(self) -> str:
)


class Gaussian(SymbolicPulse):
class _PulseType(type):
"""Metaclass to warn at isinstance check."""

def __instancecheck__(cls, instance):
try:
cls_alias = getattr(cls, "alias")
except AttributeError:
return NotImplemented
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this return NotImplemented instead of False? I didn't see anything about NotImplemented in https://peps.python.org/pep-3119/.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I changed the logic. The point of this metaclass is to raise UserWarning to tell users subclasses are removed. So this should not return before the warning.


# TODO promote this to Deprecation warning in future.
# Once type information usage is removed from user code,
# we will convert pulse classes into functions.
warnings.warn(
"Typechecking with the symbolic pulse subclass will be deprecated. "
f"'{cls_alias}' subclass instance is turned into SymbolicPulse instance. "
f"Use self.pulse_type == '{cls_alias}' instead.",
UserWarning,
nkanazawa1989 marked this conversation as resolved.
Show resolved Hide resolved
)

if not isinstance(instance, SymbolicPulse):
return False
return instance.pulse_type == cls_alias

def __getattr__(cls, item):
# For pylint. A SymbolicPulse subclass must implement several methods
# such as .get_waveform and .validate_parameters.
# In addition, they conventionally offer attribute-like access to the pulse parameters,
# for example, instance.amp returns instance._params["amp"].
# If pulse classes are directly instantiated, pylint yells no-member
# since the pulse class itself implements nothing. These classes just
# behave like a factory by internally instantiating the SymbolicPulse and return it.
# It is not realistic to write disable=no-member across qiskit packages.
return NotImplemented
Comment on lines +598 to +607
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super hacky, but it sounds like the right call if you need isinstance to keep working.



class Gaussian(metaclass=_PulseType):
r"""A lifted and truncated pulse envelope shaped according to the Gaussian function whose
mean is centered at the center of the pulse (duration / 2):

Expand All @@ -591,14 +627,16 @@ class Gaussian(SymbolicPulse):
where :math:`f'(x)` is the gaussian waveform without lifting or amplitude scaling.
"""

def __init__(
self,
alias = "Gaussian"

def __new__(
cls,
duration: Union[int, ParameterExpression],
amp: Union[complex, ParameterExpression],
sigma: Union[float, ParameterExpression],
name: Optional[str] = None,
limit_amplitude: Optional[bool] = None,
):
) -> SymbolicPulse:
"""Create new pulse instance.

Args:
Expand All @@ -610,6 +648,8 @@ def __init__(
limit_amplitude: If ``True``, then limit the amplitude of the
waveform to 1. The default is ``True`` and the amplitude is constrained to 1.

Returns:
SymbolicPulse instance.
"""
parameters = {"amp": amp, "sigma": sigma}

Expand All @@ -621,8 +661,8 @@ def __init__(
consts_expr = _sigma > 0
valid_amp_conditions_expr = sym.Abs(_amp) <= 1.0

super().__init__(
pulse_type=self.__class__.__name__,
instance = SymbolicPulse(
pulse_type=cls.alias,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just checking: does SymbolicPulse.pulse_type have any mathematical meaning, or is it just like a "name" field? I find it weird if it has mathematical meaning (the subclasses seem cleaner, in that case), but if it's just a name, then 👍.

Copy link
Contributor Author

@nkanazawa1989 nkanazawa1989 Sep 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. It could have mathematical meaning, because a symbolic pulse instance is tied to a particular symbolic expression, and we implicitly assumes unique mapping to pulse_type. This is not something user can change per instance. Note that this information is just kind of metadata, because waveform data can be generated without type information, i.e. get_waveform can generate sample data by itself through the attached symbolic expression.

The motivation of this PR is to address round-trip serialization issue. The important point here is all pulse classes are not necessary defined in Qiskit. For example, in research code, end-user can define new pulse shape in parametric form, and they may choose not to expose it to Qiskit before publication. On the other hand, they may want to share the pulse with colleagues.
In this situation, something weird happens with subclasses. If one defines class MyPulse(SymbolicPulse) and instantiates it, then serializes it through QPY and load at a later time. This comes back as a direct SymbolicPulse instance because Qiskit, particularly QPY, will never know the subclass, i.e. the MyPulse doesn't exist in qiskit.pulse.library. However, if everything is direct instance of SymbolicPulse and such type is defined by .pulse_type: str rather than class, we can serialize the data properly.

Perhaps you know better idea :)

Copy link
Contributor Author

@nkanazawa1989 nkanazawa1989 Sep 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that an end-user is not necessary a single Qiskit user. For example, IBM backend may have several pulses, and experiment library should be able to manage these resources without modifying QPY loader for different libraries. Another option would be upgrade QPY to take Encoder/Decoder like JSON.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pulse names have a special meaning because the backend can support a set of pulses parametrically for which only the name and parameters need to be provided. For IBM backends, these names are snakecase versions of the old pulse classes (Gaussian <-> gaussian). Some of the awkwardness of pulse_type is that we are still kind of supporting the old classes by letting you create a gaussian pulse using Gaussian and have its repr show up as Gaussian(...) using the pulse_type (so invocation and repr stay the same, though the object is now a SymbolicPulse rather than a Gaussian). The ParametricPulseShapes enum translates the old class name to the name for the backend.

Ideally, one day the backend can accept a symbolic expression directly in the pulse data instead of just the name of a supported pulse and then we could send that expression and the names would not matter.

It would be nice if we could move the backend names into the SymbolicPulse data and drop the special-case ParametricPulseShapes translation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ugh, in current framework pulse_type is more than metadata. So these pulses should be subclasses as long as we can avoid round trip serialization issue.

It would be nice if we could move the backend names into the SymbolicPulse data and drop the special-case ParametricPulseShapes translation.

I'm bit against to this direction because this ties a particular backend to user program. Probably we can introduce an internal pulse class so that we can set backend name reported by backends in the transform logic.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm bit against to this direction because this ties a particular backend to user program. Probably we can introduce an internal pulse class so that we can set backend name reported by backends in the transform logic.

I don't understand the concern here. You don't like including something like parametric_pulse_name: gaussian in the Gaussian SymbolicPulse because gaussian implies it is the IBM backend? With these pulses defined in terra, couldn't any backend choose to implement them with the given names?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Because the Qobj converter is standalone mechanism from assemble, and provider can give their pulse name through a custom converter. If we tie this to pulse class itself this is kind of enforcing the serialization rule to all provider and programs are no longer backend agnostic. I think Gaussian Square is kind of IBM convention, e.g. I more often see Gaussian flat top in literatures.

duration=duration,
parameters=parameters,
name=name,
Expand All @@ -631,10 +671,12 @@ def __init__(
constraints=consts_expr,
valid_amp_conditions=valid_amp_conditions_expr,
)
self.validate_parameters()
instance.validate_parameters()

return instance

class GaussianSquare(SymbolicPulse):

class GaussianSquare(metaclass=_PulseType):
"""A square pulse with a Gaussian shaped risefall on both sides lifted such that
its first sample is zero.

Expand Down Expand Up @@ -672,16 +714,18 @@ class GaussianSquare(SymbolicPulse):
where :math:`f'(x)` is the gaussian square waveform without lifting or amplitude scaling.
"""

def __init__(
self,
alias = "GaussianSquare"

def __new__(
cls,
duration: Union[int, ParameterExpression],
amp: Union[complex, ParameterExpression],
sigma: Union[float, ParameterExpression],
width: Optional[Union[float, ParameterExpression]] = None,
risefall_sigma_ratio: Optional[Union[float, ParameterExpression]] = None,
name: Optional[str] = None,
limit_amplitude: Optional[bool] = None,
):
) -> SymbolicPulse:
"""Create new pulse instance.

Args:
Expand All @@ -695,6 +739,9 @@ def __init__(
limit_amplitude: If ``True``, then limit the amplitude of the
waveform to 1. The default is ``True`` and the amplitude is constrained to 1.

Returns:
SymbolicPulse instance.

Raises:
PulseError: When width and risefall_sigma_ratio are both empty or both non-empty.
"""
Expand Down Expand Up @@ -729,8 +776,8 @@ def __init__(
consts_expr = sym.And(_sigma > 0, _width >= 0, _duration >= _width)
valid_amp_conditions_expr = sym.Abs(_amp) <= 1.0

super().__init__(
pulse_type=self.__class__.__name__,
instance = SymbolicPulse(
pulse_type=cls.alias,
duration=duration,
parameters=parameters,
name=name,
Expand All @@ -739,15 +786,12 @@ def __init__(
constraints=consts_expr,
valid_amp_conditions=valid_amp_conditions_expr,
)
self.validate_parameters()
instance.validate_parameters()

@property
def risefall_sigma_ratio(self):
"""Return risefall_sigma_ratio. This is auxiliary parameter to define width."""
return (self.duration - self.width) / (2.0 * self.sigma)
return instance


class Drag(SymbolicPulse):
class Drag(metaclass=_PulseType):
"""The Derivative Removal by Adiabatic Gate (DRAG) pulse is a standard Gaussian pulse
with an additional Gaussian derivative component and lifting applied.

Expand Down Expand Up @@ -784,15 +828,17 @@ class Drag(SymbolicPulse):
Phys. Rev. Lett. 103, 110501 – Published 8 September 2009.*
"""

def __init__(
self,
alias = "Drag"

def __new__(
cls,
duration: Union[int, ParameterExpression],
amp: Union[complex, ParameterExpression],
sigma: Union[float, ParameterExpression],
beta: Union[float, ParameterExpression],
name: Optional[str] = None,
limit_amplitude: Optional[bool] = None,
):
) -> SymbolicPulse:
"""Create new pulse instance.

Args:
Expand All @@ -804,6 +850,9 @@ def __init__(
name: Display name for this pulse envelope.
limit_amplitude: If ``True``, then limit the amplitude of the
waveform to 1. The default is ``True`` and the amplitude is constrained to 1.

Returns:
SymbolicPulse instance.
"""
parameters = {"amp": amp, "sigma": sigma, "beta": beta}

Expand All @@ -819,8 +868,8 @@ def __init__(
consts_expr = _sigma > 0
valid_amp_conditions_expr = sym.And(sym.Abs(_amp) <= 1.0, sym.Abs(_beta) < _sigma)

super().__init__(
pulse_type=self.__class__.__name__,
instance = SymbolicPulse(
pulse_type="Drag",
duration=duration,
parameters=parameters,
name=name,
Expand All @@ -829,10 +878,12 @@ def __init__(
constraints=consts_expr,
valid_amp_conditions=valid_amp_conditions_expr,
)
self.validate_parameters()
instance.validate_parameters()

return instance


class Constant(SymbolicPulse):
class Constant(metaclass=_PulseType):
"""A simple constant pulse, with an amplitude value and a duration:

.. math::
Expand All @@ -841,13 +892,15 @@ class Constant(SymbolicPulse):
f(x) = 0 , elsewhere
"""

def __init__(
self,
alias = "Constant"

def __new__(
cls,
duration: Union[int, ParameterExpression],
amp: Union[complex, ParameterExpression],
name: Optional[str] = None,
limit_amplitude: Optional[bool] = None,
):
) -> SymbolicPulse:
"""Create new pulse instance.

Args:
Expand All @@ -856,6 +909,9 @@ def __init__(
name: Display name for this pulse envelope.
limit_amplitude: If ``True``, then limit the amplitude of the
waveform to 1. The default is ``True`` and the amplitude is constrained to 1.

Returns:
SymbolicPulse instance.
"""
parameters = {"amp": amp}

Expand All @@ -872,13 +928,15 @@ def __init__(
envelope_expr = _amp * sym.Piecewise((1, sym.And(_t >= 0, _t <= _duration)), (0, True))
valid_amp_conditions_expr = sym.Abs(_amp) <= 1.0

super().__init__(
pulse_type=self.__class__.__name__,
instance = SymbolicPulse(
pulse_type="Constant",
duration=duration,
parameters=parameters,
name=name,
limit_amplitude=limit_amplitude,
envelope=envelope_expr,
valid_amp_conditions=valid_amp_conditions_expr,
)
self.validate_parameters()
instance.validate_parameters()

return instance
53 changes: 47 additions & 6 deletions qiskit/qobj/converters/pulse_instruction.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,51 @@ class ParametricPulseShapes(Enum):
value is its mapping to the OpenPulse Command in Qiskit.
"""

gaussian = library.Gaussian
gaussian_square = library.GaussianSquare
drag = library.Drag
constant = library.Constant
gaussian = "Gaussian"
gaussian_square = "GaussianSquare"
drag = "Drag"
constant = "Constant"

@classmethod
def from_instance(
cls,
instance: Union[library.ParametricPulse, library.SymbolicPulse],
) -> "ParametricPulseShapes":
"""Get Qobj name from the pulse class instance.

Args:
instance: Symbolic or ParametricPulse class.

Returns:
Qobj name.

Raises:
QiskitError: When pulse instance is not recognizable type.
"""
if isinstance(instance, library.SymbolicPulse):
return cls(instance.pulse_type)
if isinstance(instance, library.parametric_pulses.Gaussian):
return cls("Gaussian")
if isinstance(instance, library.parametric_pulses.GaussianSquare):
return cls("GaussianSquare")
if isinstance(instance, library.parametric_pulses.Drag):
return cls("Drag")
if isinstance(instance, library.parametric_pulses.Constant):
return cls("Constant")
nkanazawa1989 marked this conversation as resolved.
Show resolved Hide resolved

raise QiskitError(f"'{instance}' is not valid pulse type.")

@classmethod
def to_type(cls, name: str) -> library.SymbolicPulse:
"""Get symbolic pulse class from the name.

Args:
name: Qobj name of the pulse.

Returns:
Corresponding class.
"""
return getattr(library, cls[name].value)


class ConversionMethodBinder:
Expand Down Expand Up @@ -386,7 +427,7 @@ def convert_play(self, shift, instruction):
if isinstance(instruction.pulse, (library.ParametricPulse, library.SymbolicPulse)):
command_dict = {
"name": "parametric_pulse",
"pulse_shape": ParametricPulseShapes(type(instruction.pulse)).name,
"pulse_shape": ParametricPulseShapes.from_instance(instruction.pulse).name,
"t0": shift + instruction.start_time,
"ch": instruction.channel.name,
"parameters": instruction.pulse.parameters,
Expand Down Expand Up @@ -683,7 +724,7 @@ def convert_parametric(self, instruction):
short_pulse_id = hashlib.md5(base_str.encode("utf-8")).hexdigest()[:4]
pulse_name = f"{instruction.pulse_shape}_{short_pulse_id}"

pulse = ParametricPulseShapes[instruction.pulse_shape].value(
pulse = ParametricPulseShapes.to_type(instruction.pulse_shape)(
**instruction.parameters, name=pulse_name
)
return instructions.Play(pulse, channel) << t0
Expand Down
36 changes: 10 additions & 26 deletions qiskit/qpy/binary_io/schedules.py
Original file line number Diff line number Diff line change
Expand Up @@ -91,32 +91,16 @@ def _read_symbolic_pulse(file_obj, version):
duration = value.read_value(file_obj, version, {})
name = value.read_value(file_obj, version, {})

# TODO remove this and merge subclasses into a single kind of SymbolicPulse
# We need some refactoring of our codebase,
# mainly removal of isinstance check and name access with self.__class__.__name__.
if pulse_type == "Gaussian":
pulse_cls = library.Gaussian
elif pulse_type == "GaussianSquare":
pulse_cls = library.GaussianSquare
elif pulse_type == "Drag":
pulse_cls = library.Drag
elif pulse_type == "Constant":
pulse_cls = library.Constant
else:
pulse_cls = library.SymbolicPulse

# Skip calling constructor to absorb signature mismatch in subclass.
instance = object.__new__(pulse_cls)
instance.duration = duration
instance.name = name
instance._limit_amplitude = header.amp_limited
instance._pulse_type = pulse_type
instance._params = parameters
instance._envelope = envelope
instance._constraints = constraints
instance._valid_amp_conditions = valid_amp_conditions

return instance
return library.SymbolicPulse(
pulse_type=pulse_type,
duration=duration,
parameters=parameters,
name=name,
limit_amplitude=header.amp_limited,
envelope=envelope,
constraints=constraints,
valid_amp_conditions=valid_amp_conditions,
)


def _read_alignment_context(file_obj, version):
Expand Down
Loading