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

[WIP] introduce GroupingMeasure class #9750

Closed

Conversation

to24toro
Copy link
Contributor

@to24toro to24toro commented Mar 8, 2023

Summary

This PR introduces GroupingMeasure class which is a sub class of Instruction class and enables to n-qubits syncronized measurement in backendV2.
In previous, the class for measure, Measure class limits the number of measured qubit is one.
I expect this PR is a first step for solving failure of schedule with backendV2 and being able to measure all qubits.

Details and comments

related to #9488.

@qiskit-bot
Copy link
Collaborator

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/terra-core

@coveralls
Copy link

Pull Request Test Coverage Report for Build 4362567056

  • 4 of 14 (28.57%) changed or added relevant lines in 1 file are covered.
  • 4 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.03%) to 85.325%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/circuit/measure.py 4 14 28.57%
Files with Coverage Reduction New Missed Lines %
qiskit/circuit/tools/pi_check.py 1 91.23%
qiskit/pulse/library/waveform.py 3 91.67%
Totals Coverage Status
Change from base Build 4359967227: -0.03%
Covered Lines: 67900
Relevant Lines: 79578

💛 - Coveralls

@jakelishman
Copy link
Member

Just to let you know: this PR is unlikely to see review immediately, and it may well be in the future that we need to go a slightly different direction, but we definitely are thinking about it.

We're discussing a generalisation of how measurements are handled in Terra, but there's a lot of knock-on effects we've got to consider to get this right. There are different stages within the Terra compilation model: abstract, possibly user-defined operations when building a QuantumCircuit; an intermediate representation used within the transpiler during hardware-mapping and optimisation; the concrete, more assembler-like form output by the transpiler.

It's only the last of these that scheduling and hardware need to be concerned about, but for us we need to make sure we've got the right data structures in place to handle all three. There are a lot of old assumptions about measurements built into the transpiler, and adding more (especially co-opting the opcode measure) is something we need to take a lot of care with. We're also interested in expanding the types of measurement that can be performed, and how Terra reasons about them (e.g. #9269).

@nkanazawa1989
Copy link
Contributor

nkanazawa1989 commented Mar 8, 2023

Thanks Jake. I agree in the long run we need to consider canonicalization of measurement instruction, but on the (pulse/circuit) scheduling side we need more immediate solution. IBM backend provides the meas_map configuration that is just ignored in current transpiler. This configuration defines a list of subset qubit that must be measured simultaneously, and ignorance of this constraint gives two side effects:

  • We cannot generate pulse schedule with V2 backend. In V2 backend schedules are stored in Target but measurement calibration representing the grouped measurement (defined in the meas_map) cannot be handled as a Qiskit Measure instruction because of its qubit number limitation. We should fix this immediately.
  • Dynamical decoupling results in poor mitigation effect. Because measurement instruction is implicitly re-aligned in the backend compiler (according to the meas_map constraint), an echo pulse train in front of the measurement instruction generates unbalanced delay. One can use backend-provided scheduler pass to consider the measurement grouping so this is not super high-priority.

The first bullet is indeed the breaking API change and I want to fix this in this release. Basically we just need to introduce multi qubit measurement as a Qiskit instruction and update V2 converter not to ignore the calibration for it.

(edit)

Alternatively, we can modify the rest of Qiskit stack to generate grouped measurement calibration on the fly. With this we don't need to introduce new instruction, but there is no guarantee that grouped measurement and collection of single qubit measurements are identical (usually yes though).

@jakelishman
Copy link
Member

I don't think it's a straightforward change to allow the current Measure class and the "measure" opcode in the output to become variadic in general. The meaning of the measure opcode is, for better or worse, not fully defined by the Measure class - the Qiskit compiler behaviour and output is not primarily class-based: it's opcode-based. You can see this in the basis-translator equivalence library, the look-up keys of Target, the historic Qobj format, etc. We sometimes like to pretend it's class-based, but it's not - there's hidden historic assumptions everywhere.

Off the top of my head, it'll probably be ok to add a new "grouped_measure" opcode that's variadic, but that may have implications for the Python-space deployements of our hardware, in that they might need to report that they accept that. If we do add that as a temporary solution, I don't think we should compel any of our transpiler passes to consider it in optimisations, since it will be intended as an output operation. Upgrading transpiler passes to support other types of measurement in optimisations is something that I think needs more careful design - evidenced by how many times it's been tried unsuccessfully in the past.

@ikkoham
Copy link
Contributor

ikkoham commented Mar 9, 2023

Since I'm not so familiar with BackendV2 issue, I can't review the PR. But I would like to suggest renaming the class name. Grouping seems like abelian grouping to me. How about ConcurrentMeasure?

@nkanazawa1989
Copy link
Contributor

Yes, we should give dedicated identifier for new instruction. Note that this instruction is a compiler directive to manage circuit scheduling. I am thinking to add something like AggregateMeasure pass that is inserted right before the scheduling passes. This helps scheduling passes with analyzing a mini circuit block that is measured simultaneously. We can add SplitMeasure pass to avoid injecting grouped_measure in the output code.

Since I'm not so familiar with BackendV2 issue, I can't review the PR. But I would like to suggest renaming the class name. Grouping seems like abelian grouping to me. How about ConcurrentMeasure?

Fair enough. ConcurrentMeasure sounds good to me.

@to24toro
Copy link
Contributor Author

Thank you for discussion.
After the discussion, I close this draft.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants