Skip to content

Key the annotation record map on MeasId instead of storage order - #390

Closed
ciaranra wants to merge 1 commit into
devfrom
fix-record-map-allocation-order
Closed

Key the annotation record map on MeasId instead of storage order#390
ciaranra wants to merge 1 commit into
devfrom
fix-record-map-allocation-order

Conversation

@ciaranra

Copy link
Copy Markdown
Member

Fixes a defect that landed on dev with #386: the Tick→DAG conversion binds annotations to the wrong measurement when stored order differs from allocation order.

Both reviewers of #386 identified this independently, each by running it, and it was reproduced a third time before that PR merged. #386 merged before the fix was applied.

The defect

TickCircuit::meas_ref reads the MeasId each measurement was given at allocation time, so it is allocation-order-correct. The conversion did not: it rebuilt meas_record_to_node with a running counter over stored ticks and batches, ignoring the ids sitting on each split gate.

Storage order is not allocation order. A later measurement can be appended to an earlier compatible batch (Tick::try_add_gate_piece -> compatible_empty_attr_batch -> append_batch), and tick_at fills ticks out of sequence.

Reproduced on dev with mz(&[0]), mz_free(&[1]), mz(&[2]) -- allocation records 0, 1, 2, stored order [MZ(q0,q2), MeasureFree(q1)]:

observable over record 2 (qubit 2's MZ)
  -> resolved to gate = MeasureFree, qubits = [1]

Wrong qubit and wrong gate type, silently. DagFaultAnalyzer treats MeasureFree as a real measurement column, so the DEM observable tracks the wrong measurement rather than erroring. Reachable from the plain Python API with no tick_at at all: t.mz([0]); t.mz_free([1]); t.mz([2]) on one tick handle.

Note the interaction: before #386 the same scenario silently dropped record 2 under MZ-only counting. #386 fixed the counter, which turned a drop into a wrong answer.

The fix

Key meas_record_to_node on each measurement's MeasId rather than a positional counter. That is what meas_ref reads and what the reverse conversion writes, so all three now agree. Gates carrying no ids are externally constructed and fall back to the counter, which is the best available for them.

This is the fix both reviewers proposed independently.

What was already correct and stays correct

  • Sequential circuits -- the common path, and everything the surface generators produce -- resolved correctly before this change and still do.
  • d=3 surface DEMs still match Stim exactly: 76 mechanisms in all four configurations (cx and szz with physical prefixes, Z and X basis), verified by building the extension into an isolated venv.
  • Both Python binding regressions still pass.

Testing

annotation_reaches_the_named_measurement_regardless_of_storage_order builds the interleaved circuit and asserts the observable resolves to exactly (MZ, [2]) -- not merely to some node, and not merely to a distinct one. That precision matters: #386's tests asserted only that two nodes were distinct, which is why they missed this.

Mutation-verified: restoring the positional counter kills it.

  • cargo test -p pecos-quantum -p pecos-qec green: 775 pecos-quantum lib tests, 0 failed across both.
  • cargo clippy -p pecos-quantum -p pecos-rslib --all-targets clean, forced cold; cargo fmt --all --check clean.

Still open, tracked in #387

MeasureLeaked is excluded from record counting here and in meas_ref (consistently, in the same direction), the remaining silent filter_map drops, the DAG→Tick identity/ordinal conflation, and the mz_with_ids non-positional case. Also #387 item 6: the Python mz bindings advance the record counter before the gate add can fail.

@ciaranra

Copy link
Copy Markdown
Member Author

Two independent deep reviews, Fable and Sol, disagreed on the central question. I adjudicated by running both claims rather than choosing a side, and Sol is right. This should not merge as-is.

The disagreement

Both were asked specifically to attack the mixed id-bearing / id-less case, which I had flagged as untested.

  • Fable: SHIP. It built four probes covering both orderings of id-less versus id-carrying gates, ran each against dev and this branch, and found the branch correct in every one. Its reasoning about .max(meas_id.index() + 1) was sound as far as it went.
  • Sol: REVISE. It tested a case Fable did not: a measurement whose MeasId differs from its positional record index.

Reproduced

One measurement at positional record 0 carrying MeasId(10), with an annotation referencing record 0:

dev:        observable nodes = [1]    <- resolves
this branch: observable nodes = []    <- silently dropped

So this PR trades one silent failure for another. It fixes storage-order misattribution (annotation binds to the wrong measurement) and introduces a drop (annotation binds to nothing) whenever an annotation references a positional ordinal while the gate carries a non-positional id.

Why this cannot be fixed by picking a key space

TickCircuit::detector/observable record record_idx. meas_ref returns meas_id.index(). The forward conversion now keys on MeasId; the reverse conversion already did. Those are two different identifier spaces that coincide only when mz/mz_free auto-assign, and every attempt to fix one side while the other disagrees just moves the failure.

Sol's conclusion is the correct one: settle whether annotation references are record ordinals or identities, consistently, across both conversions and the annotation storage. That is #387 item 4, and it needs the positional ordinal stored at allocation rather than inferred.

Also confirmed by Sol and worth recording: duplicate MeasIds across gates silently last-writer-wins in the map (Gate::validate checks length, not uniqueness), and a batched MZ(q0,q1) with ids [4,4] resolves to q1. Fable independently flagged the same hazard as pre-existing and #387-shaped.

Where that leaves things

dev currently carries the storage-order misattribution from #386. It is reachable from the plain Python API -- t.mz([0]); t.mz_free([1]); t.mz([2]) -- and gives a silently wrong answer.

This PR would fix that but introduce a silent drop on a different construction. Neither state is clean, and I do not think a sixth incremental attempt at this invariant is the right move: five have now been made, four shipped a defect, and three of those were introduced while fixing the previous one.

Recommendation: close this and do the measurement-identity work in #387 as one deliberate change -- store the positional ordinal, make both conversions agree on what an annotation reference means, enforce id uniqueness, and replace the silent filter_map drops with reportable errors.

@ciaranra

Copy link
Copy Markdown
Member Author

Closing per the adjudication above: this fixes storage-order misattribution but introduces a silent drop when an annotation references a positional ordinal while the gate carries a non-positional MeasId. Reproduced both directions.

The underlying problem is that annotation references and gate identities live in two different identifier spaces that coincide only when mz auto-assigns, so any fix that picks one space relocates the failure rather than removing it. Five attempts have now been made at this invariant and four shipped a defect.

Folding into #387 to be done as one deliberate change. Everything learned is recorded there and in the comment above, so that work starts from evidence.

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.

1 participant