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

Pauli measurement #5311

Closed
wants to merge 65 commits into from
Closed

Pauli measurement #5311

wants to merge 65 commits into from

Conversation

yaelbh
Copy link
Contributor

@yaelbh yaelbh commented Oct 29, 2020

Reviving #4024.

Fixes #3967

The conversation of #4024 dealt with the addition of gates after measurement. When does the removal of such gates, after final measurements, occur? The output of transpile still contains the gates after the final measurement.

To do items:

  • Store the basis in the instruction parameters, instead of the instruction name.
  • Allow also measurement in the I basis (no measurement).
  • Verify that works well when measuring multiple qubits.

quantumjim and others added 30 commits March 25, 2020 11:48
add initial attempt at measurements
implement BasisTransformationMeasurement
its probably safer to wrap the Measure instruction if the basis is Z instead of the PauliMeasure to mimic the Measure instruction. Otherwise we might run into unforeseen issues since Measure might be used as primitive somewhere down in Terra
* use clifford gates only
* apply inverse post rotations for correct statevector simulations
* make basis argument required
* change to uppercase labels (but support lowercase still)
@yaelbh
Copy link
Contributor Author

yaelbh commented Oct 3, 2021

A few comments:

  1. I didn't add support to measure_pauli in qasm.
  2. I touched only a little the transpiler. I don't know which node is created in the DAG, in the presence of the new measure_pauli instruction, and what's its type (see for example _process_node and _process_measure in qiskit/converters/ast_to_dag.py). Checks whether something is a measurement appear all over, and I mostly didn't change them, I don't know if a change is required there, and if yes then how: in gate translations; in decisions related to optimization that are based on the number of measurements; and in passes: RemoveDiagonalGatesBeforeMeasure, AlignMeasures, BarrierBeforeFinalMeasurements, RemoveFinalMeasurements, OptimizeSwapBeforeMeasure.
  3. I touched only a little the visualization. I tried my best with matplotlib, but didn't change text and latex. Note that, unlike measure, measure_pauli can take multiple qubits.

@1ucian0 1ucian0 added this to the 0.20 milestone Nov 22, 2021
@coveralls
Copy link

Pull Request Test Coverage Report for Build 1489887938

  • 53 of 67 (79.1%) changed or added relevant lines in 8 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.006%) to 82.565%

Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/circuit/measure.py 30 32 93.75%
qiskit/visualization/text.py 12 14 85.71%
qiskit/visualization/matplotlib.py 2 12 16.67%
Totals Coverage Status
Change from base Build 1484137595: 0.006%
Covered Lines: 49969
Relevant Lines: 60521

💛 - Coveralls

@1ucian0 1ucian0 self-assigned this Nov 25, 2021
@1ucian0
Copy link
Member

1ucian0 commented Feb 15, 2022

Is there any reason for creating MeasurePauli instead of extending Measure with a basis parameter?

@yaelbh
Copy link
Contributor Author

yaelbh commented Feb 15, 2022

Is there any reason for creating MeasurePauli instead of extending Measure with a basis parameter?

@1ucian0 I'm not sure, but to get the answer it's important to emphasize:

  • MeasurePauli can have multiple qubits, whereas Measure takes only one qubit. Therefore doing something like this would also mean extension of Measure to any number of qubits.
  • Eventually, the goal of this PR is that for a simulator backend the transpiler will leave the instruction as-is, and will not convert it to a sequence of single-qubit measurements in the standard basis. We want to exploit the fact that multi-qubit Pauli measurement can be simulated more efficiently.

@nkanazawa1989
Copy link
Contributor

I found this PR is very helpful to cleanly write many calibration sequences in the experiments. I think now post rotations are okey since most backends support gate after measurement.

@yaelbh
Copy link
Contributor Author

yaelbh commented Feb 18, 2022

I'd like to draw the attention of the reviewers to my comments #5311 (comment).

@1ucian0
Copy link
Member

1ucian0 commented Feb 19, 2022

Is there any reason for creating MeasurePauli instead of extending Measure with a basis parameter?

Actually, this is a a bad idea. Each basis is, in essence, a different instructions with different decompositions and backends (such as IBM Quantum provider) and plugins (such as QASM2) might support a subset of them. Taking this suggestion back.

@@ -36,3 +39,62 @@ def broadcast_arguments(self, qargs, cargs):
yield qarg, [each_carg]
else:
raise CircuitError("register size error")


class MeasurePauli(Measure):
Copy link
Member

Choose a reason for hiding this comment

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

See https://github.com/Qiskit/qiskit-terra/pull/5311/files#r810538319

Suggested change
class MeasurePauli(Measure):
class MeasurePauli(Instruction):

Copy link
Contributor

Choose a reason for hiding this comment

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

Strong +1. MeasurePauli should not be a subclass of Measure.

Quantum measurement in the computational basis.
Quantum measurement
Copy link
Member

@1ucian0 1ucian0 Feb 19, 2022

Choose a reason for hiding this comment

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

I would leave this file as it is and move measure_pauli to its own file.

Comment on lines +762 to +767
if isinstance(op, (Measure, MeasurePauli)):
if isinstance(op, Measure):
basis = "Z"
else:
basis = op.params
self._measure(node, basis=basis)
Copy link
Member

@1ucian0 1ucian0 Feb 19, 2022

Choose a reason for hiding this comment

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

This is an example of a bug mentioned in https://github.com/Qiskit/qiskit-terra/pull/5311/files#r810538319 (it always get basis = "Z" and the basis = op.params branch is never taken).

A possible work around for this case might look something like this:

Suggested change
if isinstance(op, (Measure, MeasurePauli)):
if isinstance(op, Measure):
basis = "Z"
else:
basis = op.params
self._measure(node, basis=basis)
if isinstance(op, MeasurePauli):
self._measure(node, basis=''.join(op.params))
elif isinstance(op, Measure):
self._measure(node, basis='Z')

Comment on lines +1021 to +1026
if isinstance(op, (Measure, MeasurePauli)):
if isinstance(op, Measure):
gate = MeasureFrom()
else:
basis = op.params.upper()
gate = MeasureFrom(basis)
Copy link
Member

Choose a reason for hiding this comment

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

This seems like a very similar situation than here: https://github.com/Qiskit/qiskit-terra/pull/5311/files#r810540202

Copy link
Member

Choose a reason for hiding this comment

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

yeap.. same situation (this might need a test)

circuit = QuantumCircuit(6, 6)
circuit.measure_pauli("x", [0], [0])
circuit.barrier()
circuit.measure_pauli("y", [1], [1])
circuit.barrier()
circuit.measure_pauli("z", [2], [2])
circuit.barrier()
circuit.measure_pauli("xyz", [3, 4, 5], [3, 4, 5])
print(circuit)
     ┌─┐ ░     ░     ░    
q_0: ┤M├─░─────░─────░────
     └╥┘ ░ ┌─┐ ░     ░    
q_1: ─╫──░─┤M├─░─────░────
      ║  ░ └╥┘ ░ ┌─┐ ░    
q_2: ─╫──░──╫──░─┤M├─░────
      ║  ░  ║  ░ └╥┘ ░ ┌─┐
q_3: ─╫──░──╫──░──╫──░─┤M├
      ║  ░  ║  ░  ║  ░ └╥┘
q_4: ─╫──░──╫──░──╫──░──╫─
      ║  ░  ║  ░  ║  ░  ║ 
q_5: ─╫──░──╫──░──╫──░──╫─
      ║  ░  ║  ░  ║  ░  ║ 
c: 6/═╩═════╩═════╩═════╩═
      0     1     2     3 

So, same workaround?

Suggested change
if isinstance(op, (Measure, MeasurePauli)):
if isinstance(op, Measure):
gate = MeasureFrom()
else:
basis = op.params.upper()
gate = MeasureFrom(basis)
if isinstance(op, (Measure, MeasurePauli)):
if isinstance(op, MeasurePauli):
gate = MeasureFrom(''.join(op.params))
elif isinstance(op, Measure):
gate = MeasureFrom()

@1ucian0
Copy link
Member

1ucian0 commented Feb 19, 2022

Eventually, the goal of this PR is that for a simulator backend the transpiler will leave the instruction as-is, and will not convert it to a sequence of single-qubit measurements in the standard basis. We want to exploit the fact that multi-qubit Pauli measurement can be simulated more efficiently.

The multi-qubits feature makes this PR a bit tricky. My main red flag is that multi-qubits, in this context, implies multi-clbit
and that breaks some assumptions. For example, there is no support for drawing writing-into-multiple-clbits. This might sounds like a detail, but it might have deeper implications that are not so obvious.

Considering that and since it seems kinda trivial for a backend to detect a sequence of measures, is this feature worthy? If we are okey with removing it, then we just need to add the instructions MeasureX and MeasureY (MeasureZ could be some sort of alias of the current Measure) with their own decomposition in terms of Measure.

@yaelbh
Copy link
Contributor Author

yaelbh commented Feb 20, 2022

Considering that and since it seems kinda trivial for a backend to detect a sequence of measures, is this feature worthy? If we are okey with removing it, then we just need to add the instructions MeasureX and MeasureY

Sounds fine for me.
@nkanazawa1989 you've been involved
@chriseclectic this work started following a discussion with you
Do you (Chris & Naoki) agree?

@nkanazawa1989
Copy link
Contributor

Single qubit measure is okey with me -- as current measurement instruction does. User can write a macro for mutiqubit measure if they really want. The important point for me is pauli measure instruction will add more context to the circuit and they make the code and visualized circuit more readable.

@yaelbh
Copy link
Contributor Author

yaelbh commented Feb 22, 2022

As I said, I agree with the move to single-qubit. However I recommend to read a bit of discussion about it in #4024, which remained without conclusions.

What's everybody's opinion:

  • From user's perspective, a single instruction measure_pauli (or measure_pauli with parameters (as done in Add Pauli measurements #4024) or separate measure_x, measure_y, measure_z instructions (as suggested in Add x and y measurements #3967) ?
  • Same question from internal implementation perspective - a single class PauliMeasure (or MeasurePauli) with parameters or separate classes MeasureX, MeasureY (in any case they will all inherit directly from Instruction) ?

@itoko
Copy link
Contributor

itoko commented Feb 25, 2022

Sorry to cut in, what about an idea in the middle? That is introducing measure_pauli accepting multi-qubit pauli measurement while it internally produces single-qubit PauliMeasures for now to avoid the drawer issue for mult-clbits. The direct creation of multi-qubit pauli measurement, e.g. PauliMeasure(basis="XYZ"), should raise a "not yet allowed" error for now. In this way, once the drawer issue is fixed, we can introduce true multi-qubit pauli measurement in the future without a big change. Am I missing something?

@1ucian0
Copy link
Member

1ucian0 commented Mar 2, 2022

closing in favor of #7716

@1ucian0 1ucian0 closed this Mar 2, 2022
@itoko
Copy link
Contributor

itoko commented Mar 3, 2022

I thought some qiskit-application developers may want a general PauliMeasure interface (but I myself had no concrete use case), while there is a clear need for single-qubit Measures from qiskit-experiments. So it makes sense to introduce single-qubit Measures at this moment. Provided we introduce single-qubit Measures, I think we should not introduce measure_pauli now (it should be added only in the case we introduce PauliMeasure in future)

@joon-huh
Copy link

I thought some qiskit-application developers may want a general PauliMeasure interface (but I myself had no concrete use case), while there is a clear need for single-qubit Measures from qiskit-experiments. So it makes sense to introduce single-qubit Measures at this moment. Provided we introduce single-qubit Measures, I think we should not introduce measure_pauli now (it should be added only in the case we introduce PauliMeasure in future)

Multi-qubit Pauli measurements are essential for lattice surgery fault-tolerant QC (for example https://arxiv.org/pdf/1808.02892). While it is not relevant for current hardwares, Qiskit supporting multi-qubit Pauli measurements would be nice for those who are interested in making a compiler for FTQC. I hope this feature is integrated into Qiskit soon.
-A random person.

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.

Add x and y measurements
9 participants