-
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
Bug fix macros.measure with backendv2 #9987
Bug fix macros.measure with backendv2 #9987
Conversation
macros.py target delete raise statement in measure_v2 modify instructions.Acquire to Acquire
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/pulse/macros.py
Outdated
"Depricating ``backendV1`` as the type of measure's `backend` argument." | ||
), | ||
additional_msg=("Instead use ``backendV2``as the type of measure's `backend` argument."), | ||
since="0.25.0", |
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.
since="0.25.0", | |
since="0.24.0", |
0.25 or 0.24?
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.
TBH, I would just drop the since
field as this is a pending deprecation it's not on any timer and by default isn't user facing so people knowing that we're planning to deprecate this in the future doesn't really have a start date as there is no timer associated with it yet.
That being said I don't think we need this at all. BackendV1
is still a support interface in Qiskit we haven't marked it as deprecated or anything yet so I would just remove the use of the deprecation decorator here since I'd expect the pulse builder to support both v1 and v2.
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.
That's 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.
I deleted the deprecate decorator because "since" field must be provided.
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.
@Eric-Arellano here is another case we need to consider :) just fyi
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, the Sphinx deprecated
directive expects us to always have a since
, so that is why it's required.
The docstring for since
says this:
since: The version the deprecation started at. If the deprecation is pending, set
the version to when that started; but later, when switching from pending to
deprecated, update `since` to the new version.
I deleted the deprecate decorator because "since" field must be provided.
I encourage you to still use it and set pending=True
. That has some benefits:
- The message will be standardized
- The pending deprecation will show up in our docs. (It will say its pending, not actually deprecated)
- It's easy to switch from
pending
to deprecated. You deletepending=True
and bumpsince
to the version it became deprecated.
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 is awesome @to24toro ! We can support circuit scheduling with V2 backend with this PR! Proposed implementation looks good, but some minor edits are requested to approve. @mtreinish Can we still add this PR to 0.24 as a bugfix related to BackendV2?
qiskit/providers/backend_compat.py
Outdated
@@ -56,7 +56,10 @@ def convert_to_target( | |||
# Parse from properties if it exsits | |||
if properties is not None: | |||
qubit_properties = qubit_props_list_from_props(properties=properties) | |||
target = Target(num_qubits=configuration.n_qubits, qubit_properties=qubit_properties) | |||
target = Target( |
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.
Please revert this change.
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.
I fixed it at 3926200.
qiskit/pulse/macros.py
Outdated
If the backend has an attribute ``target``, the function uses the measurement logic, | ||
"_measure_v2" that takes ``target`` of the ``backend``, ``meas_map`` and ``qubit_mem_slots`` | ||
assignment. | ||
Otherwise, if the backend is None or ``backendV1``, the function uses the | ||
measurement logic, "_measure_v1" including ``instruction_schedule_map`` and ``meas_map`` | ||
as inputs. |
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.
If the backend has an attribute ``target``, the function uses the measurement logic, | |
"_measure_v2" that takes ``target`` of the ``backend``, ``meas_map`` and ``qubit_mem_slots`` | |
assignment. | |
Otherwise, if the backend is None or ``backendV1``, the function uses the | |
measurement logic, "_measure_v1" including ``instruction_schedule_map`` and ``meas_map`` | |
as inputs. | |
.. note:: | |
This function internally dispatches schedule generation logic depending on input backend model. | |
For the :class:`.BackendV1`, it considers conventional :class:`.InstructionScheduleMap` | |
and utilizes the backend calibration defined for a group of qubits in the `meas_map`. | |
For the :class:`.BackendV2`, it assembles calibrations of single qubit measurement | |
defined in the backend target to build a composite measurement schedule for `qubits`. |
This doesn't need to be written here. Note that this is API document that end-users read, so implementation details must be avoided.
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.
I fixed at 4029849.
qubit_mem_slots: Dict[int, int], | ||
measure_name: str = "measure", | ||
) -> Schedule: | ||
"""Return a schedule which measures the requested qubits according to the given |
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.
💯
qiskit/pulse/macros.py
Outdated
backend (Union[Backend, BaseBackend]): A backend instance, which contains | ||
hardware-specific data required for scheduling. |
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.
backend (Union[Backend, BaseBackend]): A backend instance, which contains | |
hardware-specific data required for scheduling. |
This doesn't exist.
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.
I fixed at 4029849.
qiskit/pulse/macros.py
Outdated
channels.AcquireChannel(measure_qubit), | ||
] | ||
) | ||
except exceptions.PulseError as ex: |
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.
except exceptions.PulseError as ex: | |
except KeyError as ex: |
Target doesn't raise PulseError
https://github.com/Qiskit/qiskit-terra/blob/5128c6751fc2909131ab38c72358bb1e91c9fd84/qiskit/transpiler/target.py#L904-L906
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.
I fixed at 4029849.
channels.AcquireChannel(qubit_index), | ||
mem_slot=channels.MemorySlot(reg_index), | ||
), | ||
inplace=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.
💯
qiskit/transpiler/target.py
Outdated
@@ -280,6 +280,7 @@ def __init__( | |||
matches the qubit number the properties are defined for. If some | |||
qubits don't have properties available you can set that entry to | |||
``None`` | |||
meas_map (list): List of sets of qubits that must be measured together. |
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.
Please revert this file
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.
I fixed it at 3926200.
qiskit/pulse/macros.py
Outdated
qubit_mem_slots: Mapping of measured qubit index to classical bit index. | ||
|
||
Returns: | ||
A schedule remapped by qubit_mem_slots as the input provided. |
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 schedule remapped by qubit_mem_slots as the input provided. | |
A measurement schedule with new memory slot index. |
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.
I fixed at 4029849.
test/python/pulse/test_macros.py
Outdated
@@ -91,6 +92,91 @@ def test_fail_measure(self): | |||
with self.assertRaises(PulseError): | |||
macros.measure(qubits=[0], inst_map=self.inst_map) | |||
|
|||
def test_measure_v2(self): | |||
"""Test macro - measure with backendV2.""" | |||
sched = macros.measure(qubits=[0], backend=self.backend_v2).filter( |
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.
Can you add another test with multiple qubits? Also it's better not to filter output (i.e. testing full schedule equality), because some future PR may break this function by injecting invalid instruction. This should be captured by this test.
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.
I added a multiple test at 381a8e6.
test/python/pulse/test_macros.py
Outdated
( | ||
0, | ||
Play( | ||
GaussianSquare( |
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.
You cannot hard-code these values because backend calibration snapshot may be updated in future.
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.
I fixed the test at 381a8e6.
qiskit/pulse/macros.py
Outdated
@@ -104,3 +232,33 @@ def measure_all(backend) -> Schedule: | |||
A schedule corresponding to the inputs provided. | |||
""" | |||
return measure(qubits=list(range(backend.configuration().n_qubits)), backend=backend) | |||
|
|||
|
|||
def schedule_remapping_memory_slot(schedule: Schedule, qubit_mem_slots: Dict[int, int]) -> Schedule: |
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.
@TsafrirA are you happy with this API? This is public so we cannot easily change after release. If you have any concern we can turn this into 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.
I would make this private just at this point in the release cycle if you want to include this for 0.24.0. It's best not to commit to something like this when there is a pending deadline like this. If we make it _schedule_remapping_memory_slot
this is gives a bit more time because there is no new public api addition so we can include this post RC1. Then we can always promote it to a public API in 0.25.0
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.
Make perfect sense :)
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.
I changed at b075cb3.
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.
As an isolated bugfix this is fine to include in 0.24.0. Without any API additions it's something we can also merge for 0.24.0 post rc1. I left a couple of small inline comments. Besides that I think a bugfix release release note would be good to document that the schedule is fixed when being passed a backend v2 object.
qiskit/pulse/macros.py
Outdated
"Depricating ``backendV1`` as the type of measure's `backend` argument." | ||
), | ||
additional_msg=("Instead use ``backendV2``as the type of measure's `backend` argument."), | ||
since="0.25.0", |
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.
TBH, I would just drop the since
field as this is a pending deprecation it's not on any timer and by default isn't user facing so people knowing that we're planning to deprecate this in the future doesn't really have a start date as there is no timer associated with it yet.
That being said I don't think we need this at all. BackendV1
is still a support interface in Qiskit we haven't marked it as deprecated or anything yet so I would just remove the use of the deprecation decorator here since I'd expect the pulse builder to support both v1 and v2.
qiskit/pulse/macros.py
Outdated
@@ -104,3 +232,33 @@ def measure_all(backend) -> Schedule: | |||
A schedule corresponding to the inputs provided. | |||
""" | |||
return measure(qubits=list(range(backend.configuration().n_qubits)), backend=backend) | |||
|
|||
|
|||
def schedule_remapping_memory_slot(schedule: Schedule, qubit_mem_slots: Dict[int, int]) -> Schedule: |
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.
I would make this private just at this point in the release cycle if you want to include this for 0.24.0. It's best not to commit to something like this when there is a pending deadline like this. If we make it _schedule_remapping_memory_slot
this is gives a bit more time because there is no new public api addition so we can include this post RC1. Then we can always promote it to a public API in 0.25.0
qiskit/pulse/macros.py
Outdated
for t0, inst in schedule.instructions: | ||
if isinstance(inst, instructions.Acquire): | ||
qubit_index = inst.channel.index | ||
reg_index = qubit_mem_slots.get(qubit_index, 0) |
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.
The unittest you wrote made me think this should be
reg_index = qubit_mem_slots.get(qubit_index, 0) | |
reg_index = qubit_mem_slots.get(qubit_index, qubit_index) |
because all acquisition instruction might store results in the slot0 when you specify qubit_mem_slots={0:0}
(i.e. you may provide the slot only for qubit of interested). This causes data conflict on the slot and you may get meaningless outcome from experiment.
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 @to24toro this looks almost good. Just minor comments. I'm happy to approve once these are addressed.
qiskit/pulse/macros.py
Outdated
meas_group = sorted(list(meas_group)) | ||
|
||
meas_group_set = set(range(max(meas_group) + 1)) | ||
unassigned_qubit_indices = list(meas_group_set - qubit_mem_slots.keys()) |
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.
unassigned_qubit_indices = list(meas_group_set - qubit_mem_slots.keys()) | |
unassigned_qubit_indices = list(set(meas_group) - qubit_mem_slots.keys()) |
I think this should work because qubit outside this group doesn't appear.
qiskit/pulse/macros.py
Outdated
unassigned_reg_indices = sorted(list(meas_group_set - set(qubit_mem_slots.values()))) | ||
if set(qubit_mem_slots.values()).issubset(meas_group_set): | ||
for qubit in unassigned_qubit_indices: | ||
qubit_mem_slots[qubit] = unassigned_reg_indices.pop(0) |
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 is not blocking but usually pop(0) is slow. For better performance you need to reverse sort unassigned_reg_indices
and just do .pop()
here.
Could you please also write bugfix note? |
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
* create measuregrouping class * add meas_map.setter in MeasureGrouping class * macros.measure * get_qubit_groups * generate_schedule * target.add_measuregrouping in backend_compat * target.add_measuregrouping in backend_converter * reformat and add docs * on the way of working on generate_schedule_in_measure * split measure into measure_v1 and measure_v2 macros.py target delete raise statement in measure_v2 modify instructions.Acquire to Acquire * macros.py * test_measuregrouping * bug fix schedule with backendV2 for 0.25.0 * modify comments * fix name of schedule in test_macros * delete meas_map as a Target attribute * minor changes in macros.py * add test to test_macros.py * make schedule_remapping_memory_slot private * delete since field from deprecate_arg * delete deprecate depcorator * black macros.py * revert about target * modify implementation of qubit_mem_slots * change the definition of meas_group_set * black macros.py * fix meas_group_set * fix qubit_mem_slots * reno * modify unassigned_qubit_indices * remove list() from unassigned_qubit_indices and unassigned_reg_indices
I'm relatively inexperienced with Qiskit and I think I'm still running into this issue even after building from source. Could someone please help me or direct me to the appropriate place to ask? I'm running this code:
I'm getting this error message:
I followed the "Install from source" instructions at https://qiskit.org/documentation/getting_started.html, I'm using the updated
This problem was fixed, right? Anyone have ideas about why this still isn't working? Let me know if there's somewhere else I should ask. |
@fvoichick
We would appreciate it if you could wait a little longer. |
Summary
Currently, we cannot use macros.measure with backendV2, but it is clear that this is an emergency issue that needs to be resolved.
After the discussion, the plan is to update macros.measure significantly by adding a new class
MeasureGrouping
. However, this is a major API break issue and will not be ready for the recent 0.25.0 release.Therefore, as a temporary, I made changes as a version that do not break the API.
Details and comments
fix #9488
Add
meas_map
as a new attribute ofTarget
. There is another part of the code that creates an instance of target, however I have not changed it yet now.In the
measure
in macors.py, dispatchmeasure_v1
andmeasure_v2
depending on the class of the backend.FakeHanoiV2
is used as backendV2 in test.