Resolve Python measurement refs against the circuit instead of fabricating them - #386
Conversation
|
Two independent deep reviews of The blocker: the conversion still maps by storage order
Reproduced with Wrong qubit, wrong gate type, silent. Fable added two things I had not established:
Both reviewers also pinned why my tests missed it: What both reviews confirmed is soundWorth recording, since it is most of the change:
Also found, now tracked in #387
Where this leaves the PRFive review rounds, five real defects, three introduced by me while fixing the previous one. The pattern is consistent: I fix the site I am looking at and do not check the other half of the same invariant. Both reviewers propose the same small fix -- key |
Part of #382, and fixes #377 for the generated-surface reproduction. Two coupled defects in how measurement records are identified between Python,
TickCircuitandDagCircuit.1. The Python binding fabricated measurement references
extract_tick_meas_refs(dag_circuit_bindings.rs) rebuilt aTickMeasReffrom a(tick, gate_idx, qubit)tuple and invented the rest:PyTickCircuit::detectorandPyTickCircuit::observablepass the result straight toTickCircuit::detector/observable, which readrecord_idximmediately. So every annotation built from Python collapsed all of its references onto record 0. The comment's premise -- thatTickCircuitfills the field in later -- does not hold for the only callers there are.TickCircuit::meas_ref(tick, gate_idx, qubit)now recovers the real reference by reading theMeasIdthatmz/mz_freestored alongside the qubit; the binding returns aValueErrornaming the offending tuple rather than fabricating one.An intermediate revision instead reconstructed the record index by walking stored ticks and batches. Review caught that this is wrong, and it reproduces: storage order is not allocation order. A later measurement can be appended to an earlier compatible batch, and
tick_atpermits filling ticks out of sequence. Interleavingmz(q0),mz_free(q1),mz(q2)allocates records0, 1, 2but stores[MZ(q0,q2), MeasureFree(q1)], and the reconstruction returnedq1 -> 2,q2 -> 1. That is worse in kind than the bug it replaced: the placeholder was obviously wrong, a swapped index is plausibly wrong. Reading the stored id is order-independent and needs no reconstruction.Known limitation:
mz_with_idslets callers supply external stable ids (Guppy result ids) that are not positional, and for those the record index and the id genuinely differ.TickCircuitdoes not store the positional ordinal separately, so that case cannot be resolved here. It is tracked in #387 together with the reverse-conversion conflation, which would defeat any fix confined to this side anyway.2.
MeasureFreedid not advance the Tick -> DAG record counterTickHandle::mz_freeadvancesnext_meas_recordexactly likemz, but the conversion countedMZonly, so later records became unmappable and the annotation remap silently dropped them.These two must land together. Fixing only the binding produces correct record ordinals that the conversion then cannot resolve -- trading "collapsed onto record 0" for "dropped entirely", which is not an improvement. An earlier revision of this PR narrowed to the binding alone; review caught that the narrowed form was not a safe intermediate state, and this restores the pairing.
Verified end to end
Built the extension into an isolated venv and compared d=3 surface DEMs against Stim:
from_circuitmechanismsdevL0membershipcxandszzwith physical prefixes, Z and X basis.Testing
Python-level regressions, which is what the previous revisions lacked. Both were verified to fail against a placeholder-reverted build and pass against this one:
test_observable_over_two_measurements_uses_both-- an observable over two measurements must yielderror(0.375) L0(2 * 0.25 * 0.75, odd parity of two independentp_measerrors). Collapsed references cancel by XOR parity and give a different probability or noL0at all.test_measurement_ref_from_another_circuit_is_rejected-- a tuple that identifies no measurement raisesValueErrorinstead of being fabricated.Rust tests, each mutation-verified to be killed by reverting its change:
meas_ref_recovers_what_mz_returned,meas_ref_rejects_refs_that_name_no_measurement,meas_ref_record_ordinal_is_independent_of_meas_id,measure_free_consumes_a_record_ordinal,annotation_records_survive_conversion_across_measure_free.meas_ref_is_independent_of_batch_and_tick_storage_ordercovers the interleaving case above and kills the reconstruction approach. It was added only after review found that defect -- the earlier tests all built circuits sequentially and could not see it.cargo test -p pecos-quantum -p pecos-qecgreen: 774pecos-quantumlib tests, 0 failed across both.cargo check --workspace --all-targets,cargo clippy -p pecos-quantum -p pecos-rslib --all-targets,cargo fmt --all --checkclean.Deliberately not fixed here -- tracked in #387
MeasureLeakedis a measurement inGate::validateand in DAG→Tick, but not in Tick→DAG ormeas_ref.tick_circuit.rssite panic was tried and reverted: panicking insideimpl From<&TickCircuit> for DagCircuitis not a safe resolution for an infallible std conversion trait.