-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Revert deprecation and breaking changes of scheduling and alignment passes #7835
Merged
mergify
merged 13 commits into
Qiskit:main
from
mtreinish:fix-deprecation-align-measures
Mar 31, 2022
Merged
Changes from 8 commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
ccb077d
Revert deprecation and breaking changes of scheduling and alignment p…
mtreinish 4caa4d4
More release note fixes
mtreinish a7b28c2
Fix lint
mtreinish 6ff0949
Fix docs
mtreinish 5e8c383
Fix doc whitespace
mtreinish a39ba1e
Update dynamical decoupling release note
mtreinish 1b0f0ba
Copy docstrings from pre-refactor to fix docs build
mtreinish 85dde42
Add back test coverage for legacy scheduling and alignment passes
mtreinish 8c687f2
Update releasenotes/notes/update-instruction-alignment-passes-ef0f20d…
mtreinish bae9f47
Fix release notes
mtreinish f992ea4
Rename base scheduler class in legacy path
mtreinish ecfa245
Rename DynamicalDecouplingPadding to PadDynamicalDecoupling
mtreinish 40d1cdc
Merge branch 'main' into fix-deprecation-align-measures
mergify[bot] File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,155 @@ | ||
# This code is part of Qiskit. | ||
# | ||
# (C) Copyright IBM 2020. | ||
# | ||
# This code is licensed under the Apache License, Version 2.0. You may | ||
# obtain a copy of this license in the LICENSE.txt file in the root directory | ||
# of this source tree or at http://www.apache.org/licenses/LICENSE-2.0. | ||
# | ||
# Any modifications or derivative works of this code must retain this | ||
# copyright notice, and modified files need to carry a notice indicating | ||
# that they have been altered from the originals. | ||
|
||
"""ALAP Scheduling.""" | ||
|
||
import warnings | ||
|
||
from qiskit.circuit import Delay, Qubit, Measure | ||
from qiskit.dagcircuit import DAGCircuit | ||
from qiskit.transpiler.exceptions import TranspilerError | ||
|
||
from .base_scheduler import BaseScheduler | ||
|
||
|
||
class ALAPSchedule(BaseScheduler): | ||
"""ALAP Scheduling pass, which schedules the **stop** time of instructions as late as possible. | ||
|
||
See :class:`~qiskit.transpiler.passes.scheduling.base_scheduler.BaseScheduler` for the | ||
detailed behavior of the control flow operation, i.e. ``c_if``. | ||
""" | ||
|
||
def __init__(self, *args, **kwargs): | ||
super().__init__(*args, **kwargs) | ||
warnings.warn( | ||
"The ALAPSchedule class has been supersceded by the ALAPScheduleAnalysis class " | ||
"which performs the as analysis pass that requires a padding pass to later modify " | ||
"the circuit. This class will be deprecated in a future release and subsequently " | ||
"removed after that.", | ||
PendingDeprecationWarning, | ||
) | ||
|
||
def run(self, dag): | ||
"""Run the ALAPSchedule pass on `dag`. | ||
|
||
Args: | ||
dag (DAGCircuit): DAG to schedule. | ||
|
||
Returns: | ||
DAGCircuit: A scheduled DAG. | ||
|
||
Raises: | ||
TranspilerError: if the circuit is not mapped on physical qubits. | ||
TranspilerError: if conditional bit is added to non-supported instruction. | ||
""" | ||
if len(dag.qregs) != 1 or dag.qregs.get("q", None) is None: | ||
raise TranspilerError("ALAP schedule runs on physical circuits only") | ||
|
||
time_unit = self.property_set["time_unit"] | ||
new_dag = DAGCircuit() | ||
for qreg in dag.qregs.values(): | ||
new_dag.add_qreg(qreg) | ||
for creg in dag.cregs.values(): | ||
new_dag.add_creg(creg) | ||
|
||
idle_before = {q: 0 for q in dag.qubits + dag.clbits} | ||
bit_indices = {bit: index for index, bit in enumerate(dag.qubits)} | ||
for node in reversed(list(dag.topological_op_nodes())): | ||
op_duration = self._get_node_duration(node, bit_indices, dag) | ||
|
||
# compute t0, t1: instruction interval, note that | ||
# t0: start time of instruction | ||
# t1: end time of instruction | ||
|
||
# since this is alap scheduling, node is scheduled in reversed topological ordering | ||
# and nodes are packed from the very end of the circuit. | ||
# the physical meaning of t0 and t1 is flipped here. | ||
if isinstance(node.op, self.CONDITIONAL_SUPPORTED): | ||
t0q = max(idle_before[q] for q in node.qargs) | ||
if node.op.condition_bits: | ||
# conditional is bit tricky due to conditional_latency | ||
t0c = max(idle_before[c] for c in node.op.condition_bits) | ||
# Assume following case (t0c > t0q): | ||
# | ||
# |t0q | ||
# Q ░░░░░░░░░░░░░▒▒▒ | ||
# C ░░░░░░░░▒▒▒▒▒▒▒▒ | ||
# |t0c | ||
# | ||
# In this case, there is no actual clbit read before gate. | ||
# | ||
# |t0q' = t0c - conditional_latency | ||
# Q ░░░░░░░░▒▒▒░░▒▒▒ | ||
# C ░░░░░░▒▒▒▒▒▒▒▒▒▒ | ||
# |t1c' = t0c + conditional_latency | ||
# | ||
# rather than naively doing | ||
# | ||
# |t1q' = t0c + duration | ||
# Q ░░░░░▒▒▒░░░░░▒▒▒ | ||
# C ░░▒▒░░░░▒▒▒▒▒▒▒▒ | ||
# |t1c' = t0c + duration + conditional_latency | ||
# | ||
t0 = max(t0q, t0c - op_duration) | ||
t1 = t0 + op_duration | ||
for clbit in node.op.condition_bits: | ||
idle_before[clbit] = t1 + self.conditional_latency | ||
else: | ||
t0 = t0q | ||
t1 = t0 + op_duration | ||
else: | ||
if node.op.condition_bits: | ||
raise TranspilerError( | ||
f"Conditional instruction {node.op.name} is not supported in ALAP scheduler." | ||
) | ||
|
||
if isinstance(node.op, Measure): | ||
# clbit time is always right (alap) justified | ||
t0 = max(idle_before[bit] for bit in node.qargs + node.cargs) | ||
t1 = t0 + op_duration | ||
# | ||
# |t1 = t0 + duration | ||
# Q ░░░░░▒▒▒▒▒▒▒▒▒▒▒ | ||
# C ░░░░░░░░░▒▒▒▒▒▒▒ | ||
# |t0 + (duration - clbit_write_latency) | ||
# | ||
for clbit in node.cargs: | ||
idle_before[clbit] = t0 + (op_duration - self.clbit_write_latency) | ||
else: | ||
# It happens to be directives such as barrier | ||
t0 = max(idle_before[bit] for bit in node.qargs + node.cargs) | ||
t1 = t0 + op_duration | ||
|
||
for bit in node.qargs: | ||
delta = t0 - idle_before[bit] | ||
if delta > 0: | ||
new_dag.apply_operation_front(Delay(delta, time_unit), [bit], []) | ||
idle_before[bit] = t1 | ||
|
||
new_dag.apply_operation_front(node.op, node.qargs, node.cargs) | ||
|
||
circuit_duration = max(idle_before.values()) | ||
for bit, before in idle_before.items(): | ||
delta = circuit_duration - before | ||
if not (delta > 0 and isinstance(bit, Qubit)): | ||
continue | ||
new_dag.apply_operation_front(Delay(delta, time_unit), [bit], []) | ||
|
||
new_dag.name = dag.name | ||
new_dag.metadata = dag.metadata | ||
new_dag.calibrations = dag.calibrations | ||
|
||
# set circuit duration and unit to indicate it is scheduled | ||
new_dag.duration = circuit_duration | ||
new_dag.unit = time_unit | ||
|
||
return new_dag |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be careful for this because some people are trying to use upgraded pass for their workshop. This change could break upgraded code so we need kind announce in community slack channel about how to use DD pass with recent systems and how they can migrate (I assumed there is no subclass of them and keep API consistent so that this doesn't become breaking "API" change).
If I understand correctly,
DynamicalDecouplingPadding
pass will be deprecated again once it replacesDynamicalDecoupling
. However, this pass is not integrated into preset pass manager and user should integrateDynamicalDecouplingPadding
into their experimental code. This could bother experimentalists.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well we don't necessarily need to deprecate and rename
DynamicalDecouplingPadding
back toDynamicalDecoupling
. We can just deprecate and removeDynamicalDecoupling
and makeDynamicalDecouplingPadding
the new canonical name moving forward. I do think it's a bit uglier but it is a bit more descriptive too.TBH, I wasn't thinking too much about the longer term plan here more that we should just avoid a breaking change here because anyone that was using
DynamicalDecoupling
today successfully (which there are even with the new alignment constraints on IBM backends it works elsewhere) would be broken by changing the underlying implementation to be a padding passThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then the name should be
PadDynamicalDecoupling
or renamePadDelay
toDelayPadding
? I think both should be okey. Anyways I am not an active user of this pass, so I'd like to hear @ajavadia 's thought.No one can use the pass successfully today. Anyways I agree if someone has custom DD pass this change will break their code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like
PadDynamicalDecoupling
that fits the new naming and is clearer to me than what I used. I'll rename itThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in: ecfa245
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm almost good with this PR. Just holding my approval until we hear some user's voice about the class name and future deprecation plan.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I think
ASAPScheduleAnalysis
andALAPScheduleAnalysis
andPadDynamicalDecoupling
are fine, they are explicit about what they do.