[WIP] Fixing symbolic pulse equating for non-unique representations

[TsafrirA]

Summary

Qiskit-Terra PR #9002, converted the library symbolic pulses from a complex amp representation to a real (float) amp,angle representation. To overcome the non-unique nature of the new representation (particularly, when negative amp is allowed), this PR adds two optional attributes canonical_params and excluded_params - the first is used for extra parameters which remove the degeneracy, and the second to indicate which parameters don't necessarily have to be equa.

PR #9247 presented a different option for this - using SymbolicExpressions - and was closed.
PR #9314 presented a different approach altogether - not a general solution for every non unique representation, but rather a focused solution for pulses with amp,angle representation via a new subclass - leading to this PR being closed.

Details and comments

Qiskit-Terra PR #9002, converted the library symbolic pulses from a complex amp representation to a real (float) amp,angle representation. To better support several experiments in Qiskit-Experiments, the amp parameter is allowed to take both positive and negative values. This decision creates a non-unique representation for the pulses, where a pi shift of angle and a sign flip of amp have the same effect (the issue obviously also exists for 2*pi shift of angle). For example, the following two pulses

from qiskit.pulse.library import Gaussian
import numpy as np

g1 = Gaussian(duration=100, sigma=50, amp=-1, angle=0)
g2 = Gaussian(duration=100, sigma=50, amp=1, angle=np.pi)

are not the same when equated (because their parameters are different):

g1 == g2 # False

but they represent the same waveform:

np.all(g1.get_waveform().samples == g2.get_waveform().samples) # True

This PR sets to correct this situation, by introducing two new attributes to the SymbolicPulse class - canonical_params and excluded_params. 'canonical_params' is a list of values or ParametricExpressions which are equated when two pulses are equated. excluded_params, on the other hand, is a tuple of strings matching keys of parameters which should be ignored in the equating process of the two pulses. For library pulses, the new parameters are assigned the following values: canonical_params=[amp * np.exp(1j * angle)] and excluded_params=("amp","angle").
By adjusting the parameter assignment procedure for symbolic pulses, this method works for ParameterExpressions arguments for canonical_params.

Returning to the example above, the new PR yields:

g1 == g2 # True

• Correct QPY save\load (including for older versions of terra).
• Input verification with SymbolicPulse instantiation?

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

[coveralls]

Pull Request Test Coverage Report for Build 3670658549

• 35 of 37 (94.59%) changed or added relevant lines in 2 files are covered.
• 2 unchanged lines in 1 file lost coverage.
• Overall coverage increased (+0.04%) to 84.605%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/pulse/library/symbolic_pulses.py	30	32	93.75%

Files with Coverage Reduction	New Missed Lines	%
src/sabre_swap/layer.rs	2	98.95%

Totals
Change from base Build 3669391724:	0.04%
Covered Lines:	63845
Relevant Lines:	75462

---

💛 - Coveralls

[nkanazawa1989]

Thanks @TsafrirA updated logic looks good to me. Do you want to remove draft now?

[nkanazawa1989]

This is nitpicky, but the logic seems to me bit inefficient. It first fully evaluates the dict equality and then compares the canonicals and rest of dict items. Maybe just return self._equate_paramters(other) without if clause enough? Probably my suggestion is wrong because evaluation of builtin dict equality might be faster.