Fix start time updates in ConstrainedReschedule pass

[alexanderivrii]

Summary

In #10051 the ConstraintReschedule transpiler pass was updated to remove recursion, however it changed (in a very subtle way_ how node start times were updated. This PR fixes makes sure that node start times get updated in the exactly same way as before.

Details and comments

See #10051 for details.

[qiskit-bot]

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

[mtreinish]

LGTM, thanks for catching and fixing this for the comments discussing the need for this fix see: #10051 (comment)

[coveralls]

Pull Request Test Coverage Report for Build 4881895055

• 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
• 17 unchanged lines in 6 files lost coverage.
• Overall coverage increased (+0.02%) to 85.888%

---

Files with Coverage Reduction	New Missed Lines	%
crates/qasm2/src/expr.rs	1	93.76%
qiskit/transpiler/passes/synthesis/unitary_synthesis.py	1	90.46%
crates/qasm2/src/lex.rs	2	90.63%
qiskit/extensions/quantum_initializer/squ.py	2	80.0%
crates/accelerate/src/sabre_swap/layer.rs	4	97.32%
crates/accelerate/src/vf2_layout.rs	7	88.14%

Totals
Change from base Build 4876894924:	0.02%
Covered Lines:	71228
Relevant Lines:	82931

---

💛 - Coveralls

[mtreinish]

I've pushed this back to 0.24.1, I think we still need to do some more testing here and I don't want to block the release on this until we're sure this is correct. Although for 0.24.1 we might just want to rewrite the internals of the pass to be more efficient (which is risky from a backport potential the runtime performance of the pass makes it very difficult to use in practice).

[mtreinish]

So for more context on what I'm referring to in my above comment. My primary concern is running the below script with this PR applied it hasn't finished running ConstrainedReschedule after ~2hrs so far. So I'm worried about merging this. Intuitively I agree with the assessment in #10051 (comment) and feel this change is correct, but I'm very concerned that it's adding so much runtime to the pass (the same script returns in about 20min without this PR)

import time

from qiskit.transpiler.passes import ALAPScheduleAnalysis, TimeUnitConversion, InstructionDurationCheck, ConstrainedReschedule, PadDelay
from qiskit.transpiler.target import Target, InstructionProperties
from qiskit import transpile
from qiskit.transpiler import PassManager
from qiskit.circuit.random import random_circuit
from qiskit.circuit.library.standard_gates import *
from qiskit.circuit import Parameter, Measure, Delay
import rustworkx as rx
import numpy as np

rng = np.random.default_rng(seed=12345678942)

g = rx.generators.directed_heavy_hex_graph(13)
target = Target(
    num_qubits=len(g),
    min_length=16,
    pulse_alignment=8,
    acquire_alignment=16,
    granularity=8
)
rz_props = {}
x_props = {}
sx_props = {}
measure_props = {}
delay_props = {}
id_props = {}
for i in range(len(g)):
    qarg = (i,)
    rz_props[qarg] = InstructionProperties(error=0.0, duration=0.0)
    x_props[qarg] = InstructionProperties(
        error=rng.uniform(1e-6, 1e-5), duration=rng.uniform(1e-8, 9e-7)
    )
    sx_props[qarg] = InstructionProperties(
        error=rng.uniform(1e-6, 1e-5), duration=rng.uniform(1e-8, 9e-7)
    )
    measure_props[qarg] = InstructionProperties(
        error=rng.uniform(1e-6, 1e-5), duration=rng.uniform(1e-8, 9e-7)
    )
    delay_props[qarg] = None
    id_props[qarg] = None

target.add_instruction(XGate(), x_props)
target.add_instruction(SXGate(), sx_props)
target.add_instruction(RZGate(Parameter("theta")), rz_props)
target.add_instruction(Measure(), measure_props)
target.add_instruction(Delay(Parameter("t")), delay_props)
target.add_instruction(IGate(), id_props, name="id")
target.add_instruction(
    ECRGate(),
    {
        edge: InstructionProperties(
            error=rng.uniform(1e-6, 5e-5), duration=rng.uniform(1e-8, 9e-7)
        )
        for edge in g.edge_list()
    }
)

durations = target.durations()

schedule = PassManager([
    TimeUnitConversion(durations),
    ALAPScheduleAnalysis(durations),
    InstructionDurationCheck(target.acquire_alignment, target.pulse_alignment),
    ConstrainedReschedule(target.acquire_alignment, target.pulse_alignment),
    PadDelay(),
])

print("circuit")
qc = random_circuit(100, 20, measure=True, seed=202342)
print("transpile")
tqc = transpile(qc, target=target)
print("schedule")
import logging

logging.basicConfig(level="INFO")

start = time.perf_counter()
schedule.run(tqc)
stop = time.perf_counter()
print(stop - start)

[kdk]

Is there somewhere in test_instruction_alignments.py (or elsewhere) that we should add a test for this? Would it be worthwhile to check the output circuit's .op_start_times?

[mtreinish]

I think with #10077 we have a path to testing this. It was prohibitively slow without that. But I think having a test like this maybe with a few different alignment values that validates the output scheduled circuit is properly aligned would have a lot of value moving forward.

That being said I think this PR has been superseded by #10077 so maybe we should open a separate one to expand the testing.

[mtreinish]

Confirmed with @alexanderivrii that this isn't needed anymore and I'm going to close this. I still think it might be worth adding additional test coverage like what I used to test the performance here. But we can do that in a different PR.