Add a new argument concurrent_measurements to target class#10258
Conversation
|
One or more of the the following people are requested to review this:
|
TsafrirA
left a comment
There was a problem hiding this comment.
I have some minor suggestions for the doc strings which relate to the next point.
This goes beyond the scope of this PR, but I am not a fan of the fact that we use meas_map to describe two different representations (list vs. dict). At the very least, I think we should be clearly documenting this - What is accepted as input? What is the actual format used later on?
| 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, dict): List of sets of qubits that must be measured together. |
There was a problem hiding this comment.
The two representations of meas_map are accepted as input, but only one is documented here.
There was a problem hiding this comment.
So far, most part of the meas_map type is list and this is converted into Dict in inner codes. However, I think that 'meas_map' accepts Dict, e.g. macros.measure, in some cases because Dict of meas_map is easy to understand intuitively rather than List if there exist hardware constraints.
I treated the meas_map type as Dict and implemented macros.measure in backendV2. (It seems that format_measmap was implemented because it is easier to use for Dict than for List.)
So, if the input is a List, I would like to use format_measmap to convert the List to a Dict, and accept both List and Dict as input.
| instruction_durations: Optional instruction durations for instructions. If specified | ||
| it will take priority for setting the ``duration`` field in the | ||
| :class:`~InstructionProperties` objects for the instructions in the target. | ||
| meas_map: List of sets of qubits that must be measured together. |
There was a problem hiding this comment.
Again, only one of the two accepted formats is documented here.
There was a problem hiding this comment.
As you pointed out, the document is not easy for users or developers to understand.
I fixed at 407f87d .
| "length of the input qubit_properties list" | ||
| ) | ||
| self.qubit_properties = qubit_properties | ||
| self.meas_map = format_meas_map(meas_map) if isinstance(meas_map, list) else meas_map |
There was a problem hiding this comment.
Despite documenting the list option in the doc string, we actually convert to the dictionary option here.
There was a problem hiding this comment.
Do we need to convert the data format here? Probably this is bit confusing to users if input data is implicitly modified. Since formatted meas map is useful for implementing pulse scheduling, probably we can do this format in the scheduler. Then we can restrict the input to list format.
| pulse_alignment=pulse_alignment, | ||
| acquire_alignment=acquire_alignment, | ||
| qubit_properties=qubit_properties, | ||
| meas_map=format_meas_map(meas_map) if isinstance(meas_map, list) else meas_map, |
There was a problem hiding this comment.
And again, the inconsistency between the doc string and the actual format used later.
Pull Request Test Coverage Report for Build 5597675106
💛 - Coveralls |
nkanazawa1989
left a comment
There was a problem hiding this comment.
This looks good to me. Just to make sure
@mtreinish Are you fine with updating the Target model at Qiskit level? Alternatively we can do this in IBM provider, however pulse and circuit scheduler may rely on this constraint and exposing this in general Target makes sense to me (as long as Qiskit provides schedulers).
@wshanks We assume concurrent_measurements (aka meas_map) is just a static list of sub qubits measured together. Can future/non-IBM backend expose different constraints? If so probably we should make Target.concurrent_measurements a method so that we can do some formatting/operation in future.
@nkanazawa1989 Do you have anything specific in mind? The current status is that all IBM hardware backends currently put all qubits into one measurement group. I think this is mainly a software constraint left over from how the system was originally designed with a single acquisition trigger. Beyond that, I could imagine qubits being split into groups based on multiplexed readout -- requiring qubits on the same output line to be measured simultaneously (I could also imagine not having this constraint). Those are the most concrete constraints I can suggest. I don't know other qubit architectures that well though. |
I'm fine with adding it. I'll defer to both you and @wshanks on how common you think it'll be. If you guys think it's a common constraint that is not just a custom implementation detail for IBM hardware then I'm fine adding it as an optional field to the target. My thinking is for multiplexed readout it feels like this would be a common thing if other hardware does that and we want to support scheduling for that. But I'm also much less familiar with these hardware constraints. |
Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com>
Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com>
|
Thanks @wshanks , @mtreinish sounds like we can go with current assumption. What I had in mind was some edge case that cannot bootstrap sub-qubit lists, e.g. qubit group may change depending on inclusion of a particular qubit -- in this case I'll merge this PR as-is. Thanks for your input! |
…skit#10258) * fix measure_v2 * modify measure_all * dispatch backend * add test of the builder with backendV2 * reconfigure test codes and some func * refactoring * add reno * fix _measure_v2 * fix backend.meas_map in measure_v2 * fix reno * delete get_qubit_channels from utils * add get_qubits_channels in qubit_channels * recostruct test about the builder with backendV2 * add meas_map to Target class * fix after mergin main branch * fix documents about meas_map * format target.py * add reno * add meas_map to target in convert_to_target * add the more description about meas_map * Update releasenotes/notes/enable_target_aware_meas_map-0d8542402a74e9d8.yaml Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com> * fix test_meas_map * remove format_meas_map * rename meas_map in target to concurrent_measuments * change reno * remove Unused Union Dict * concurrent_measurements set as getattr(configuration, meas_map) * Update qiskit/transpiler/target.py Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com> * Update qiskit/transpiler/target.py Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com> * format --------- Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com>
…skit#10258) * fix measure_v2 * modify measure_all * dispatch backend * add test of the builder with backendV2 * reconfigure test codes and some func * refactoring * add reno * fix _measure_v2 * fix backend.meas_map in measure_v2 * fix reno * delete get_qubit_channels from utils * add get_qubits_channels in qubit_channels * recostruct test about the builder with backendV2 * add meas_map to Target class * fix after mergin main branch * fix documents about meas_map * format target.py * add reno * add meas_map to target in convert_to_target * add the more description about meas_map * Update releasenotes/notes/enable_target_aware_meas_map-0d8542402a74e9d8.yaml Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com> * fix test_meas_map * remove format_meas_map * rename meas_map in target to concurrent_measuments * change reno * remove Unused Union Dict * concurrent_measurements set as getattr(configuration, meas_map) * Update qiskit/transpiler/target.py Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com> * Update qiskit/transpiler/target.py Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com> * format --------- Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com>
) * fix measure_v2 * modify measure_all * dispatch backend * add test of the builder with backendV2 * reconfigure test codes and some func * refactoring * add reno * fix _measure_v2 * fix backend.meas_map in measure_v2 * fix reno * delete get_qubit_channels from utils * add get_qubits_channels in qubit_channels * recostruct test about the builder with backendV2 * add meas_map to Target class * fix after mergin main branch * fix documents about meas_map * format target.py * add reno * add meas_map to target in convert_to_target * add the more description about meas_map * Update releasenotes/notes/enable_target_aware_meas_map-0d8542402a74e9d8.yaml Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com> * fix test_meas_map * remove format_meas_map * rename meas_map in target to concurrent_measuments * change reno * remove Unused Union Dict * concurrent_measurements set as getattr(configuration, meas_map) * Update qiskit/transpiler/target.py Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com> * Update qiskit/transpiler/target.py Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com> * format --------- Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com>
…skit#10258) * fix measure_v2 * modify measure_all * dispatch backend * add test of the builder with backendV2 * reconfigure test codes and some func * refactoring * add reno * fix _measure_v2 * fix backend.meas_map in measure_v2 * fix reno * delete get_qubit_channels from utils * add get_qubits_channels in qubit_channels * recostruct test about the builder with backendV2 * add meas_map to Target class * fix after mergin main branch * fix documents about meas_map * format target.py * add reno * add meas_map to target in convert_to_target * add the more description about meas_map * Update releasenotes/notes/enable_target_aware_meas_map-0d8542402a74e9d8.yaml Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com> * fix test_meas_map * remove format_meas_map * rename meas_map in target to concurrent_measuments * change reno * remove Unused Union Dict * concurrent_measurements set as getattr(configuration, meas_map) * Update qiskit/transpiler/target.py Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com> * Update qiskit/transpiler/target.py Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com> * format --------- Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com>
Summary
Make the
Targetclass be aware ofmeas_map(concurrent_measurementsis an alias). I added a new attributeconcurrent_measurementsto thefrom_configurationfunction and we can prepare for theTargetclass that has the info of theScheduleConfig.Details and comments
Need to review after #10150.
related to #10200.