Make observable metadata win over observable annotations (fixes #377)

[ciaranra]

Fixes #377.

DetectorErrorModel::from_circuit returned DEMs with spurious error mechanisms for any circuit carrying both circuit annotations and detector/observable metadata, which is what the surface-code generators produce. An independent Stim oracle agreed with the other DEM path and disagreed with from_circuit (86 mechanisms versus 76 at d=3).

Root cause

An observable annotation silently overrode the observable declared in metadata JSON.

build_dem_from_circuit installed annotation-derived DEM outputs into the influence map (merge_dem_outputs_from) before it read the metadata attributes. The annotation's definition therefore won during mechanism generation even though with_observables_json was called with the caller's explicit declaration.

Minimal reproduction -- metadata covers both measurements, the annotation covers only the first, so the probability identifies which definition was used:

WITHOUT annotation:  error(0.375) L0     <- 2 * 0.25 * 0.75, observable over BOTH measurements (metadata)
WITH annotation:     error(0.25)  L0     <- flips only on the first measurement (annotation)

The asymmetry is essential. With one measurement per qubit and a symmetric observable the two definitions produce identical DEMs, and an earlier version of this test proved nothing for exactly that reason.

The fix

Metadata is parsed before annotations are collected, and observable annotations are excluded when the circuit supplies observables JSON:

let annotation_map = if obs_json.is_some() {
    annotations.with_circuit_annotations_excluding_observables(circuit)
} else {
    annotations.with_circuit_annotations(circuit)
}
.build();

with_circuit_annotations_excluding_observables is a new pub(crate) sibling sharing one inner implementation, so the ~25 existing callers of the public method are untouched. Tracked-Pauli annotations are still collected either way -- metadata has no equivalent for them.

This matches the precedence the very next block already applied to observable records: metadata first, annotations only as fallback.

Coupled fix: observable labels from metadata

Excluding annotations initially broke sampler_paths_preserve_output_split_for_noiseless_and_forced_faults, which expected a DEM-output label of "obs0" and got None.

That was not the test encoding the bug. parse_observables_json discarded the label field. ParsedObservable had no label, and DEM-output labels were populated in exactly one place (influence_builder.rs:441) from annotation-derived outputs -- so annotations were the only possible source of an observable label, even though the metadata format already carries one and callers already write it.

Editing that test to expect None would have been a real regression. Instead the metadata format is now self-sufficient: ParsedObservable carries label, parse_single_observable reads it, and try_build attaches it when emitting the observable. The previously-failing test now passes on its own merits, with the label coming from the metadata that always declared it.

Testing

Four tests, each verified to FAIL when the corresponding change is reverted -- not merely to pass:

test	pins
test_observable_metadata_wins_over_observable_annotation	metadata records win over a disagreeing annotation
test_observable_annotation_used_when_metadata_absent	the fallback path still works (passes either way by design -- guards against over-fixing)
test_observable_label_comes_from_metadata_without_annotations	a metadata label reaches the DEM with no annotation present
sampler_paths_preserve_output_split_for_noiseless_and_forced_faults (existing)	label survives when both sources are present

• cargo test -p pecos-qec green: 598 lib tests and 58 doctests, 0 failed.
• cargo check --workspace --all-targets, cargo clippy -p pecos-qec --all-targets, cargo fmt --all --check all clean.

End-to-end verification (draft criterion met)

Built the pecos-rslib extension from this branch into an isolated venv and re-ran the original Stim comparison. Same script, same circuits, only the binary differs:

	from_circuit mechanisms	agrees with Stim?
dev (pre-fix)	86 (Z basis) / 80 (X basis)	no -- all four configs
this branch	76 in all four	yes -- all four configs

Covered: interaction_basis="cx" and interaction_basis="szz", szz_physical_prefixes=True, each in Z and X basis, d=3, 1 round, p2-only. influence == stim in both runs, confirming that path was unaffected and that nothing else shifted.

The spurious mechanisms reported in #377 are gone.

One methodology note: a pecos_rslib.pth initially shadowed the built wheel with the repo source tree, which would have silently tested the pre-fix binary. The run above loads the wheel's own pecos_rslib.abi3.so from the isolated venv.

[ciaranra]

Independent adversarial review returned REVISE. I verified the load-bearing findings against the code rather than taking them; all three confirmed. Converting this back to draft.

Confirmed: the fix covers one of two entry points

DemSampler::from_circuit (sampler.rs:424) carries the identical defect this PR fixes in build_dem_from_circuit:

let annotation_map = InfluenceBuilder::new(circuit)
    .with_circuit_annotations(circuit)
    .build();
influence_map.merge_dem_outputs_from(&annotation_map);
// ... metadata read afterwards

So #377 still reproduces through DemSampler::from_circuit / the Python DemSampler.from_circuit. I fixed DemBuilder and did not check its sibling. The two need shared preprocessing rather than parallel copies -- the same "two hand-maintained copies of one thing" shape as #376.

Confirmed: this PR regresses the observable label on the real surface path

My end-to-end verification compared error mechanisms only, so it could not see this.

The surface generator writes observables JSON without a label (circuit_builder.py:2681):

observables = [{"id": 0, "records": logical_rec_offsets}]

and puts the label solely on the annotation (circuit_builder.py:2797):

circuit.observable(obs_refs, label=f"logical_{basis.upper()}")

Excluding observable annotations therefore removes logical_Z / logical_X from the generated DEM -- and also the observable's Pauli metadata, which metadata JSON has no field for at all. My label plumbing only helps callers who put a label in the JSON; the default generator does not.

Confirmed: the label conflict path is fail-silent in release

merge_observable_definition (types.rs:5104) resolves conflicting labels with debug_assert_eq! -- panics in debug, silently keeps the existing label in release. Parsing metadata labels makes that path newly reachable, so this PR increases exposure to it.

On failing fast and loudly

Reviewing this against the project's fail-loud rule, the current answer is no, in three places:

1. debug_assert_eq! above -- the worst of both worlds: a debug-only panic with no message a user could act on, and silent divergence in release. Should be a real error naming both labels and the observable.
2. parse_single_observable (builder.rs:2667) silently discards a malformed label: "label": 42 is treated identically to no label. The richer DEM parser correctly rejects non-string/non-null labels (types.rs:3351). Should be a ParseError.
3. This PR's own precedence is silent. Rejecting a genuine conflict is the right end state, but only after Batched MZ nodes: InfluenceBuilder records one measurement, DagFaultAnalyzer records one per qubit #382, since today the "conflict" is PECOS misresolving the annotation (see below).

Revised plan

1. Batched MZ nodes: InfluenceBuilder records one measurement, DagFaultAnalyzer records one per qubit #382 first, and broader than filed. The review found the one-index-per-node map in three places (builder.rs:3331, sampler.rs:1109) plus a related batched-node defect in InfluenceBuilder (influence_builder.rs:229) where a reference to a batched measurement node becomes a propagation term over every qubit that node measures. Fixing .or_insert alone is not sufficient -- it needs per-measurement identity (node + qubit, or a stable MeasId).
2. Then decide the authoritative representation for observables -- records, label and Pauli -- so excluding annotations cannot silently drop fields metadata is unable to express. Either extend the metadata format or validate equivalence and merge the annotation-only fields.
3. Then this PR's precedence, applied to DemBuilder and DemSampler through one shared path, with loud rejection of genuine conflicts.
4. Independently of all of the above: make the two silent failures in (1) and (2) above loud.

The review confirmed the three tests are sound (test_observable_annotation_used_when_metadata_absent passes against pre-PR code by design -- it guards against over-fixing rather than detecting the bug), and that tracked-Pauli ID allocation and ordering are unaffected. The defect is scope, not the mechanism.

[ciaranra]

Parking this and splitting out the part that stands alone.

Split out: #388 -- carrying observable labels from metadata JSON, plus rejecting a malformed label rather than silently dropping it. That work was originally needed here (excluding annotations otherwise lost labels) but is not contingent on the precedence decision: the metadata format documents a label, callers write one, and the parser discarded it. It is worth landing on its own merit.

Parked: the precedence change itself. Three reasons.

1. Its premise weakened. This PR made metadata authoritative on the grounds that the annotation-derived observable was untrustworthy. Resolve Python measurement refs against the circuit instead of fabricating them #386 established why it was untrustworthy -- the Python binding fabricated record_idx: 0 for every reference, so an observable over three measurements collapsed to one. That is a corruption, not a disagreement. Deciding precedence against corrupted data decides the wrong question.

2. It is incomplete. Review found DemSampler::from_circuit (sampler.rs:424) carries the byte-identical defect this PR fixes in build_dem_from_circuit, so from_circuit and generate_dem_from_tick_circuit produce different DEMs for the same circuit #377 would still reproduce through the Python DemSampler.from_circuit.

3. It would regress the generated surface path. The surface builder writes observables JSON without a label and puts it solely on the annotation (circuit_builder.py:2681, :2797). Excluding annotations therefore strips logical_Z/logical_X from generated DEMs, along with the observable's Pauli, which the JSON format cannot express at all. My end-to-end verification compared error mechanisms only and could not see this.

What should happen instead

After #387 fixes measurement-record resolution, a disagreement between metadata and annotations will mean the two sources genuinely differ. At that point the right response is probably to reject it loudly rather than silently pick a winner -- the conflict check prototyped on #377 already works and is what surfaced the [-9, -9, -9] corruption in the first place. It was not shipped only because, today, it fires on the standard surface path for a defect that is PECOS's fault rather than the caller's.

Leaving this open as a placeholder for that decision rather than closing it, since the underlying question -- what happens when both sources describe the same output -- is still unanswered and worth answering deliberately.