Clarify sparse pyQuil unitary errors - #1291
Conversation
Reject sparse qubit indices before pyQuil leaks a low-level permutation failure and point callers to the supported compaction paths. Add a regression test and changelog entry. Signed-off-by: Sanjay Santhanam <51058514+Sanjays2402@users.noreply.github.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Signed-off-by: Sanjay Santhanam <51058514+Sanjays2402@users.noreply.github.com>
ryanhill1
left a comment
ryanhill1
left a comment
There was a problem hiding this comment.
Verified both symptoms from #1287 are resolved — the sparse unitary() call and the circuits_allclose path now give the actionable error, and both suggested remedies work. One issue with the guard condition inline; also needs a rebase, main has moved and the CHANGELOG conflicts.
| """Return the unitary of a pyQuil program.""" | ||
| program_copy = self.remove_measurements(self.program) | ||
| qubits = sorted(program_copy.get_qubits()) | ||
| if qubits and qubits != list(range(len(qubits))): |
There was a problem hiding this comment.
This checks the gate qubits for contiguity, but program_unitary only needs the largest gate index bounded by the n_qubits you pass — and that's self.num_qubits, which counts measurement qubits too. The two diverge whenever a program measures a qubit it never gates:
prog = Program(Declare("ro", "BIT", 3), H(0), CZ(0, 2))
for q in range(3):
prog += MEASURE(q, ("ro", q))
load_program(prog).unitary()num_qubits is 3 here, so the call is correctly sized and returns (8, 8) on main. But remove_measurements strips the MEASUREs first, leaving gate qubits [0, 2], so this now raises — and it's a dead end: get_qubits() is already [0, 1, 2], so remove_idle_qubits() and index_contig=True are both no-ops that re-raise the identical error.
The condition from #1287 fixes it and keeps your new test green as-is:
| if qubits and qubits != list(range(len(qubits))): | |
| if qubits and max(qubits) >= self.num_qubits: |
Worth adding a regression test for the measure-the-whole-register shape too, since that's the case the current guard gets wrong.
There was a problem hiding this comment.
You are right, and your example does raise on the branch as it stood. Switched to the #1287 condition:
if qubits and max(qubits) >= self.num_qubits:
and added test_unitary_allows_measured_only_qubits covering the measure-the-whole-register shape (Declare ro BIT 3, H 0, CZ 0 2, MEASURE 0-2) asserting an (8, 8) unitary. With the old qubits != list(range(len(qubits))) condition restored that test fails, and it passes with the fix. The sparse test is unchanged and still green. 9 passed in tests/programs/test_program_circuits_pyquil.py. Pushed as e0e706b.
Gate qubits alone can be non-contiguous while the program is still correctly sized, e.g. when a qubit is measured but never gated. Compare the largest gate index against num_qubits instead.
Argus reviewAuto-review is off for this repo. Tick the box below to run a review on this PR.
Estimated cost
Tip: you can also comment |
2 participants
Closes #1287
Summary of changes
Sparse pyQuil programs now fail before pyQuil's low-level permutation logic and explain both supported qubit-compaction options. The regression test fails with
Permutation SWAP index not validbefore the fix and passes with the actionable error; all 8 focused pyQuil program tests and Ruff checks pass.