Decoder evaluation: paired DUT/reference comparison and shot-corpus save/load - #432
Closed
ciaranra wants to merge 5 commits into
Closed
Decoder evaluation: paired DUT/reference comparison and shot-corpus save/load#432ciaranra wants to merge 5 commits into
ciaranra wants to merge 5 commits into
Conversation
…bind loaded corpora to their DEM
…unt paths fail loud on decoder errors
Member
Author
|
Merged into #421 at the maintainer's request; the branch history is preserved there via a merge commit, and this PR's body remains the detailed record of the evaluation work's review rounds. |
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.
Summary
Adds paired decoder comparison: decode every shot in a
SampleBatchwith a decoder undertest (DUT) and a reference decoder, and report the joint outcome counts.
Today PECOS can tell a researcher "your logical error rate is 5%". It cannot tell them
whether a stronger decoder would rescue those same shots — so the choice between widen
the decoder, fix the noise model, and stop optimizing is a guess. This makes the
first of those questions answerable.
SampleBatch.compare_decoders(dem, dut_decoder_type, reference_decoder_type, alpha=0.05)returns a
DecoderComparisonResultwith:correct/mismatch/decode error), exposed both as a nested list and as named per-cell getters;dut_only_failures— DUT wrong where the reference was right, i.e. measurable headroom;both_failed— shots neither decoder got;pecos-numhelper.Three behaviors are deliberate and tested:
existing
decode_countfolds them; that behavior is unchanged, and not copied.decode_eachbails on thefirst error; this loop records the shot and continues. Both decoders are always run
before either result is classified, so a DUT error cannot hide the reference's answer.
ObsMask, with no 64-observable narrowing anywhere in thepath.
Nothing here claims termination status, MAP-optimality, or "irreducible" failure. Those
are not knowable through
ObservableDecoder— Tesseract's adapter discardslow_confidence, MWPF discards timeout status, and A* returns budget exhaustion as anordinary
Ok— so they are reported as unavailable rather than inferred.Verification
required): all-correct, a known wrong subset, DUT errors, reference errors, a
error as a mismatch kills 3; narrowing the comparison so wide bits vanish kills exactly
the wide test. (A first narrowing attempt survived because the two masks had different
word-vector lengths — a faulty mutant, not a vacuous test; re-run correctly it kills.)
cargo fmt,cargo clippy -p pecos-rslib --all-targets -D warnings, and repo-widepre-commit run --all-filesall clean.Scope
This is the first slice of a larger design (
pecos-docs/design/decoder-failure-diagnosis.md),which went through two adversarial review rounds and shrank substantially as a result.
Deliberately not here: a diagnostic trait, candidate-list/support-loss analysis, a
status ontology, work-metric plumbing, and evidence-weighted committees. Each is recorded
in the design note with the condition that would justify building it.
The natural follow-up is corpus export, now also on this branch.
Shot-corpus save/load (second commit)
SampleBatch.save(path, dem=..., metadata_json=None)/SampleBatch.load(path)freeze asample corpus to a single self-describing file:
PECOSCORPUS\0magic, u32 header length,JSON header, then detector and observable columns as little-endian u64. The header carries
dimensions, the resolved seed, the exact DEM text, sha256 of both DEM and payload, an
opaque caller
metadata_jsonstring, and aformat_version.Why store shots rather than re-seed:
generate_samplesis serial and seed-deterministic,so a seed does reproduce shots on another machine at the same PECOS version — but RNG
stream stability across versions is explicitly not promised (
pecos-random/src/rapid_rng.rsreserves the right to a "deliberate, release-noted reproducibility break"), an external
tool has no access to PECOS's RNG at all, and archived figures should not depend on any
internal staying fixed. (The thread-count non-determinism in
sample_statistics_parallelis real but applies to the parallel statistics/decode-count paths, which discard shots
entirely and therefore cannot be captured at all — a separate, named limitation.)
Supporting fix:
generate_samplespreviously didPecosRng::seed_from_u64(rand::rng().random())whenseed=None, generating andimmediately discarding the seed that produced the samples. It now resolves the seed once,
uses that value, and stores it on the batch, where
saverecords it.saverequires the DEM (the batch does not carry one, and a corpus without it cannot bedecoded) and validates its detector/observable counts against the batch — write time is
the last point where a wrong-DEM mistake is cheap to catch. Decoder identities and configs
are deliberately not in the format: a corpus is the shots, a decoder spec belongs to a
run, and baking today's config shapes into a file format would rot it; callers record that
in
metadata_json.Verified: 12 Rust tests plus 8 Python tests, all re-run independently against a
maturin-built extension. Integrity guards mutation-tested — neutering the payload checksum,
the payload length check, the
format_versioncheck, or the save-time DEM dimension checkeach kills at least one test. The load path uses checked arithmetic throughout and maps
malformed files to
ValueError, never a panic.Robustness review round (third commit)
An adversarial review of the diff returned REVISE. The headline finding was reproduced by
experiment before acting on it: editing one header field (
"num_shots": 65->66)passed the dimension check, the payload-length check, and both hashes, and
load()returned a batch with a fabricated all-zero 66th shot. Only the DEM and payload were
authenticated; the header was not.
Fixes in this commit:
after the length prefix and now covers
header || payload, verified before any headerfield is interpreted semantically.
payload_sha256is gone (subsumed);dem_sha256remains as a convenience identifier, not the integrity mechanism. Duplicate JSON keys
stop being a concern once the header bytes are authenticated.
num_shots == 0the payload length was zero regardless ofdeclared column count, so a tiny file could declare billions of detectors and drive a
huge allocation. Explicit
MAX_SHOTS/MAX_DETECTORS/MAX_OBSERVABLESare nowchecked before anything is allocated from them.
cannot distinguish two same-dimensional models; the docstring said otherwise and has been
corrected. Separately, a batch loaded from a corpus now rejects a different DEM across
save,compare_decoders, and thedecode_*family, with an explicitallow_dem_mismatch=Trueopt-out for deliberate cross-model work.alphanow raiseValueErrorup front instead of surfacing asRuntimeErrorafter decoding the batch.clear_metadatais the explicit way to discard it.
and
FileNotFoundErrorinstead of a bareOSError.Verified: 55 Rust and 18 Python tests, re-run independently; the original tamper attack is
now rejected on
num_shots,seed, andnum_detectors. Guards mutation-tested —neutering the content digest kills 4 tests (including both header-tamper tests), removing
the dimension checks kills 3, and accepting nonzero padding kills 1.
Deliberately not fixed, recorded instead: a 32-bit overflow in
SparseDem::from_dem_str(pre-existing, different crate, unreachable on 64-bit), the reader's payload copies
(performance, not correctness), and the
ObservableDecoderstate contract.Pre-existing defect fixes (fourth commit, requested on-PR instead of as issues)
Two defects found during this work but pre-existing on
dev:DEM parser dimension overflow (32-bit targets).
SparseDem,DemCheckMatrix, andDemMatchingGraphall computedmax_index as usize + 1; on a 32-bit target the maximalindex wraps to zero dimensions in release builds (silent misparse) or panics in debug. A
shared fallible
dimension_counthelper now promotes throughu64and converts withusize::try_from, preserving 64-bit behavior exactly (aD4294967295regression testpins
num_detectors == 4294967296) while producing a cleanInvalidConfigurationwherethe count cannot fit.
parse_dem_metadata's rawu32 + 1counters — which could paniceven on 64-bit — are now checked as well.
Decoder errors were folded into logical-error counts.
decode_count,decode_count_parallel,decode_stats, anddecode_stats_parallelscored a decodeerror as a logical error via
.map_or(true, ...); worse,sample_decode_count_parallelsubstituted the full observable selection mask as the prediction, so an erroring decoder
scored correct on any shot whose truth flips every observable. All scoring now goes
through one fail-loud helper (
decoder_scoring.rs): any decoder error aborts with aRuntimeErrornaming the failing shot (parallel paths report the globally lowestfailing shot index), matching the serial
sample_decode_countprecedent. Callers whoneed per-shot error tolerance use
compare_decoders, which reports errors as a distinctoutcome. For decoders that never error, all counts are unchanged — pinned by a seeded
regression (48 errors from 257 shots) and by masking/timing decorators that preserve the
existing per-shot semantics.
Verified: 189 Rust tests + 30 Python tests re-run independently; reverting the fail-loud
guard kills exactly the two new stub-decoder tests, including the
all-observables-flipped trap. The 32-bit error branch has no demonstrable kill on a
64-bit host — the fix is behavior-preserving there by design and the arithmetic is total
by construction; stated rather than faked.