Skip to content

fix: don't overwrite unrelated CAPTURE memory reference when expanding measure calibrations - #531

Open
0xhokugava wants to merge 2 commits into
rigetti:mainfrom
0xhokugava:fix/issue-499-capture-memory-reference
Open

0xhokugava wants to merge 2 commits into
rigetti:mainfrom
0xhokugava:fix/issue-499-capture-memory-reference

Conversation

@0xhokugava

Copy link
Copy Markdown

Body

Fixes #499

Problem

When expanding a DEFCAL MEASURE calibration, Calibrations::expand_inner unconditionally
rewrote the memory_reference of every CAPTURE instruction in the calibration body to
the target of the MEASURE being expanded even when that CAPTURE had nothing to do with
the calibration's declared target parameter.

The sibling PRAGMA LOAD-MEMORY branch right above it already guards this substitution with
pragma.data == calibration.identifier.target; the CAPTURE branch was missing the same check.

In realistic calibrations (see this repo's own benches/sample-calibrations.quil), CAPTURE
writes raw IQ data to an intermediate/unclassified memory region, and the actual routing to the
calibration's target happens later via a PRAGMA FILTER-NODE / PRAGMA LOAD-MEMORY classifier
pipeline. The bug redirected that raw capture straight into the final (classified) register, e.g.:

# before expansion (calibration body)
DECLARE q0_unclassified REAL[2]
NONBLOCKING CAPTURE 0 "ro_rx" boxcar_kernel(...) q0_unclassified[0]
...
PRAGMA LOAD-MEMORY q0 "addr"

# after expansion (buggy)
NONBLOCKING CAPTURE 0 "ro_rx" boxcar_kernel(...) ro[0]   # <- raw IQ capture now targets the BIT register directly
...
PRAGMA LOAD-MEMORY q0 "ro[0]"                             # <- this substitution was already correct

Fix

Add the same target-name guard to the Capture arm that the Pragma arm already has: only rewrite capture.memory_reference when it equals calibration.identifier.target.

Instruction::Capture(capture)
    if calibration.identifier.target.as_deref()
        == Some(capture.memory_reference.name.as_str()) =>
{
    if let Some(target) = &measurement.target {
        capture.memory_reference = target.clone()
    }
}

Testing

  • New unit test measure_calibration_does_not_overwrite_unrelated_capture_memory_reference, reproducing the bug against a realistic calibration shape (asserts the CAPTURE keeps its own memory region after expansion).
  • New test_expansion snapshot case Measure-Calibration-Capture-Memory-Reference covering both branches in one calibration: a CAPTURE targeting an unrelated region is left untouched, while a CAPTURE that does target the declared parameter is correctly substituted.
  • cargo fmt --all -- --check and cargo clippy --workspace --all-targets --all-features -- -D warnings: clean.
  • cargo test (workspace): all 2977 tests + doctests green, no snapshot regressions.
  • cargo make test-quil (quil-py pytest + stubtest): green. The fix touches no pub/pyclass/pymethods surface, so no Python-facing behavior or types change beyond the corrected runtime output.

Known limitations / open questions for reviewers

  • This narrowly fixes the CAPTURE overwrite. It does not address a related gap also visible in the issue's own repro: a calibration that routes its result via e.g. MOVE addr[0] ... (rather than through PRAGMA LOAD-MEMORY or a directly-matching CAPTURE) still won't have that reference substituted at all. That seems like a broader question - should target substitution walk every instruction/operand referencing the formal target parameter, not just these two special-cased instruction kinds and felt out of scope for this fix. Happy to open a follow-up issue if that's worth tracking separately.
  • I wasn't able to run scripts/check-quil-api.sh to completion locally (missing the knope binary), so I couldn't fully reproduce the CI check-api gate. The diff here touches no public API surface, so I don't expect it to flag anything, but wanted to call this out rather than claim a check I couldn't actually finish running.

This is my first contribution to quil-rs so feedback very welcome, especially on whether the target-matching approach here is the right mental model for how DEFCAL MEASURE substitution is supposed to work.

This branch has not been deployed

No deployments
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.

Expanding calibrations replaces CAPTURE memory reference

1 participant