Removing complex values support from Calibrations #1067

TsafrirA · 2023-03-02T13:52:34Z

Summary

This PR aims to remove the use of complex amplitude for SymbolicPulse in calibration experiments. Most notable changes are to the HalfAngleCal experiment and the FixedFrquencyTransmon library. With these changes, support of complex values in general raises a PendingDeprecationWarning.

Details and comments

Qiskit Terra recently changed the representation of SymbolicPulse from complex amplitude to (amp,angle). Once the deprecation is completed, some calibration experiments will fail. Additionally, assignment of complex parameters in general has caused problems recently (See Qiskit-Terra issue 9187), and is also being phased out (See Qiskit-Terra PR 9735).

Most calibration experiments are oblivious to these changes, with the exception of HalfAngleCal and RoughAmplitudeCal. The library FixedFrequencyTransmon also has to conform with the new representation.

To create as little breaking changes as possible, the following were changed:

FixedFrequencyTransmon library was converted to the new representation. All experiments will work as they have been with it.
RoughAmplitudeCal was changed such that it will work for both real or complex amp, without changing the type of the value.
HalfAngleCal was changed to calibrate 'angle' instead of the complex amplitude. A user which uses the FixedFrequencyTransmon library will experience no change (except for the added parameters). A user which uses custom built schedules will experience an error. To simplify the transition, most likely scenarios (schedule with no angle parameter, cal_parameter_name="amp") will raise an informative error with explanation about the needed changes.

A PendingDeprecationWarning is raised with every initialization of ParameterValue with complex type value (which also covers addition of parameter value to a calibration). Note that Qiskit-Terra PR 9897 will also raise a warning from the Terra side, for all assignment of complex parameters.

Handling of loaded calibrations which do not conform to the new representation will be sorted out once PR #1120 is merged, as it introduces a major change in calibration loading.

CLAassistant · 2023-03-02T13:52:46Z

All committers have signed the CLA.

qiskit_experiments/calibration_management/basis_gate_library.py

wshanks · 2023-03-02T16:25:38Z

Regarding deprecation warnings, I have not seen much community use of Calibrations so far, so I would not put too much effort into backwards compatibility or a transition period. Some things that could be done:

I think one of the most likely cases of a user having a problem is trying to load old calibrations. If BasisGateLibrary.config stored the qiskit-experiments version (like BaseExperiment.config does), we could check for this and warn about the incompatibility if attempting to load an old library. We should consider adding the qiskit-experiments version to Calibrations.config and BasisGateLibrary.config.
Outside of warning at library load time, we could warn at Calibration.add_parameter_value if we see a complex valued parameter being added since after loading the libraries Calibrations.from_config will load all the values with add_parameter_value. This warning makes sense if we are completely dropping support for complex parameters (in user defined calibrations) and not just in the calibrations and libraries in the qiskit-experiments package.
We could possibly provide a conversion function that would take an old calibrations config and split out the complex amp parameters into amp and angle parameters. I don't know that there is enough demand for this though.

…qiskit-experiments into AmpAngleSymbolicPulses

…into AmpAngleSymbolicPulses � Conflicts: � test/calibration/test_setup_library.py

nkanazawa1989

Thanks @TsafrirA , this PR looks great. I really wanted to upgrade library pulses before terra 0.25 which we start actual deprecation for complex parameters.

nkanazawa1989 · 2023-04-13T03:00:52Z

qiskit_experiments/calibration_management/basis_gate_library.py


        if self._link_parameters:
-            y_amp, y_beta = 1.0j * x_amp, x_beta
+            y_amp, y_beta, y_angle = x_amp, x_beta, x_angle + np.pi / 2


I think angle must be separated to cope with mixer skew, i.e. angle difference is not exactly pi/2. So they should have independent angles but default value can be angle + pi/2 for y gate. Please confirm @wshanks

We could do that, but I think that would contradict the logic which was in place before. If the amplitudes were linked, then the angles were also linked in the previous implementation. Linking the angles will preserve the previous logic. This doesn't mean that the previous logic is bullet-proof. What do you think @wshanks ?

That's fair point. Since (sx, rz, cx) are enough to compose a universal set, I think experimentalist who requested sy to the library wants to correct for the mixer skew. But we can keep the logical change minimum and do this in the follow up.

Hmm, I think mixer skew is a lower level concern that we don't need to worry about too much in the qiskit layer where some details about the signal generation hardware are abstracted. If a user is worried about it, they could choose not to link the parameters. Perhaps a user would like to link amp but not angle...we can wait until someone requests that.

The main thing regarding angle is that sx and x can have different angles to account for non-linearity in the mixer or other components.

I think this is fine. The user has the option to unlink the parameters and therefore has full flexibility. We could be more granular as to what parameters we link and do not link but I would not go down that rabbit hole here.

qiskit_experiments/calibration_management/basis_gate_library.py

qiskit_experiments/calibration_management/parameter_value.py

qiskit_experiments/library/calibration/half_angle_cal.py

releasenotes/notes/adjust-symbolic-pulses-amp-angle-representation-f5c40007416cf938.yaml

…tion-f5c40007416cf938.yaml Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com>

nkanazawa1989

Thanks @TsafrirA this looks almost good to me. Minor comments for docs. If you are happy with them I'll merge this PR. We can come back to handling of sy angle link in future.

qiskit_experiments/library/calibration/half_angle_cal.py

releasenotes/notes/adjust-symbolic-pulses-amp-angle-representation-f5c40007416cf938.yaml

Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com>

…tion-f5c40007416cf938.yaml Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com>

nkanazawa1989

Thanks Tsafrir. I think this PR is good to go. @eggerdj @wshanks if you are happy with this upgrade I'll merge this PR.

wshanks · 2023-04-19T13:55:24Z

qiskit_experiments/calibration_management/basis_gate_library.py


        if self._link_parameters:
-            y_amp, y_beta = 1.0j * x_amp, x_beta
+            y_amp, y_beta, y_angle = x_amp, x_beta, x_angle + np.pi / 2


Hmm, I think mixer skew is a lower level concern that we don't need to worry about too much in the qiskit layer where some details about the signal generation hardware are abstracted. If a user is worried about it, they could choose not to link the parameters. Perhaps a user would like to link amp but not angle...we can wait until someone requests that.

The main thing regarding angle is that sx and x can have different angles to account for non-linearity in the mixer or other components.

qiskit_experiments/calibration_management/basis_gate_library.py

wshanks · 2023-04-19T14:06:50Z

test/library/calibration/test_half_angle.py

+from qiskit_experiments.test.pulse_backend import SingleTransmonTestBackend
+
+
+class TestHalfAngleCal(QiskitExperimentsTestCase):


Out of scope for this PR, but it would be good to have a basic test of the class working.

I agree, I already opened an issue - #1123 .

releasenotes/notes/adjust-symbolic-pulses-amp-angle-representation-f5c40007416cf938.yaml

qiskit_experiments/library/calibration/half_angle_cal.py

releasenotes/notes/adjust-symbolic-pulses-amp-angle-representation-f5c40007416cf938.yaml

eggerdj

LGTM. Minor comments only.

eggerdj · 2023-04-20T22:00:36Z

qiskit_experiments/calibration_management/basis_gate_library.py


        if self._link_parameters:
-            y_amp, y_beta = 1.0j * x_amp, x_beta
+            y_amp, y_beta, y_angle = x_amp, x_beta, x_angle + np.pi / 2


I think this is fine. The user has the option to unlink the parameters and therefore has full flexibility. We could be more granular as to what parameters we link and do not link but I would not go down that rabbit hole here.

qiskit_experiments/calibration_management/basis_gate_library.py

qiskit_experiments/library/calibration/rough_amplitude_cal.py

…tion-f5c40007416cf938.yaml Co-authored-by: Will Shanks <wshaos@posteo.net>

Co-authored-by: Will Shanks <wshaos@posteo.net>

…tion-f5c40007416cf938.yaml Co-authored-by: Will Shanks <wshaos@posteo.net>

Co-authored-by: Daniel J. Egger <38065505+eggerdj@users.noreply.github.com>

Co-authored-by: Will Shanks <wshaos@posteo.net>

…tion-f5c40007416cf938.yaml Co-authored-by: Will Shanks <wshaos@posteo.net>

wshanks

Looks good! I just suggested one small grammar change.

I wonder if we should have a release note when we increase the minimum version of terra?

releasenotes/notes/adjust-symbolic-pulses-amp-angle-representation-f5c40007416cf938.yaml

wshanks · 2023-05-08T13:45:35Z

Oops, in my last comment, I meant "release note", not "release" (I edited it now but I am just commenting since the meaning is so different).

…tion-f5c40007416cf938.yaml Co-authored-by: Will Shanks <wshaos@posteo.net>

releasenotes/notes/adjust-symbolic-pulses-amp-angle-representation-f5c40007416cf938.yaml

…tion-f5c40007416cf938.yaml Co-authored-by: Will Shanks <wshaos@posteo.net>

eggerdj

LGTM.

TsafrirA and others added 2 commits March 2, 2023 15:22

Minimal Example

ba83e0e

Merge branch 'Qiskit:main' into master

deab57d

wshanks reviewed Mar 2, 2023

View reviewed changes

qiskit_experiments/calibration_management/basis_gate_library.py Show resolved Hide resolved

TsafrirA and others added 9 commits March 9, 2023 09:19

Merge branch 'Qiskit:main' into AmpAngleSymbolicPulses

7e136f9

Completing the changes.

415f5f6

Merge branch 'main' into AmpAngleSymbolicPulses

52cdb30

Lint.

c9e3955

Merge branch 'AmpAngleSymbolicPulses' of https://github.com/TsafrirA/…

de4311e

…qiskit-experiments into AmpAngleSymbolicPulses

Adjust to main

75915c6

Merge branch 'main' of https://github.com/TsafrirA/qiskit-experiments …

ec0a99d

…into AmpAngleSymbolicPulses � Conflicts: � test/calibration/test_setup_library.py

minor style & doc changes

bc4c3eb

Release Notes

522aaba

TsafrirA mentioned this pull request Apr 3, 2023

HalfAngleCal has no test module #1123

Open

TsafrirA added 2 commits April 3, 2023 15:24

Tests Correction

ef4f228

Add PendingDeprecationWarning

2ca5316

TsafrirA changed the title ~~[WIP] Removing complex amplitude from Calibrations~~ Removing complex values support from Calibrations Apr 4, 2023

nkanazawa1989 requested changes Apr 13, 2023

View reviewed changes

TsafrirA and others added 6 commits April 16, 2023 11:33

Update releasenotes/notes/adjust-symbolic-pulses-amp-angle-representa…

36d20f7

…tion-f5c40007416cf938.yaml Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com>

Update releasenotes/notes/adjust-symbolic-pulses-amp-angle-representa…

7987a6a

…tion-f5c40007416cf938.yaml Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com>

Update releasenotes/notes/adjust-symbolic-pulses-amp-angle-representa…

ed7970b

…tion-f5c40007416cf938.yaml Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com>

Minor corrections

522ebfa

Minor corrections

7cf2702

Merge branch 'main' into AmpAngleSymbolicPulses

e030ee2

nkanazawa1989 reviewed Apr 18, 2023

View reviewed changes

qiskit_experiments/library/calibration/half_angle_cal.py Outdated Show resolved Hide resolved

releasenotes/notes/adjust-symbolic-pulses-amp-angle-representation-f5c40007416cf938.yaml Outdated Show resolved Hide resolved

TsafrirA and others added 3 commits April 19, 2023 02:49

Update qiskit_experiments/library/calibration/half_angle_cal.py

147db6f

Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com>

Update releasenotes/notes/adjust-symbolic-pulses-amp-angle-representa…

5e1e3e7

…tion-f5c40007416cf938.yaml Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com>

Merge branch 'main' into AmpAngleSymbolicPulses

d9656d8

nkanazawa1989 approved these changes Apr 19, 2023

View reviewed changes

wshanks reviewed Apr 19, 2023

View reviewed changes

coruscating added this to the Release 0.6 milestone Apr 19, 2023

eggerdj reviewed Apr 20, 2023

View reviewed changes

TsafrirA and others added 12 commits April 25, 2023 21:51

Update releasenotes/notes/adjust-symbolic-pulses-amp-angle-representa…

57cc08c

…tion-f5c40007416cf938.yaml Co-authored-by: Will Shanks <wshaos@posteo.net>

Update releasenotes/notes/adjust-symbolic-pulses-amp-angle-representa…

d089d51

…tion-f5c40007416cf938.yaml Co-authored-by: Will Shanks <wshaos@posteo.net>

Update qiskit_experiments/library/calibration/half_angle_cal.py

755a25e

Co-authored-by: Will Shanks <wshaos@posteo.net>

Update releasenotes/notes/adjust-symbolic-pulses-amp-angle-representa…

8722e79

…tion-f5c40007416cf938.yaml Co-authored-by: Will Shanks <wshaos@posteo.net>

Update qiskit_experiments/library/calibration/rough_amplitude_cal.py

bdeead6

Co-authored-by: Daniel J. Egger <38065505+eggerdj@users.noreply.github.com>

Update qiskit_experiments/library/calibration/half_angle_cal.py

fba9e3d

Co-authored-by: Will Shanks <wshaos@posteo.net>

Update releasenotes/notes/adjust-symbolic-pulses-amp-angle-representa…

064b17c

…tion-f5c40007416cf938.yaml Co-authored-by: Will Shanks <wshaos@posteo.net>

Update releasenotes/notes/adjust-symbolic-pulses-amp-angle-representa…

4b1ff54

…tion-f5c40007416cf938.yaml Co-authored-by: Will Shanks <wshaos@posteo.net>

Merge branch 'main' into AmpAngleSymbolicPulses

b0e8cad

Update requirements

2cc99f3

Merge branch 'main' into AmpAngleSymbolicPulses

a3e1e62

Release notes update

a798ae8

wshanks approved these changes May 8, 2023

View reviewed changes

releasenotes/notes/adjust-symbolic-pulses-amp-angle-representation-f5c40007416cf938.yaml Outdated Show resolved Hide resolved

TsafrirA and others added 2 commits May 8, 2023 16:47

Update releasenotes/notes/adjust-symbolic-pulses-amp-angle-representa…

5088869

…tion-f5c40007416cf938.yaml Co-authored-by: Will Shanks <wshaos@posteo.net>

Release notes - terra bump

487d6a4

wshanks approved these changes May 8, 2023

View reviewed changes

wshanks reviewed May 8, 2023

View reviewed changes

releasenotes/notes/adjust-symbolic-pulses-amp-angle-representation-f5c40007416cf938.yaml Outdated Show resolved Hide resolved

wshanks reviewed May 8, 2023

View reviewed changes

releasenotes/notes/adjust-symbolic-pulses-amp-angle-representation-f5c40007416cf938.yaml Outdated Show resolved Hide resolved

TsafrirA and others added 2 commits May 8, 2023 22:16

Update releasenotes/notes/adjust-symbolic-pulses-amp-angle-representa…

5e477ef

…tion-f5c40007416cf938.yaml Co-authored-by: Will Shanks <wshaos@posteo.net>

Update releasenotes/notes/adjust-symbolic-pulses-amp-angle-representa…

330219b

…tion-f5c40007416cf938.yaml Co-authored-by: Will Shanks <wshaos@posteo.net>

eggerdj approved these changes May 9, 2023

View reviewed changes

eggerdj added this pull request to the merge queue May 9, 2023

Merged via the queue into qiskit-community:main with commit 5459cc1 May 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removing complex values support from Calibrations #1067

Removing complex values support from Calibrations #1067

TsafrirA commented Mar 2, 2023 •

edited

Loading

CLAassistant commented Mar 2, 2023 •

edited

Loading

wshanks commented Mar 2, 2023

nkanazawa1989 left a comment

nkanazawa1989 Apr 13, 2023

TsafrirA Apr 16, 2023

nkanazawa1989 Apr 17, 2023

wshanks Apr 19, 2023

eggerdj Apr 20, 2023

nkanazawa1989 left a comment

nkanazawa1989 left a comment

wshanks Apr 19, 2023

wshanks Apr 19, 2023

TsafrirA Apr 25, 2023

eggerdj left a comment

eggerdj Apr 20, 2023

wshanks left a comment •

edited

Loading

wshanks commented May 8, 2023

eggerdj left a comment

		from qiskit_experiments.test.pulse_backend import SingleTransmonTestBackend


		class TestHalfAngleCal(QiskitExperimentsTestCase):

Removing complex values support from Calibrations #1067

Removing complex values support from Calibrations #1067

Conversation

TsafrirA commented Mar 2, 2023 • edited Loading

Summary

Details and comments

CLAassistant commented Mar 2, 2023 • edited Loading

wshanks commented Mar 2, 2023

nkanazawa1989 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nkanazawa1989 left a comment

Choose a reason for hiding this comment

nkanazawa1989 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eggerdj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wshanks left a comment • edited Loading

Choose a reason for hiding this comment

wshanks commented May 8, 2023

eggerdj left a comment

Choose a reason for hiding this comment

TsafrirA commented Mar 2, 2023 •

edited

Loading

CLAassistant commented Mar 2, 2023 •

edited

Loading

wshanks left a comment •

edited

Loading