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.

Sign up for GitHub

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

Deprecate limit_amplitude constructor argument #7948

Closed
wants to merge 2 commits into from
Closed

Deprecate limit_amplitude constructor argument #7948

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
17d2671
deprecate limit_amplitude
nkanazawa1989 Apr 18, 2022
435adae
add test for deprecation
nkanazawa1989 Apr 18, 2022
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
7 changes: 7 additions & 0 deletions qiskit/pulse/library/pulse.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
"""Pulses are descriptions of waveform envelopes. They can be transmitted by control electronics
to the device.
"""
import warnings
from abc import ABC, abstractmethod
from typing import Dict, Optional, Any, Tuple, Union

Expand Down Expand Up @@ -47,6 +48,12 @@ def __init__(
self.duration = duration
self.name = name
if limit_amplitude is not None:
warnings.warn(
"Instantiating a pulse instance with 'limit_amplitude' has been deprecated. "
f"Please directly update class variable '{self.__class__.__name__}.limit_amplitude.'",
DeprecationWarning,
stacklevel=4,
)
Comment on lines +51 to +56
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it not be more flexible to deprecate the class variable instead and do this validation from one instance to the next? With this PR you either validate all your pulses or none of them. On the other hand, I'm not sure if we need the flexibility.

Copy link
Contributor Author

@nkanazawa1989 nkanazawa1989 Apr 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah that's the point I'd like to hear your thought. I think instance wise validation is not realistic from viewpoint of hardware, because we cannot dynamically change representation in the same channel. I think limit_amplitude is more like a instance variable of channel rather than pulse.
#7948 (comment)

self.limit_amplitude = limit_amplitude

@property
Expand Down
14 changes: 14 additions & 0 deletions releasenotes/notes/deprecate_limit_amplitude-454f3ea455f3a37e.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
---
deprecations:
- |
Instantiating a :class:`Pulse` subclass instance with ``limit_amplitude`` has been deprecated.
Though this is a constructor argument, it implicitly updates other instance settings
since this actually updates a class variable. To avoid confusion, user must explicitly
update the class variable. For example,

.. code-block:: python

from qiskit.pulse.library import Pulse, Gaussian

Pulse.limit_amplitude = False
waveform = Gaussian(duration=160, amp=120, sigma=40)
56 changes: 28 additions & 28 deletions test/python/pulse/test_pulse_lib.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,16 +81,9 @@ def test_pulse_limits(self):
with self.assertRaises(PulseError):
Waveform(invalid_const * np.exp(1j * 2 * np.pi * np.linspace(0, 1, 1000)))

invalid_const = 1.1
Waveform.limit_amplitude = False
wave = Waveform(invalid_const * np.exp(1j * 2 * np.pi * np.linspace(0, 1, 1000)))
self.assertGreater(np.max(np.abs(wave.samples)), 1.0)
with self.assertRaises(PulseError):
wave = Waveform(
invalid_const * np.exp(1j * 2 * np.pi * np.linspace(0, 1, 1000)),
limit_amplitude=True,
)
Waveform.limit_amplitude = True
with patch("qiskit.pulse.library.pulse.Pulse.limit_amplitude", new=False):
wave = Waveform(invalid_const * np.exp(1j * 2 * np.pi * np.linspace(0, 1, 1000)))
self.assertGreater(np.max(np.abs(wave.samples)), 1.0)

# Test case where data is converted to python types with complex as a list
# with form [re, im] and back to a numpy array.
Expand Down Expand Up @@ -282,37 +275,44 @@ def test_hash_generation(self):

def test_gaussian_limit_amplitude(self):
"""Test that the check for amplitude less than or equal to 1 can be disabled."""
waveform = Gaussian(duration=100, sigma=1.0, amp=1.1 + 0.8j, limit_amplitude=False)
self.assertGreater(np.abs(waveform.amp), 1.0)

with self.assertRaises(PulseError):
Gaussian(duration=100, sigma=1.0, amp=1.1 + 0.8j, limit_amplitude=True)
GaussianSquare(duration=100, sigma=1.0, amp=1.1 + 0.8j, width=10)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the change to GaussianSqure?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch this is a typo.


with patch("qiskit.pulse.library.pulse.Pulse.limit_amplitude", new=False):
waveform = GaussianSquare(duration=100, sigma=1.0, amp=1.1 + 0.8j, width=10)
self.assertGreater(np.abs(waveform.amp), 1.0)

def test_gaussian_square_limit_amplitude(self):
"""Test that the check for amplitude less than or equal to 1 can be disabled."""
waveform = GaussianSquare(
duration=100, sigma=1.0, amp=1.1 + 0.8j, width=10, limit_amplitude=False
)
self.assertGreater(np.abs(waveform.amp), 1.0)

with self.assertRaises(PulseError):
GaussianSquare(duration=100, sigma=1.0, amp=1.1 + 0.8j, width=10, limit_amplitude=True)
GaussianSquare(duration=100, sigma=1.0, amp=1.1 + 0.8j, width=10)

with patch("qiskit.pulse.library.pulse.Pulse.limit_amplitude", new=False):
waveform = GaussianSquare(duration=100, sigma=1.0, amp=1.1 + 0.8j, width=10)
self.assertGreater(np.abs(waveform.amp), 1.0)

def test_drag_limit_amplitude(self):
"""Test that the check for amplitude less than or equal to 1 can be disabled."""
waveform = Drag(duration=100, sigma=1.0, beta=1.0, amp=1.1 + 0.8j, limit_amplitude=False)
self.assertGreater(np.abs(waveform.amp), 1.0)

with self.assertRaises(PulseError):
Drag(duration=100, sigma=1.0, beta=1.0, amp=1.1 + 0.8j, limit_amplitude=True)
Drag(duration=100, sigma=1.0, beta=1.0, amp=1.1 + 0.8j)

with patch("qiskit.pulse.library.pulse.Pulse.limit_amplitude", new=False):
waveform = Drag(duration=100, sigma=1.0, beta=1.0, amp=1.1 + 0.8j)
self.assertGreater(np.abs(waveform.amp), 1.0)

def test_constant_limit_amplitude(self):
"""Test that the check for amplitude less than or equal to 1 can be disabled."""
waveform = Constant(duration=100, amp=1.1 + 0.8j, limit_amplitude=False)
self.assertGreater(np.abs(waveform.amp), 1.0)

with self.assertRaises(PulseError):
Constant(duration=100, amp=1.1 + 0.8j, limit_amplitude=True)
Constant(duration=100, amp=1.1 + 0.8j)

with patch("qiskit.pulse.library.pulse.Pulse.limit_amplitude", new=False):
waveform = Constant(duration=100, amp=1.1 + 0.8j)
self.assertGreater(np.abs(waveform.amp), 1.0)

def test_deprecate_instantiating_with_limit(self):
"""Test if deprecation warning is thrown when instance is created with limit option."""
with self.assertWarns(DeprecationWarning):
GaussianSquare(duration=100, sigma=1.0, amp=1.1 + 0.8j, width=10, limit_amplitude=False)


# pylint: disable=invalid-name,unexpected-keyword-arg
Expand Down