Deprecate limit_amplitude constructor argument

[nkanazawa1989]

Summary

This PR deprecates limit_amplitude which is a constructor argument of pulse subclass. This option is confusing because user may unintentionally update class variable, or sometime instance variable.

Details and comments

Fix #7932

[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

[coveralls]

Pull Request Test Coverage Report for Build 2185123677

• 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
• 1 unchanged line in 1 file lost coverage.
• Overall coverage decreased (-0.001%) to 83.945%

---

Files with Coverage Reduction	New Missed Lines	%
qiskit/pulse/library/parametric_pulses.py	1	96.23%

Totals
Change from base Build 2184748177:	-0.001%
Covered Lines:	54214
Relevant Lines:	64583

---

💛 - Coveralls

[jakelishman]

I don't actually use the code, so I'm not the best-placed person to be involved in the decision, but certainly from the outside it feels like instance-level control of limiting the amplitude is a useful thing to have. Setting the class-level attribute affects already-instantiated classes, which is (to me) very surprising to be the suggested path.

[nkanazawa1989]

From control electronics point of view, instance level amplitude limit is bit unrealistic. Historically this control parameter is introduced to manage pulse amplitude in SI value, which is based upon the request from the community.

Usually, when we design the controller, we decide how we control the pulse amplitude. In IBM provider, amplitude indicates gradation of quantized amplitude (i.e. level of AWG output) in the digital world which will be converted into analog signal by mixing with local oscillator or LO signal (the power of the oscillator is not controllable from Qiskit). This means amplitude beyond 1.0 should not exist because we cannot output power exceeding LO power, i.e. 1.0 indicates 100% of LO power.

As such, we cannot mix different amplitude representations in the same pulse channel because of physically implemented hardware. This means limit_amplitude is the control parameter of the Channel object rather than Pulse instance. However, since pulse instance is not aware of channel being associated with, thus it cannot retrieve limit amplitude policy from channel at instantiation.

The assumption behind the PR is -- if one design a control electronics that employs SI amplitude, then one will not implement the whole control system with channels with different amplitude representations. Then, limit_amplitude is something that is set before one start building pulse programs.

[nkanazawa1989]

This makes me think we can delay the validation of amplitude or entire pulse parameters. If Play(pulse, channel) instruction instance does the validation, we can get amplitude limit policy from the cannel, which physically makes sense.

[eggerdj]

Why the change to GaussianSqure?

[nkanazawa1989]

Good catch this is a typo.

[eggerdj]

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.

[nkanazawa1989]

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)

[taalexander]

I don't actually use the code, so I'm not the best-placed person to be involved in the decision, but certainly from the outside, it feels like instance-level control of limiting the amplitude is a useful thing to have. Setting the class-level attribute affects already-instantiated classes, which is (to me) very surprising to be the suggested path.

I tend to agree with @jakelishman, why are we switching this to a class variable? What about the situation where we might have two users (say one a simulator, the other a real-backend) in the same process? Or the situation where we don't want to limit to 1 due to an optimization routine, that we later rewrite all pulses that exceed 1 to be 1?

[nkanazawa1989]

The issue of current implementation is delegation that @jakelishman pointed out. I'm currently cleaning up the data structure of pulse schedule before starting the QPY serialization (i.e. in circuit QPY we have to maintain tons of branching logic to cover many variants of instruction data, so I'm trying to make pulse data structure bit more strict). In this sense, I find the limit_amplitude is quite problematic, since it's difficult to write clean code to infer situation of delegation.

Another simple option would be just turning it into instance variable, but this means a coder must set limit_amplitude=True to every pulse instance which is very cumbersome. From hardware point of view I think this option belongs to PulseChannel rather than pulse, but as you mentioned

What about the situation where we might have two users (say one a simulator, the other a real-backend) in the same process?

this could be argument of .run.

[jakelishman]

If it's just the delegation and QPY serialisation we're primarily worried about, then the instance-level attributes will probably be easiest to standardise around rather than the class-level attributes - QPY currently deals with objects that can be serialised with no global state.

Perhaps a more sensible state is if the class-level attribute is used during object instantiation as the default, but there's no delegation after that (e.g. something like self.limit_amplitude = type(self).limit_amplitude if limit_amplitude is None else limit_amplitude). That still lets you set things on a larger scale (though personally I find class variables aren't much better than true globals - it's asking for problems in multithreading), but stops the issue of later changes affecting already created objects. QPY wouldn't need to worry any more, because it could just restore the state it found.

That doesn't solve the problem you mentioned about the amplitude limiting being more a property of a Channel than a Pulse, but I'm not sure that's completely solvable without a larger re-write. At any rate, removing the instance variables and making it a global only means we can't simultaneously describe one Channel with limiting and one without.

[nkanazawa1989]

In #7821 limit_amplitude is turned into an instance variable (self._limit_amplitude). This is sufficient for QPY serialization.

[wshanks]

I'll just throw my opinion in here in case the larger design gets revisited later.

I don't see much of a use case for setting limit_amplitude on individual pulses. The current behavior is just to raise an exception if an amplitude over the limit is detected. For running on IBM hardware at least, any amplitude over the limit will cause the provider to give back an error. Getting the error in terra saves you sitting through the queue, so you want to check every pulse. For a pulse simulator, there is probably no limit (other than maximum allowed floating point value) so you don't want to check any pulse.

@taalexander imagined a more interesting case where amplitudes could be rescaled. An interesting variant would be a provider that silently scaled down amplitudes to the max values. Perhaps you would not care that some pulses were getting scaled but would want to make sure others were exactly as specified and so marking individual pulses would make sense?

I agree that basing the behavior off a class level variable is not good. @nkanazawa1989 has talked about adding a pass manager for pulse schedules. I think amplitude limit checking would fit better as a validation pass than something that is checked at class instantation and parameter assignment. Perhaps such a pass could take filtering arguments for choosing which pulses to limit or not so that this does not need to be set on each individual pulse.

[taalexander]

I think you're right about how to approach amplitude verification @wshanks it should also have performance improvements