-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Add support for pulse reference to QPY #9890
Add support for pulse reference to QPY #9890
Conversation
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:
|
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 the code LGTM, the approach for storing the references seems sensible. I just had a few comments in the documentation.
qiskit/qpy/__init__.py
Outdated
Conventional :ref:`qpy_schedule_block` data model is preserved, | ||
but it is immediately followed by an extra :ref:`qpy_mapping` utf8 bytes block | ||
representing the data of the referenced subroutines. |
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.
We probably should also document that the SCHEDULE_BLOCK_HEADER
now has a num_references
field in it too?
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 catching this. I forgot to remove SCHEDULE_BLOCK_HEADER_v7
(I was thinking different data model in the beginning). Since we already have mapping data model in QPY (key value must be string), I just reused it to express the reference mapping. Namely schedule block and references are stored back to back but they are different data section.
qiskit/qpy/type_keys.py
Outdated
@@ -303,6 +309,11 @@ class ScheduleOperand(TypeKeyBase): | |||
# Data format of these object is somewhat opaque and not defiend well. | |||
# It's rarely used in the Qiskit experiements. Of course these can be added later. | |||
|
|||
# A temp hack. If pulse instruction operands involve a string, |
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.
Heh, well it's not going to be so temporary if we push this out as version 7 we'll need to support o
as a type key for string forever as we'll always need to be able to load version 7. But it's also not the end of the world from my perspective, we're just using letters because it's easier to reason about compared to 0x6F
. So the fact that we're already using s
in this context it doesn't seem to be such a big deal we have to use a different character for string here. I think we already have a conflict like this for something on the circuit side.
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.
Fair enough. I updated the comments.
Co-authored-by: Matthew Treinish <[email protected]>
common.write_mapping( | ||
file_obj=file_obj, | ||
mapping=flat_key_refdict, | ||
serializer=_dumps_reference_item, | ||
metadata_serializer=metadata_serializer, | ||
) |
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.
common.write_mapping( | |
file_obj=file_obj, | |
mapping=flat_key_refdict, | |
serializer=_dumps_reference_item, | |
metadata_serializer=metadata_serializer, | |
) | |
common.write_mapping( | |
file_obj=file_obj, | |
mapping=flat_key_refdict, | |
serializer=_dumps_reference_item, | |
metadata_serializer=metadata_serializer, | |
) |
We have QPY function to write mapping object here.
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.
This LGTM now, thanks for making the updates. The cleanups make it a bit more clear to me how the new format works. I was getting a bit confused by the new header type before that I didn't see used anywhere.
I left an inline comment about having an upgrade release note, but we can do that as part of the release notes roundup.
pulse.reference("cr45p", "q0", "q1") | ||
|
||
with open('template_ecr.qpy', 'wb') as fd: | ||
qpy.dump(schedule, fd) |
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.
A note to myself (or anyone who is doing release notes) we should have an upgrade release note here about bumping to version 7.
with pulse.build() as schedule: | ||
pulse.reference("cr45p", "q0", "q1") | ||
pulse.reference("x", "q0") | ||
pulse.reference("cr45p", "q0", "q1") |
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.
Another note. @mtreinish could you also replace this with
pulse.reference("cr45m", "q0", "q1")
cr45"p" -> cr45"m" when you cleanup the release note? This is just a typo but not critical. Usually ECR pulse sequence has negative rotation (subscripted with "m") on the second pulse.
* Add support pulse reference to QPY * Review comments Co-authored-by: Matthew Treinish <[email protected]> --------- Co-authored-by: Matthew Treinish <[email protected]>
* Add support pulse reference to QPY * Review comments Co-authored-by: Matthew Treinish <[email protected]> --------- Co-authored-by: Matthew Treinish <[email protected]>
### 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 - Update JSON Encoder and Decoder to support ScheduleBlock and datetime object - Add save utils that provides a standard data model for calibration. Note that this offers portability of calibration data across different versions, platforms, etc.. - Deprecate conventional csv format that only supports calibration table and bit complicated. ### Sample code for testing Checkout my Terra folk https://github.com/nkanazawa1989/qiskit-terra/tree/feature/qpy-schedule-reference ```python from qiskit_experiments.calibration_management import Calibrations from qiskit import pulse from qiskit.circuit import Parameter cals = Calibrations( coupling_map=[[0, 1]], control_channel_map={(0, 1): [pulse.ControlChannel(0)], (1, 0): [pulse.ControlChannel(1)]}, ) with pulse.build(name="my_gate") as sched: pulse.play( pulse.Constant(Parameter("duration"), Parameter("amp")), pulse.ControlChannel(Parameter("ch0.1")), ) cals.add_schedule(sched, qubits=(0, 1)) cals.add_parameter_value(100, "duration", qubits=(0, 1), schedule="my_gate") cals.add_parameter_value(0.123, "amp", qubits=(0, 1), schedule="my_gate") cals.save(file_prefix="data", overwrite=True) roundtrip = Calibrations.load("./data.json") sched = cals.get_schedule("my_gate", (0, 1)) roundtrip_sched = roundtrip.get_schedule("my_gate", (0, 1)) assert sched == roundtrip_sched ``` --------- Co-authored-by: Daniel Egger <[email protected]> Co-authored-by: Daniel J. Egger <[email protected]>
Summary
This PR adds support for
Reference
pulse instruction to QPY. In addition, ScheduleBlock with assigned subroutine after round-trip keeps original reference data structure.Original reference structure:
Conventional behavior:
With this PR:
This doesn't impact program execution (because reference doesn't appear in the wire format), but it may help saving a template program for calibration task to preserve the original context.
Close #9889
Details and comments
This PR avoids key conflict issue described in #9889 in bit fragile way. This introduces new reference key for string
o
that appears in the context of pulse instruction operands, rather than delegating serialization to the standard value encoding in QPY. Since current type key doesn't have namespace, it could cause further key conflicts in future as we add more data types. However, adding namespace requires major refactoring of the current QPY module, and I wanted avoid this effort because of the limited bandwidths.