Skip to content
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

Merged
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
4bab6a6
create measuregrouping class
to24toro Apr 5, 2023
9d109d0
add meas_map.setter in MeasureGrouping class
to24toro Apr 5, 2023
a40211c
macros.measure
to24toro Apr 5, 2023
25d0719
get_qubit_groups
to24toro Apr 10, 2023
11f734b
generate_schedule
to24toro Apr 10, 2023
ca3f032
target.add_measuregrouping in backend_compat
to24toro Apr 10, 2023
2d2ff79
target.add_measuregrouping in backend_converter
to24toro Apr 10, 2023
0d4b715
reformat and add docs
to24toro Apr 10, 2023
d223f4f
Merge remote-tracking branch 'origin/main' into feature/add_meas_grou…
to24toro Apr 11, 2023
d29025f
on the way of working on generate_schedule_in_measure
to24toro Apr 11, 2023
68ec213
split measure into measure_v1 and measure_v2
to24toro Apr 13, 2023
d19464b
macros.py
to24toro Apr 18, 2023
5930369
test_measuregrouping
to24toro Apr 18, 2023
e53f226
bug fix schedule with backendV2 for 0.25.0
to24toro Apr 19, 2023
c1a51d5
modify comments
to24toro Apr 19, 2023
f31ae23
fix name of schedule in test_macros
to24toro Apr 19, 2023
3926200
delete meas_map as a Target attribute
to24toro Apr 19, 2023
4029849
minor changes in macros.py
to24toro Apr 19, 2023
381a8e6
add test to test_macros.py
to24toro Apr 19, 2023
b075cb3
make schedule_remapping_memory_slot private
to24toro Apr 19, 2023
df4c4c7
delete since field from deprecate_arg
to24toro Apr 19, 2023
d4ff655
delete deprecate depcorator
to24toro Apr 19, 2023
952f865
black macros.py
to24toro Apr 20, 2023
f487cfe
revert about target
to24toro Apr 20, 2023
9fe1944
modify implementation of qubit_mem_slots
to24toro Apr 20, 2023
f35ed36
change the definition of meas_group_set
to24toro Apr 20, 2023
25f2ac9
black macros.py
to24toro Apr 20, 2023
acb0c93
fix meas_group_set
to24toro Apr 20, 2023
9f973ea
fix qubit_mem_slots
to24toro Apr 20, 2023
c2bed42
reno
to24toro Apr 20, 2023
f5fb919
modify unassigned_qubit_indices
to24toro Apr 20, 2023
9986e7f
remove list() from unassigned_qubit_indices and unassigned_reg_indices
to24toro Apr 20, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion qiskit/providers/backend_compat.py
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert this change.

Copy link
Contributor Author

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.

num_qubits=configuration.n_qubits,
qubit_properties=qubit_properties,
)
# Parse instructions
gates: Dict[str, Any] = {}
for gate in properties.gates:
Expand Down
174 changes: 166 additions & 8 deletions qiskit/pulse/macros.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,29 @@
# that they have been altered from the originals.

"""Module for common pulse programming macros."""
from __future__ import annotations

from typing import Dict, List, Optional, Union
from typing import Dict, List, Optional, Union, TYPE_CHECKING

from qiskit.pulse import channels, exceptions, instructions, utils
from qiskit.pulse.instruction_schedule_map import InstructionScheduleMap
from qiskit.pulse.schedule import Schedule
from qiskit.utils.deprecation import deprecate_arg


if TYPE_CHECKING:
from qiskit.transpiler import Target


@deprecate_arg(
"backend",
deprecation_description=(
"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",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
since="0.25.0",
since="0.24.0",

0.25 or 0.24?

Copy link
Member

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true

Copy link
Contributor Author

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.

Copy link
Contributor

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

Copy link
Collaborator

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:

  1. The message will be standardized
  2. The pending deprecation will show up in our docs. (It will say its pending, not actually deprecated)
  3. It's easy to switch from pending to deprecated. You delete pending=True and bump since to the version it became deprecated.

pending=True,
)
def measure(
qubits: List[int],
backend=None,
Expand All @@ -30,6 +45,13 @@ def measure(
"""Return a schedule which measures the requested qubits according to the given
instruction mapping and measure map, or by using the defaults provided by the backend.

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.
Copy link
Contributor

@nkanazawa1989 nkanazawa1989 Apr 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed at 4029849.


By default, the measurement results for each qubit are trivially mapped to the qubit
index. This behavior is overridden by qubit_mem_slots. For instance, to measure
qubit 0 into MemorySlot(1), qubit_mem_slots can be provided as {0: 1}.
Expand All @@ -47,18 +69,67 @@ def measure(

Returns:
A measurement schedule corresponding to the inputs provided.
"""

# backend is V2.
if hasattr(backend, "target"):
try:
meas_map = backend.configuration().meas_map
except AttributeError:
# TODO add meas_map to Target in 0.25
meas_map = [list(range(backend.num_qubits))]

return _measure_v2(
qubits=qubits,
target=backend.target,
meas_map=meas_map,
qubit_mem_slots=qubit_mem_slots or dict(zip(qubits, range(len(qubits)))),
measure_name=measure_name,
)
# backend is V1 or backend is None.
else:
try:
return _measure_v1(
qubits=qubits,
inst_map=inst_map or backend.defaults().instruction_schedule_map,
meas_map=meas_map or backend.configuration().meas_map,
qubit_mem_slots=qubit_mem_slots,
measure_name=measure_name,
)
except AttributeError as ex:
raise exceptions.PulseError(
"inst_map or meas_map, and backend cannot be None simultaneously"
) from ex


def _measure_v1(
qubits: List[int],
inst_map: InstructionScheduleMap,
meas_map: Union[List[List[int]], Dict[int, List[int]]],
qubit_mem_slots: Optional[Dict[int, int]] = None,
measure_name: str = "measure",
) -> Schedule:
"""Return a schedule which measures the requested qubits according to the given
instruction mapping and measure map, or by using the defaults provided by the backendV1.

Args:
qubits: List of qubits to be measured.
backend (Union[Backend, BaseBackend]): A backend instance, which contains
hardware-specific data required for scheduling.
inst_map: Mapping of circuit operations to pulse schedules. If None, defaults to the
``instruction_schedule_map`` of ``backend``.
meas_map: List of sets of qubits that must be measured together. If None, defaults to
the ``meas_map`` of ``backend``.
qubit_mem_slots: Mapping of measured qubit index to classical bit index.
measure_name: Name of the measurement schedule.
Returns:
A measurement schedule corresponding to the inputs provided.
Raises:
PulseError: If both ``inst_map`` or ``meas_map``, and ``backend`` is None.
"""

schedule = Schedule(name=f"Default measurement schedule for qubits {qubits}")
try:
inst_map = inst_map or backend.defaults().instruction_schedule_map
meas_map = meas_map or backend.configuration().meas_map
except AttributeError as ex:
raise exceptions.PulseError(
"inst_map or meas_map, and backend cannot be None simultaneously"
) from ex

if isinstance(meas_map, list):
meas_map = utils.format_meas_map(meas_map)

Expand Down Expand Up @@ -92,6 +163,63 @@ def measure(
return schedule


def _measure_v2(
qubits: List[int],
target: Target,
meas_map: Union[List[List[int]], Dict[int, List[int]]],
qubit_mem_slots: Dict[int, int],
measure_name: str = "measure",
) -> Schedule:
"""Return a schedule which measures the requested qubits according to the given
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

target and measure map, or by using the defaults provided by the backendV2.

Args:
qubits: List of qubits to be measured.
backend (Union[Backend, BaseBackend]): A backend instance, which contains
hardware-specific data required for scheduling.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
backend (Union[Backend, BaseBackend]): A backend instance, which contains
hardware-specific data required for scheduling.

This doesn't exist.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed at 4029849.

target: The :class:`~.Target` representing the target backend.
meas_map: List of sets of qubits that must be measured together.
qubit_mem_slots: Mapping of measured qubit index to classical bit index.
measure_name: Name of the measurement schedule.

Returns:
A measurement schedule corresponding to the inputs provided.
"""
schedule = Schedule(name=f"Default measurement schedule for qubits {qubits}")

if isinstance(meas_map, list):
meas_map = utils.format_meas_map(meas_map)
meas_group = set()
for qubit in qubits:
meas_group |= set(meas_map[qubit])
meas_group = sorted(list(meas_group))

for measure_qubit in meas_group:
try:
if measure_qubit in qubits:
default_sched = target.get_calibration(measure_name, (measure_qubit,)).filter(
channels=[
channels.MeasureChannel(measure_qubit),
channels.AcquireChannel(measure_qubit),
]
)
else:
default_sched = target.get_calibration(measure_name, (measure_qubit,)).filter(
channels=[
channels.AcquireChannel(measure_qubit),
]
)
except exceptions.PulseError as ex:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed at 4029849.

raise exceptions.PulseError(
"We could not find a default measurement schedule called '{}'. "
"Please provide another name using the 'measure_name' keyword "
"argument. For assistance, the instructions which are defined are: "
"{}".format(measure_name, target.instructions)
) from ex
schedule += schedule_remapping_memory_slot(default_sched, qubit_mem_slots)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

return schedule


def measure_all(backend) -> Schedule:
"""
Return a Schedule which measures all qubits of the given backend.
Expand All @@ -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:
Copy link
Contributor

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.

Copy link
Member

@mtreinish mtreinish Apr 19, 2023

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make perfect sense :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed at b075cb3.

"""
Return a Schedule which is remapped by given qubit_mem_slots.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Return a Schedule which is remapped by given qubit_mem_slots.
A helper function to overwrite MemorySlot index of :class:`.Acquire` instruction.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed at 4029849.


Args:
schedule: A measurement schedule.
qubit_mem_slots: Mapping of measured qubit index to classical bit index.

Returns:
A schedule remapped by qubit_mem_slots as the input provided.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
A schedule remapped by qubit_mem_slots as the input provided.
A measurement schedule with new memory slot index.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed at 4029849.

"""
new_schedule = Schedule()
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)
Copy link
Contributor

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

Suggested change
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.

new_schedule.insert(
t0,
instructions.Acquire(
inst.duration,
channels.AcquireChannel(qubit_index),
mem_slot=channels.MemorySlot(reg_index),
),
inplace=True,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

)
else:
new_schedule.insert(t0, inst, inplace=True)
return new_schedule
1 change: 1 addition & 0 deletions qiskit/transpiler/target.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert this file

Copy link
Contributor Author

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.

Raises:
ValueError: If both ``num_qubits`` and ``qubit_properties`` are both
defined and the value of ``num_qubits`` differs from the length of
Expand Down
90 changes: 88 additions & 2 deletions test/python/pulse/test_macros.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,11 @@
MemorySlot,
GaussianSquare,
Play,
Delay,
)
from qiskit.pulse import macros
from qiskit.pulse.exceptions import PulseError
from qiskit.providers.fake_provider import FakeOpenPulse2Q
from qiskit.providers.fake_provider import FakeOpenPulse2Q, FakeHanoiV2
from qiskit.test import QiskitTestCase


Expand All @@ -34,6 +35,7 @@ class TestMeasure(QiskitTestCase):
def setUp(self):
super().setUp()
self.backend = FakeOpenPulse2Q()
self.backend_v2 = FakeHanoiV2()
self.inst_map = self.backend.defaults().instruction_schedule_map

def test_measure(self):
Expand All @@ -43,7 +45,6 @@ def test_measure(self):
self.inst_map.get("measure", [0, 1]).filter(channels=[MeasureChannel(0)]),
Acquire(10, AcquireChannel(0), MemorySlot(0)),
)

self.assertEqual(sched.instructions, expected.instructions)

def test_measure_sched_with_qubit_mem_slots(self):
Expand Down Expand Up @@ -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(
Copy link
Contributor

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.

Copy link
Contributor Author

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.

channels=[MeasureChannel(0), AcquireChannel(0)]
)
expected = Schedule(
(
0,
Play(
GaussianSquare(
Copy link
Contributor

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.

Copy link
Contributor Author

@to24toro to24toro Apr 19, 2023

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.

duration=1792,
sigma=64,
width=1536,
amp=0.3940453,
angle=0.08222747293766576,
name="M_m0",
),
MeasureChannel(0),
name="M_m0",
),
),
(1792, Delay(1616, MeasureChannel(0))),
name="Default measurement schedule for qubits [0]",
)
expected += Acquire(1792, AcquireChannel(0), MemorySlot(0))
self.assertEqual(sched.instructions, expected.instructions)

def test_measure_v2_sched_with_qubit_mem_slots(self):
"""Test measure with custom qubit_mem_slots."""
sched = macros.measure(qubits=[0], backend=self.backend_v2, qubit_mem_slots={0: 2}).filter(
channels=[MeasureChannel(0), AcquireChannel(0)]
)
expected = Schedule(
(
0,
Play(
GaussianSquare(
duration=1792,
sigma=64,
width=1536,
amp=0.3940453,
angle=0.08222747293766576,
name="M_m0",
),
MeasureChannel(0),
name="M_m0",
),
),
(1792, Delay(1616, MeasureChannel(0))),
name="Default measurement schedule for qubits [0]",
)
expected += Acquire(1792, AcquireChannel(0), MemorySlot(2))
self.assertEqual(sched.instructions, expected.instructions)

def test_measure_v2_sched_with_meas_map(self):
"""Test measure with custom meas_map as list and dict."""
sched_with_meas_map_list = macros.measure(
qubits=[0], backend=self.backend_v2, meas_map=[[0, 1]]
).filter(channels=[MeasureChannel(0), AcquireChannel(0)])
sched_with_meas_map_dict = macros.measure(
qubits=[0], backend=self.backend_v2, meas_map={0: [0, 1], 1: [0, 1]}
).filter(channels=[MeasureChannel(0), AcquireChannel(0)])
expected = Schedule(
(
0,
Play(
GaussianSquare(
duration=1792,
sigma=64,
width=1536,
amp=0.3940453,
angle=0.08222747293766576,
name="M_m0",
),
MeasureChannel(0),
name="M_m0",
),
),
(1792, Delay(1616, MeasureChannel(0))),
name="Default measurement schedule for qubits [0]",
)
expected += Acquire(1792, AcquireChannel(0), MemorySlot(0))
self.assertEqual(sched_with_meas_map_list.instructions, expected.instructions)
self.assertEqual(sched_with_meas_map_dict.instructions, expected.instructions)


class TestMeasureAll(QiskitTestCase):
"""Pulse measure all macro."""
Expand Down