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

Add align measure pass #6673

Merged
merged 28 commits into from
Jul 2, 2021
Merged

Conversation

nkanazawa1989
Copy link
Contributor

@nkanazawa1989 nkanazawa1989 commented Jul 1, 2021

Summary

This pass fixes the strange behavior of IBM Quantum backends when a circuit with length not multiple of 16 is provided. This behavior has been reported multiple times so far, especially some experiments requiring arbitrary delay between instructions, such as inversion recovery or Ramsey experiment.

This malfunction is caused by the misalignment of acquire instruction from the clock boundary of 16dt, which causes a phase error in measurement channels, yielding the drift of blob position in the measurement IQ plane (you can check this behavior with level1 measurement).

Fix #6666

Details and comments

Instead of fixing the rotation error, this PR adds a pass that shift the start time of measurement instruction to match with the 16dt clocks. This number will be provided by backend as alignment. The pass is currently called by the transpiler, but we can call this also from the IBMQ backend object via run method (in this case legacy backend can still cause the error).

This needs Qiskit/qiskit-ibmq-provider#983

from qiskit import IBMQ, circuit, transpile

IBMQ.load_account()
backend = IBMQ.get_provider(hub='ibm-q', group='open', project='main').get_backend('ibmq_armonk')

qc = circuit.QuantumCircuit(1, 1)
qc.x(0)
qc.delay(110, 0, unit="dt")
qc.measure(0, 0)

qc.draw()  #

     ┌───┐┌────────────────┐┌─┐
q_0: ┤ X ├┤ Delay(110[dt]) ├┤M├
     └───┘└────────────────┘└╥┘
c: 1/════════════════════════╩═
                             0 

qct = transpile(qc, backend)

qct.draw()  #

     ┌───┐┌────────────────┐┌─┐
q_0: ┤ X ├┤ Delay(112[dt]) ├┤M├
     └───┘└────────────────┘└╥┘
c: 1/════════════════════════╩═
                             0 

TODO

  • decide from where we call this pass
  • add reno

nkanazawa1989 and others added 2 commits July 1, 2021 22:48
Co-authored-by: Toshinari Itoko <itoko@jp.ibm.com>
@nkanazawa1989 nkanazawa1989 requested a review from a team as a code owner July 1, 2021 14:04
@nkanazawa1989
Copy link
Contributor Author

This is the result of integration test

from qiskit import IBMQ, circuit, transpile
import matplotlib.pyplot as plt
import numpy as np

IBMQ.load_account()
backend = IBMQ.get_provider(hub='ibm-q', group='open', project='main').get_backend("ibmq_armonk")

p_delay = circuit.Parameter("Δt")
delays = np.linspace(0, 100, 20)

qc = circuit.QuantumCircuit(1, 1)
qc.x(0)
qc.delay(p_delay, 0, unit="us")
qc.measure(0, 0)

qc.draw()  #

     ┌───┐┌───────────────┐┌─┐
q_0: ┤ X ├┤ Delay(Δt[us]) ├┤M├
     └───┘└───────────────┘└╥┘
c: 1/═══════════════════════╩═
                            0 

exp_qcs = [qc.assign_parameters({p_delay: delay}, inplace=False) for delay in delays]
result = backend.run(transpile(exp_qcs, backend), shots=256).result()

plt.plot(delays, [count.get("1", 0)/256 for count in result.get_counts()])
plt.xlabel("delay, us")

image

Copy link
Member

@ajavadia ajavadia left a comment

Choose a reason for hiding this comment

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

partial review before i look at the actual pass algorithm

qiskit/compiler/transpiler.py Outdated Show resolved Hide resolved
qiskit/compiler/transpiler.py Outdated Show resolved Hide resolved
qiskit/compiler/transpiler.py Outdated Show resolved Hide resolved
qiskit/transpiler/passes/scheduling/align_measures.py Outdated Show resolved Hide resolved
test/python/transpiler/test_align_measure.py Outdated Show resolved Hide resolved
test/python/transpiler/test_align_measure.py Outdated Show resolved Hide resolved
Comment on lines 42 to 43
def test_t1_experiment_type(self):
"""Test T1 experiment type circuit."""
Copy link
Member

Choose a reason for hiding this comment

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

for the circuits in this file do you mind drawing the circuits before/after alignment, in the doctstring, so it's easier to know what's happening? See e.g. the tests in #6619

Copy link
Contributor Author

@nkanazawa1989 nkanazawa1989 Jul 1, 2021

Choose a reason for hiding this comment

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

This is awesome idea! done in 92163d4 (I also added description for how the test works)

qiskit/transpiler/passes/scheduling/align_measures.py Outdated Show resolved Hide resolved

if len(dag.op_nodes(Delay)) == 0 or len(dag.op_nodes(Measure)) == 0:
# delay is only instruction that induce misalignment
# other instructions cannot be arbitrarily stretched
Copy link
Member

Choose a reason for hiding this comment

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

is this true? what if I define a pulse gate with a duration of 100dt? I think this pass should look at the measurement start times, and not make assumptions about the rest of the instructions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This instruction cannot be accepted by the backend because this already violate chunk size constraint. I was thinking to add chunk size validation for pulse gate later but I can add this check in this PR too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just added custom gate validation example in this commit 72821a3
I'll revert this commit and can move to separate PR if you wish.

Comment on lines 59 to 66
# if circuit is not yet scheduled, schedule with ALAP method
if dag.duration is None:
scheduler = ALAPSchedule(durations=self.durations)
scheduler.property_set["time_unit"] = time_unit
# run ALAP schedule to find measurement times
for required_pass in scheduler.requires:
dag = required_pass.run(dag)
dag = scheduler.run(dag)
Copy link
Member

Choose a reason for hiding this comment

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

A pass shouldn't really call another pass. See discussion here: #6580 (comment)

I think there are two alternate ways here:
1- just make the pass raise an error if the circuit is not scheduled. This is what I did for DD. The passmanager will be built in a way that scheduling is always called before this pass.
2- If you really want to do this then leave a TODO. Because what you really want is to "declare that the circuit must be scheduled, otherwise instruct the pass manager to insert an ALAPSchedule right before this pass". There is a requires field right now in the passes, but they only declare that a certain pass must have been run before them, not that a certain property must have been set. I think @1ucian0 has been meaning to implement this.

Copy link
Contributor Author

@nkanazawa1989 nkanazawa1989 Jul 1, 2021

Choose a reason for hiding this comment

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

Thanks. I took the approach 1 with slight improvement of the logic. Because we want to reduce breaking change as much as possible, so new logic has validation of delay duration to trigger pass execution. If all delays are multiple of 16 this pass is not executed, thus calling transpile without scheduling option doesn't cause an error.

Copy link
Contributor

Choose a reason for hiding this comment

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

I know, in general, we should not call another pass internally. I just want to note that we are accepting to break some existing codes using delays without scheduling, e.g. T1/T2 experiments, if requiring the external call of ALAP pass. They can be fixed only by supplying scheduling_method to transpile though. I'm wondering if this cost is allowable. What do you think? @ajavadia

Copy link
Member

Choose a reason for hiding this comment

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

I think the existence of alignment should tell the transpiler to do scheduling, even if scheduling_method is not supplied. There is a similar issue here #6667

Copy link
Member

Choose a reason for hiding this comment

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

but they only declare that a certain pass must have been run before them, not that a certain property must have been set. I think @1ucian0 has been meaning to implement this.

it is not possible to do "require a property" right now. However, you can conditionally run a pass based on the existence of a property which, if I understand this correctly, is the case in this situation.

nkanazawa1989 and others added 8 commits July 2, 2021 02:05
- change argment ordering
- update docstring
- move pass adding to each level function
- add circuit drawings to test
- avoid calling other passes from pass
Co-authored-by: Ali Javadi-Abhari <ajavadia@users.noreply.github.com>
Co-authored-by: Ali Javadi-Abhari <ajavadia@users.noreply.github.com>
Co-authored-by: Ali Javadi-Abhari <ajavadia@users.noreply.github.com>
@nkanazawa1989
Copy link
Contributor Author

nkanazawa1989 commented Jul 1, 2021

Thanks for the quick review. I updated the code according to your comments. The major changes are

  • Update the logic of pass execution judgement. Just minimized the risk of unnecessarily executing the heavy pass.
  • Added example of pulse gate alignment validation 72821a3. This can be moved to another PR, but in this way we can guarantee only delays can cause misalignment. Actually this is necessary validation. We haven't had this before and we used to get backend error after long queuing time.

before i look at the actual pass algorithm

The logic is almost the copy of ASAPScheduling pass. This is slightly modified to avoid inserting consecutive delay, such as delay(100dt)-delay(12dt)-measure. To isolate the context of alignment from other passes, the ASAPSchedule pass is kept as-is.

@ajavadia ajavadia added this to the 0.18 milestone Jul 2, 2021
@ajavadia
Copy link
Member

ajavadia commented Jul 2, 2021

The logic is almost the copy of ASAPScheduling pass.

Instead of copying ASAPScheduling, isn't it easier to just iterate over all Measure nodes, take their first predecessor that is a Delay, and increase its duration by some amount? If no Delay exists in the predecessors of the Measurement, create one.

I think this will be much shorter in terms of lines of code.

nkanazawa1989 and others added 6 commits July 2, 2021 11:40
…26a73.yaml

Co-authored-by: Ali Javadi-Abhari <ajavadia@users.noreply.github.com>
Co-authored-by: Ali Javadi-Abhari <ajavadia@users.noreply.github.com>
Co-authored-by: Ali Javadi-Abhari <ajavadia@users.noreply.github.com>
Co-authored-by: Ali Javadi-Abhari <ajavadia@users.noreply.github.com>
Co-authored-by: Ali Javadi-Abhari <ajavadia@users.noreply.github.com>
@nkanazawa1989
Copy link
Contributor Author

nkanazawa1989 commented Jul 2, 2021

just iterate over all Measure nodes

Yes this is what I wanted to implement, however we cannot do this because DAGNode doesn't have information of t0 even after scheduled. Perhaps we need to allow a node to have t0 after this release.

I just removed redundant validations from the ASAPScheduling implementation. ie we are assuming already scheduled DAG input in this pass: 291a1a5 This reduces lines of code.

@itoko
Copy link
Contributor

itoko commented Jul 2, 2021

In addition, if we have a mid measurement, the insertion of delay in front of the measurement affects the start times of the following instructions globally, suggesting local changes on measurements would not work for circuits with mid measurements.

nkanazawa1989 and others added 2 commits July 2, 2021 12:53
Co-authored-by: Toshinari Itoko <itoko@jp.ibm.com>
Copy link
Member

@ajavadia ajavadia left a comment

Choose a reason for hiding this comment

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

ok this looks good to me now. thanks for the explanations and making the updates.

qiskit/compiler/transpiler.py Outdated Show resolved Hide resolved
Co-authored-by: Ali Javadi-Abhari <ajavadia@users.noreply.github.com>
ajavadia
ajavadia previously approved these changes Jul 2, 2021
@mtreinish mtreinish added the Changelog: New Feature Include in the "Added" section of the changelog label Jul 2, 2021
@mergify mergify bot merged commit 3e1ca64 into Qiskit:main Jul 2, 2021
@nkanazawa1989 nkanazawa1989 deleted the fix/issue-6666_tempfix branch November 25, 2022 02:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: New Feature Include in the "Added" section of the changelog priority: high
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Delay instruction is perhaps not working
5 participants