-
Notifications
You must be signed in to change notification settings - Fork 127
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Saving and loading calibrations data #1120
Saving and loading calibrations data #1120
Conversation
…decoder to handle Qiskit data types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this! Have you tried checking if the links between parameters are preserved after a round trip serialize/deserialize? Can you add a few tests for that?
file_path = file_prefix + ".json" | ||
if os.path.isfile(file_path) and not overwrite: | ||
raise CalibrationError(f"{file_path} already exists. Set overwrite to True.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a test to check that we raise if overwrite
is False and there already exists a file with this name?
for sched in model.schedules: | ||
cals.add_schedule( | ||
schedule=sched.data, | ||
qubits=sched.qubits if len(sched.qubits) != 0 else None, | ||
num_qubits=sched.num_qubits, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this keep any parameter links that we initially had?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. QPY preserves parameter UUID and it serializes the schedule through QPY format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok good. Thanks, do we have a test on this?
releasenotes/notes/feature-support-calibrations-roundtrip-47f09bd9ff803479.yaml
Show resolved
Hide resolved
releasenotes/notes/feature-support-calibrations-roundtrip-47f09bd9ff803479.yaml
Show resolved
Hide resolved
- Docs update - Remove schema for schedule and move qubit info to metadata - Change function names Co-authored-by: Daniel Egger <[email protected]>
- Fix Calibrations eq logic. This logic uses json dump for dict serialization which doesn't work for complex parameter value. - Fix test without deprecation. Revert logic change to Calibrations.config method which is no longer used anyways. - Fix wrong import
### 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](Qiskit/qiskit#9187)), and is also being phased out (See Qiskit-Terra PR [9735](Qiskit/qiskit#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](Qiskit/qiskit#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. --------- Co-authored-by: Naoki Kanazawa <[email protected]> Co-authored-by: Will Shanks <[email protected]> Co-authored-by: Daniel J. Egger <[email protected]>
…ure/calibration-save
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this is good. I have a few comments mainly related to the messaging.
Co-authored-by: Daniel J. Egger <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks!
Summary
This PR adds support for full roundtrip of
Calibrations
instance through JSON file format. Even though this PR is blocked by the following PR, this branch is ready to streamline calibration tasks.This is blocked by Qiskit/qiskit#9890
Details and comments
Sample code for testing
Checkout my Terra folk https://github.com/nkanazawa1989/qiskit-terra/tree/feature/qpy-schedule-reference