Pulse limit_amplitude is confusing

[nkanazawa1989]

What should we add?

The class variable limit_amplitude in Pulse base class controls validation of max pulse amplitude. According to how test is written, likely this is designed as a global variable.

https://github.com/Qiskit/qiskit-terra/blob/a735680f69f7a2e2757dddbedf09f6d7c6a5e692/test/python/pulse/test_pulse_lib.py#L85-L93

However, the every pulse subclass constructor takes limit_amplitude, and this may confuse users because it looks like limit_amplitude is an instance variable.

https://github.com/Qiskit/qiskit-terra/blob/a735680f69f7a2e2757dddbedf09f6d7c6a5e692/qiskit/pulse/library/parametric_pulses.py#L103-L125

with pulse.build(backend) as sched:
    pulse.play(Gaussian(duration=160, amp=1.6, sigma=40, limit_amplitude=False), d0)
    pulse.play(Gaussian(duration=160, amp=0.8, sigma=40, limit_amplitude=True), d0)

If something like this is supported, limit_amplitude should be really instance variable, i.e. here the parameter validation happens only in the constructor, so setting limit_amplitude for the first pulse only affects the first pulse, if this variable is specified by every class instantiation.

I think limit_amplitude should be removed at least from all pulse subclass constructor. It would be great Qiskit offers some mechanism to manage preference of users, rather than directly attaching such preference to class.

[nkanazawa1989]

Likely we have this configuration class. I'm not sure if this mechanism can update values at run time, especially with unit tests where multiple workers may run different tests with different limit_amplitude.
https://github.com/Qiskit/qiskit-terra/blob/a735680f69f7a2e2757dddbedf09f6d7c6a5e692/qiskit/user_config.py#L24-L35

[jakelishman]

I don't have opinions on this, but I remember being confused about it too when I came across it. That said, I think it is both a class variable and and instance variable; if limit_amplitude is passed to a constructor then it's set as an instance attribute, which overrides the class-level attribute on lookup, but doesn't actually change the value:

In [1]: from qiskit.pulse.library import Gaussian
   ...: Gaussian.limit_amplitude = True
   ...: always_true = Gaussian(1, 1, 1, limit_amplitude=True)
   ...: delegates = Gaussian(1, 1, 1)
   ...: always_false = Gaussian(1, 1, 1, limit_amplitude=False)
   ...: always_true.limit_amplitude, delegates.limit_amplitude, always_false.limit_amplitude
Out[1]: (True, True, False)

In [2]: Gaussian.limit_amplitude = False
   ...: always_true.limit_amplitude, delegates.limit_amplitude, always_false.limit_amplitude
Out[2]: (True, False, False)

[nkanazawa1989]

Ah, that's good point. I changed the constructor in #7821 so that it always interact with class variable (otherwise serialization logic becomes terrible). But I think this is still not enough. I'll add deprecation warning in this release.