Carry observable labels from metadata JSON and reject malformed ones - #388
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The observables metadata format carries a
label, and callers already write one, butparse_observables_jsondiscarded it -- so a label could only ever reach a DEM through a circuit annotation.Split out of #384, which is parked. This part stands on its own: it makes the metadata format self-sufficient, independently of any decision about annotation-versus-metadata precedence.
The gap
ParsedObservablewas{ id, records, meas_ids }-- no label.parse_single_observableread the id and measurement refs and ignored"label"entirely. DEM-output labels are populated in exactly one place,influence_builder.rs:441, from annotation-derived outputs.So a caller writing
[{"id": 0, "records": [-1], "label": "logical_Z"}]got an unlabelled observable, with no indication the field had been dropped.
The change
ParsedObservablecarrieslabel,parse_single_observablereads it, andtry_buildattaches it when emitting the observable.A malformed label is now rejected rather than silently treated as absent:
"label": 42previously parsed as "no label", which is the same silent-discard pattern the richer DEM metadata parser already rejects. An explicitnullremains equivalent to omitting the field.Testing
Both behaviours mutation-verified -- each is killed by reverting the corresponding change:
DemOutputas_strdrop)test_observable_label_comes_from_metadata_without_annotations-- a metadata label reaches the DEM with no annotation present.test_observable_metadata_rejects_non_string_label--"label": 42is aParseErrornaming the problem.test_observable_metadata_accepts_null_label-- explicitnullparses as no label.cargo test -p pecos-qecgreen: 598 lib tests and 58 doctests, 0 failed.cargo clippy -p pecos-qec --all-targetsclean, forced cold;cargo fmt --all --checkclean.Relationship to #384
#384 proposed making metadata authoritative over annotations. That is parked: investigation in #386 established that the annotation-derived observable on the surface path was corrupted rather than legitimately disagreeing, so the precedence question should be decided after #387 fixes measurement-record resolution -- at which point a disagreement means something real and can be rejected loudly.
This label work was originally required by #384 (excluding annotations otherwise lost labels) but is not contingent on it.
Interaction with #385, and a doc correction
#385 (now on
dev) madeadd_observablepanic in every build when two definitions of the same observable disagree about the label or the Pauli. Before this PR, metadata labels were discarded, so metadata could never contribute a label and never conflict. After it, it can.Independent review confirmed by running: a circuit with annotation label
logical_Zand metadata labelfrom_metadataon one observable now panics, where ondevit silently keeps one and discards the other. That is #385's stated target case -- its doc names "metadata JSON and a circuit annotation contradicting each other" explicitly -- so the two changes are complementary rather than conflicting. This PR is what makes that case reachable.Real generators are unaffected. The surface builders write observables JSON with
id/recordsand nolabel(circuit_builder.py:2681,logical_circuit.py:1008), so the annotation label wins uncontested. Verified by reading both writers and by running a surface-shaped mimic.Review also found the
try_*constructors' docs stale as a result: they promise "returns parser errors instead of dropping malformed metadata", but a metadata-versus-annotation conflict -- newly reachable because of this PR -- panics rather than returningDemBuilderError. All four (try_from_circuit,try_from_circuit_with_noise_config,try_from_tick_circuit,try_from_tick_circuit_with_noise_config) now carry a# Panicssection saying so. Malformed metadata is still an error; contradictory sources are not a parse failure and are not reported as one.