Skip to content

Fix ConstrainedReschedule Rust port giving wrong node_start_time (#16186)#16210

Draft
bnayahu wants to merge 2 commits into
Qiskit:mainfrom
bnayahu:fix/constrained-reschedule-rust-port-16186
Draft

Fix ConstrainedReschedule Rust port giving wrong node_start_time (#16186)#16210
bnayahu wants to merge 2 commits into
Qiskit:mainfrom
bnayahu:fix/constrained-reschedule-rust-port-16186

Conversation

@bnayahu
Copy link
Copy Markdown

@bnayahu bnayahu commented May 19, 2026

Fixes: #16186.

Three bugs in the Rust port of ConstrainedReschedule (PR #14883):

  1. Delay duration ignored when target is provided: push_node_back computed new_t1q using target.get_duration("delay", …), which returns None0 because Delay is not in the target gate table. The Delay end-time was therefore this_t0 + 0 instead of this_t0 + delay_duration, causing a u64 underflow in the successor overlap calculation and silently setting the measure start time to a wrapped garbage value (160 dt instead of 272 dt). Fixed by always reading Delay duration from the instruction parameter first, falling back to the target only for non-Delay ops.

  2. pulse_align/acquire_align swapped in the push_node_back call: run_constrained_reschedule passed the two alignment arguments in the wrong order. As a result acquire_align inside push_node_back received the pulse alignment value (1) and Measure/Reset were never shifted to the correct acquire-alignment boundary. Fixed by correcting the argument order.

  3. u64 underflow in overlap arithmetic: new_t1q - next_t0q and t1c - t0c used plain subtraction; when the current node ends before the successor starts (no real overlap) this wraps to a large value and incorrectly shifts successors. Changed to saturating_sub.

Also adds is_measure_or_reset() helper to consolidate the three Measure/Reset checks and to handle Python-level Measure/Reset operations (stored as OperationRef::Instruction) that do not match the StandardInstruction::Measure/Reset pattern.

Adds regression tests in test_scheduling_padding_pass.py.

AI/LLM disclosure

  • I didn't use LLM tooling, or only used it privately.
  • I used the following tool to help write this PR description: Clause Code, with the draft Qiskit skills
  • I used the following tool to generate or modify code: Clause Code, with the draft Qiskit skills

@qiskit-bot qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label May 19, 2026
@CLAassistant
Copy link
Copy Markdown

CLAassistant commented May 19, 2026

CLA assistant check
All committers have signed the CLA.

@bnayahu bnayahu force-pushed the fix/constrained-reschedule-rust-port-16186 branch from 338a38a to 7b6af51 Compare May 19, 2026 08:16
bnayahu and others added 2 commits May 20, 2026 09:01
…kit#16186)

Three bugs in the Rust port of `ConstrainedReschedule` (PR Qiskit#14883):

1. **Delay duration ignored when `target` is provided**: `push_node_back`
   computed `new_t1q` using `target.get_duration("delay", …)`, which returns
   `None` → `0` because Delay is not in the target gate table.  The Delay
   end-time was therefore `this_t0 + 0` instead of `this_t0 + delay_duration`,
   causing a u64 underflow in the successor overlap calculation and silently
   setting the measure start time to a wrapped garbage value (160 dt instead
   of 272 dt).  Fixed by always reading Delay duration from the instruction
   parameter first, falling back to the target only for non-Delay ops.

2. **`pulse_align`/`acquire_align` swapped in the `push_node_back` call**:
   `run_constrained_reschedule` passed the two alignment arguments in the
   wrong order.  As a result `acquire_align` inside `push_node_back` received
   the pulse alignment value (1) and Measure/Reset were never shifted to the
   correct acquire-alignment boundary.  Fixed by correcting the argument order.

3. **u64 underflow in overlap arithmetic**: `new_t1q - next_t0q` and
   `t1c - t0c` used plain subtraction; when the current node ends before the
   successor starts (no real overlap) this wraps to a large value and
   incorrectly shifts successors.  Changed to `saturating_sub`.

Also adds `is_measure_or_reset()` helper to consolidate the three
Measure/Reset checks and to handle Python-level Measure/Reset operations
(stored as `OperationRef::Instruction`) that do not match the
`StandardInstruction::Measure/Reset` pattern.

Adds regression tests in `test_scheduling_padding_pass.py`.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@bnayahu bnayahu force-pushed the fix/constrained-reschedule-rust-port-16186 branch from 8ada766 to ca90988 Compare May 20, 2026 06:02
gkneighb added a commit to gkneighb/qiskit that referenced this pull request May 22, 2026
- Reno: adopt @jakelishman's shorter suggested wording.
- Test: trim the long docstring (git holds the rest).
- Test: replace the implicit "doesn't raise" assertion with a positive
  assertion that the alignment-only path returns the expected
  node_start_time for an already-aligned circuit.  The shifted-alignment
  behaviour is tracked separately in Qiskit#16186 / Qiskit#16210, so this PR's test
  exercises the no-shift case.
- Test: add the Claude Opus 4.7 attribution comment per the LLM policy
  discussion.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Community PR PRs from contributors that are not 'members' of the Qiskit repo

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

ConstrainedReschedule giving different values for node_start_time after porting to Rust

3 participants