Remove symbolic pulse subclass implementation

[nkanazawa1989]

Summary

This PR removes implementation of symbolic pulse subclasses. These classes become a factory of SymbolicPulse, and envelope kind-wise subclassing will be prohibited (classes are not actually removed, but never instantiated). Accordingly, use of isinstance has been also discouraged by the user warning.

pulse = Gaussian(160, 0.1, 40)

isinstance(pulse, Gaussian)  # True

with user warning

UserWarning: Typechecking with the symbolic pulse subclass will be deprecated. 'Gaussian' subclass instance is turned into SymbolicPulse instance. Use self.pulse_type == 'Gaussian' instead.

(future plan)
After sufficient transition period from type information to .pulse_type, we will promote the user warning to deprecation warning, then eventually we will remove the metaclass and convert pulse subclasses into python functions.

close #8261

Details and comments

[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
• @mtreinish
• @nkanazawa1989

[coveralls]

Pull Request Test Coverage Report for Build 3132260277

• 56 of 72 (77.78%) changed or added relevant lines in 5 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.04%) to 84.406%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/pulse/library/symbolic_pulses.py	32	34	94.12%
qiskit/visualization/pulse_v2/generators/waveform.py	10	15	66.67%
qiskit/qobj/converters/pulse_instruction.py	12	21	57.14%

Totals
Change from base Build 3132258269:	0.04%
Covered Lines:	59615
Relevant Lines:	70629

---

💛 - Coveralls

[wshanks]

I put a few minor comments in but this looks good to me!

Since a terra release just come out, this change will be staggered one release behind the conversion to SymbolicPulse which seems like good pacing of changes.

[wshanks]

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

[nkanazawa1989]

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.

[wshanks]

Should this be classified as a deprecation rather than an upgrade?

[nkanazawa1989]

This is not deprecated yet. I heard we need to prepare alternative before deprecation, then after 1 release we can deprecate it and finally remove. This asks user to replace isinstance with .pulse_type in their code, so I think this is upgrade. Perhaps I'm wrong. Could you please advice? @mtreinish

[wshanks]

Okay, I see the policy here (maybe you should use PendingDeprecationWarning instead of UserWarning?). Actually, I didn't understand what the upgrade section meant until I read about it here (I was thinking upgraded features rather than suggestions for how to upgrade). So maybe it is standard to describe the upgrade procedure in one release in the upgrade section and then flag the old feature in the deprecation section in the next release.

I wonder about the note above now. that I understand feature vs upgrade better The point about the subclasses now being factories is more of a feature than something requiring action (outside of the instance check described in the second note). Maybe you should emphasize more that SymbolicPulse should not be subclassed and that Gaussian.__class__.__name__ will stop working because those things do require user action (though I don't think either is common).

[mtreinish]

I agree with @wshanks here, if we're still supporting the old method but adding support for the new factory construction that's a feature note. We should reverse upgrade notes for things where a user would need to take action when upgrading from release N -> N+1. Like when we remove support for the old subclasses (after the deprecation) that would be an upgrade note since the user would have to update their code if they were using the subclasses.

[wshanks]

Looks good to me now!

[jakelishman]

You possibly also want to override __subclasscheck__ if you're being thorough?

[nkanazawa1989]

I think this is overkill in terms of its usage. Usually we don't inherit pulse class from another symbolic pulse subclass, and thus class is somewhat obvious.

[jakelishman]

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

[jakelishman]

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 👍.

[nkanazawa1989]

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 :)

[nkanazawa1989]

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.

[wshanks]

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.

[nkanazawa1989]

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.

[wshanks]

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?

[nkanazawa1989]

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.

[jakelishman]

I'm not super sure what this magic value represents, but it appears a couple of times. Should it be in the other enum of magic values that appeared earlier?

[nkanazawa1989]

I don't take care of these information too much because they are not used for now. I was planning to prepare Plotly or Bokeh visualizer since they are good at visualizing very long pulse data, i.e. they can scroll the window, pop-up the pulse detail in another window. Note that v2 drawer is backend agnostic, and this metadata is prepared for such interactive backend which has never existed :(

[jakelishman]

If pulse_type was already in use as an alias, it might be a little safer to do the instance check on some private internal string instead, just to avoid cases where the user overrides the alias and still wants the type check to work.

[nkanazawa1989]

I wanted to remove the usage of subclass since this will never work properly with custom pulse defined by user or third party library, as described here: #8278 (comment)

[mtreinish]

Overall this LGTM, I just had one inline comment about the release notes. I think the first note is miscategorized as an update note. We should be documenting the new factory construction while this is strictly adding the new functionality. When we change the warning to DeprecationWarning or something else that shows up with the default filters that will be a deprecation note, and when we remove the subclasses we can make that an upgrade note. I think I'm good to merge after the release note update, unless @wshanks has other comments

[mtreinish]

I agree with @wshanks here, if we're still supporting the old method but adding support for the new factory construction that's a feature note. We should reverse upgrade notes for things where a user would need to take action when upgrading from release N -> N+1. Like when we remove support for the old subclasses (after the deprecation) that would be an upgrade note since the user would have to update their code if they were using the subclasses.

[wshanks]

I think this is good and don't have further comments.

[nkanazawa1989]

Thanks @mtreinish @wshanks I updated the release note and fixed failing tests.

[mtreinish]

LGTM, thanks for updating things