Skip to content

Fix silent contact-matrix/transmission-mode misattribution#20

Open
gavdoubleu wants to merge 22 commits into
IDAS-Durham:mainfrom
gavdoubleu:default_contact_matrix_fix
Open

Fix silent contact-matrix/transmission-mode misattribution#20
gavdoubleu wants to merge 22 commits into
IDAS-Durham:mainfrom
gavdoubleu:default_contact_matrix_fix

Conversation

@gavdoubleu

Copy link
Copy Markdown
Contributor

Title: Fix silent contact-matrix/transmission-mode misattribution (default-matrix path + per-mode path)

Original bug

contact_matrices.yaml's default_contacts_matrix is meant to be the fallback
contact matrix used whenever a (venue type, transmission mode) pair has no
dedicated entry. Every production config writes this key in modes: form
(per-mode matrices), but the loader only ever parsed it as a flat single
matrix. Result: default_contacts_matrix silently resolved to an empty
matrix on every real config, and every lookup that needed it fell through to
a hardcoded scalar, ContactMatrixConfig::default_contacts = 2.0, with no
error, no warning, and no relation to whatever beta/contact rate the user had
actually configured. Simulated infection numbers for any venue/mode without
an explicit matrix were silently wrong, and there was no way to tell from
config or logs that this was happening.

There was a second, more subtle layer to this: even after fixing the parser,
ContactMatrixConfig::resolve() only built the per-mode default lookup table
(default_mode_matrices_by_id) when mode_names was non-empty. A config
that supplies default_contacts_matrix.modes but has no venue-level
contact_matrices: ... modes: block leaves mode_names empty, so the table
was never built and the per-mode default stayed unreachable — surfacing
later as a runtime std::runtime_error instead of the load-time failure ADR
0006 was meant to guarantee.

A third, independent bug affected the primary (non-default) per-mode lookup:
disease.yaml's transmission_params.modes and contact_matrices.yaml's
per-venue modes: blocks are two independently-ordered lists. getMatrix/
getVirtualMatrix indexed contact matrices by the raw disease-mode index
into contact_matrices.yaml's list, silently misattributing a matrix
whenever the two files declared modes in different orders (e.g. a fomite
matrix applied to droplet transmission).

Fix, in order

  1. 2aa6dd3 / 9bed681 — Parse default_contacts_matrix in both flat and
    modes: form (mirroring the existing per-venue parser), bin-resolve it
    like every other matrix, and delete the default_contacts scalar
    fallback entirely. The key is now mandatory: a venue/mode with no matrix
    anywhere is a load-time error, not a silent stand-in value (ADR 0006).
  2. 5d64c38lookupContactsForBinPair now throws instead of returning
    0.0 when a matrix is still missing at that point — by then the default
    chain guarantees non-null, so hitting this branch means a real bug
    (mismatched bins), not a config gap to paper over.
  3. 15e4da5getVirtualMatrix (virtual encounters) never consulted
    default_contacts_matrix at all, unlike the venue path — an encounter
    type with no dedicated matrix crashed instead of falling back. Extracted
    the shared default-fallback tail (applyDefaultChain) so venues and
    virtual encounters go through the same fallback logic.
  4. d188e3c — Fixed the mode_names-empty gap described above: added
    ContactMatrixConfig::finalizeDefaultModeMatrices(world, disease_mode_names), called from Simulator::Simulator right after
    disease_ loads (the first point both the disease's canonical mode list
    and ContactMatrixConfig co-exist), which rebuilds the per-mode default
    table keyed to the disease's actual modes and throws if any disease mode
    has neither a per-mode default nor a flat fallback (ADR 0007).
  5. e99e511 — Consolidated the bin-resolution logic that step 4
    duplicated from resolve().
  6. e5bbcec / efe472f — Removed the now-dead default_contacts key from
    tracked config YAMLs; untracked config_plague_fitting /
    config_plague_fitting_short (large generated fixtures that shouldn't be
    version-controlled).
  7. bc18375 — Fixed the primary (non-default) per-mode lookup positional
    mismatch described above. Added
    ContactMatrixConfig::finalizeDiseaseModeAlignment, building a
    name-matched disease-mode-index → contact-matrix-mode-index permutation
    vector (mode_index_translation_) once at Simulator construction,
    right after finalizeDefaultModeMatrices. getMatrix/getVirtualMatrix
    translate through it via a new translateModeIndex helper before
    indexing. A disease mode absent from contact_matrices.yaml falls
    through to the existing default chain; a contact_matrices.yaml mode
    absent from the disease warns and is ignored (configs may be shared
    across diseases with differing mode sets).
  8. 8bddce4 — Closed two follow-up correctness gaps in the step-7
    mechanism, found by review:
    • finalizeDiseaseModeAlignment's name-matching used break-on-first-match
      with no uniqueness check, so two transmission_params.modes entries
      sharing a name (or both omitting one, defaulting to "default")
      collided — the second mode's matrix silently aliased onto the first's,
      the same failure class as step 7 fixes, just triggered by an in-file
      naming collision rather than cross-file ordering. Now throws on a
      duplicate disease mode name instead of aliasing.
    • translateModeIndex used an empty mode_index_translation_ to mean
      "not yet finalized, fall back to raw positional indexing" — but
      finalizing with a genuinely empty disease-mode list produces the same
      empty vector, making "finalized, zero modes" indistinguishable from
      "never finalized" and reverting to unsafe positional lookups. Added an
      explicit disease_mode_alignment_finalized_ flag as the sentinel
      instead of inferring it from vector emptiness.

See docs/adr/0006-default-contact-matrix-fails-loud-no-scalar-fallback.md
and docs/adr/0007-default-mode-matrix-finalised-in-simulator-constructor.md
for the design rationale behind steps 1-6. Steps 7-8 are bug fixes to that
existing mechanism, not new architectural decisions, so no new ADR.

Test plan

  • ctest — fixtures rewritten off ContactMatrixConfig::default_contacts;
    new fixtures cover missing default, missing mode in default, empty
    mode-names-with-per-mode-default, reversed disease/contact-matrix mode
    order, orphaned contact-matrix mode names, duplicate disease mode names,
    and zero-disease-mode finalization
  • Run a config with default_contacts_matrix.modes and no venue-level
    contact_matrices: modes: block — confirm it no longer throws at runtime
    and no longer silently uses a stale scalar
  • Run a config where disease.yaml and contact_matrices.yaml declare
    the same mode names in different orders — confirm matrices attach to the
    correct mode

gavdoubleu added 18 commits May 28, 2026 15:00
Reproducible conda environment for building and running JUNE2. Includes cxx-compiler (GCC on Linux), cmake, hdf5, yaml-cpp, openmpi, Python test deps (h5py, numpy), and optional deps metis and gperftools.
default_contacts is a hardcoded scalar fallback that ADR-0006 deletes,
since it silently masks a parsing bug where modes:-format
default_contacts_matrix configs resolve empty. Ahead of that fix, ~15
call sites across 11 test files set cm.default_contacts directly as a
fixture knob for a distinguishable contact-rate canary; each is
rewritten to build a one-bin ContactMatrix (bins={"all"},
contacts={{X}}) and assign it to cm.default_matrix instead, which
getMatrix() already consults before falling through to the scalar, so
runtime behaviour is unchanged. Also drops test_config_loading.cpp's
SUBCASE asserting the scalar loads from YAML, since that field is
going away; the YAML fixtures' now-unused default_contacts: keys are
left alone for the follow-up loader change to clean up.

All 11 rewritten test binaries pass (test_mpi_transmission_modes
verified via mpirun -np 2).
default_contacts_matrix in contact_matrices.yaml was parsed with the
flat-only parser even though every production config writes it in
modes: format, so it silently resolved empty and every lookup fell
through to a hardcoded default_contacts scalar (2.0) with no relation
to the user's configured beta.

Step 1 of the fix (loader/runtime changes to actually populate and
require the new fields land separately):
- ContactMatrixConfig gains default_mode_matrices (mode_name ->
  ContactMatrix) and a resolved default_mode_matrices_by_id lookup,
  mirroring the existing per-venue mode_matrices/mode_matrices_by_id
  pattern. getMatrix(venue_type_id, mode_index) now falls back to the
  per-mode default before the flat default_matrix.
- resolve() now bin-resolves default_matrix and each entry of
  default_mode_matrices (male/female bin, subset-type bin, age-to-bin
  table) - default_matrix previously skipped this step entirely, so
  any lookup that reached it saw unresolved bin fields.
- Deleted ContactMatrixConfig::default_contacts. Its two remaining
  call sites are stubbed pending the next steps: config_loader.cpp no
  longer reads the now-deleted YAML key, and
  InteractionManager::lookupContactsForBinPair returns 0.0 with a TODO
  where it used to return the scalar (load-time validation coming
  next will make that branch unreachable in practice).

Full build and test suite pass (42/43; the one failure is a
pre-existing missing-world-file issue unrelated to this change).
default_contacts_matrix was always parsed flat, so every production
config (which writes it in modes: form) silently resolved to an empty
matrix and fell through to the now-deleted default_contacts scalar.
Rewrite ConfigLoader::loadContactMatrices to require the key (throws
naming the filename if absent) and dispatch on modes: presence, mirroring
parseContactMatricesList's per-venue handling: modes: form parses each
mode into default_mode_matrices and validates coverage against
config.mode_names, throwing on any disease mode left uncovered; flat
form parses into default_matrix as before.

Drop the now-dead default_contacts: scalar lines from
contact_matrices.yaml and romantic_regression.yaml fixtures; the latter
had no default_contacts_matrix at all, so add a flat one (mirroring
romantic_encounter's shape) since the key is now mandatory.

Add SUBCASEs and fixtures exercising modes-form loading, the
missing-key throw, and the missing-mode-in-per-mode-default throw.
By this point mode_matrix/fallback_matrix already route through
ContactMatrixConfig::getMatrix's default-matrix fallback chain (now
guaranteed non-empty since default_contacts_matrix is required, see
2aa6dd3/9bed681), so both being null/out-of-bounds means mismatched
bins - a real bug to surface, not a made-up contact rate to hide it.

Full ctest run unaffected: 42/43 pass, same pre-existing unrelated
test_mpi_full_reproducibility failure as before.
getVirtualMatrix(encounter_type_id, mode_index) never consulted
default_contacts_matrix, unlike getMatrix's venue-path chain -- an
encounter type with no matching virtual_contact_matrix entry (missing
config, typo) resolved to a null matrix and crashed
lookupContactsForBinPair at first use, instead of failing loud at
config-load time or falling back sensibly like venues do.

Extracted applyDefaultChain() as the shared tail (per-mode default,
then flat default_matrix) used by both getMatrix and getVirtualMatrix,
so virtual encounters now fall through to the same
default_contacts_matrix a venue would -- no separate virtual-specific
default. Added a regression test covering an unconfigured encounter
type, and corrected the now-stale invariant comment on
lookupContactsForBinPair.
…esolve

Four near-identical 6-line blocks (male_bin/female_bin/bin_by_subset_type/
age-bin resolution) had built up across per-venue matrices, per-venue mode
matrices, default_matrix and default_mode_matrices - the latter two added
this branch to satisfy ADR 0006's fail-loud requirement, copy-pasting the
existing per-venue pattern rather than reusing it. A fifth, already-
deduplicated copy (resolve_matrix_bins lambda + bin_resolved guard) existed
further down for virtual-encounter matrix aliasing.

Hoist resolve_matrix_bins above all five use sites and call it everywhere;
delete the now-redundant second declaration. The dedup guard is a no-op for
the four venue/mode/default sites since each matrix there is touched once,
so behaviour is unchanged. Net -35 lines, one place to edit bin-resolution
semantics instead of five.
default_contacts was replaced by default_contacts_matrix (2aa6dd3) and
the loader no longer reads it (parseContactMatrixScalars deleted). Left
in place it silently misleads: a researcher editing default_contacts
next to default_contacts_matrix would expect it to affect the sim, but
it has zero effect.
Files stay on disk but git no longer tracks them.
A config supplying default_contacts_matrix.modes but no contact_matrices
venue modes: block left ContactMatrixConfig::mode_names empty, so
resolve() skipped building default_mode_matrices_by_id entirely (gated
on !mode_names.empty()). The per-mode default was silently discarded at
load, surfacing later as a runtime std::runtime_error from
InteractionManager::lookupContactsForBinPair - exactly what ADR 0006's
load-time validation was meant to prevent.

Add ContactMatrixConfig::finalizeDefaultModeMatrices(world,
disease_mode_names), which rebuilds default_mode_matrices_by_id keyed to
an explicit disease mode-name list instead of mode_names, and throws if
a disease mode has neither a per-mode default entry nor a flat
default_matrix. Wired into Simulator::Simulator right after disease_
loads, the first point both Disease and ContactMatrixConfig co-exist.
Required widening Simulator::config_ from const Config& to Config&.

A disease mode absent from mode_names/per-venue matrices is fine (falls
back to default); absent from the default itself is now a fatal, loud
error rather than a silent nullptr.

Also fixes two test fixtures (test_simulator.cpp) that relied on the
old bug's permissiveness by never configuring a default matrix at all.

Docs: corrected CONTEXT.md's Transmission Mode/Default Contact Matrix
entries, added ADR 0007 recording the Simulator-constructor placement
decision (and rejected alternative of reworking resolve()'s signature).
The separate, deferred order/index-alignment gap between mode_names and
transmission_params.modes (positional lookup, unvalidated) is flagged
as future work only, not fixed here.
…deMatrices

d188e3c added finalizeDefaultModeMatrices() and copy-pasted the
male_bin/female_bin/bin_by_subset_type/age_to_bin logic into a new free
function rather than reusing resolve()'s resolve_matrix_bins lambda -
undoing the dedup 493bbbb had just done, and leaving a comment claiming
the logic was "shared" when resolve() still ran its own copy.

resolve_matrix_bins now delegates to the free function; resolve()'s
local resolveAgeToBin lambda is gone. One implementation, both callers.
disease.yaml's transmission_params.modes and contact_matrices.yaml's
per-venue modes: blocks are independently-ordered lists. getMatrix/
getVirtualMatrix indexed contact matrices by the raw disease mode
index, silently misattributing a matrix to the wrong mode whenever the
two files declared modes in different orders (e.g. fomite applied to
droplet transmission). A prior fix only corrected the default-fallback
path; this fixes the primary per-venue/per-encounter lookup.

Add ContactMatrixConfig::finalizeDiseaseModeAlignment, building a
name-matched disease-mode-index -> contact-matrix-mode-index
permutation vector once at Simulator init (avoids per-lookup string
comparison). getMatrix/getVirtualMatrix translate through it before
indexing; a disease mode absent from contact_matrices.yaml falls
through to the existing default chain, and an orphaned
contact_matrices.yaml mode warns rather than erroring (configs may be
shared across diseases with differing mode sets).

Add regression test with reversed mode order between the two config
files, plus fixture tests/configs/contact_matrices_reversed_modes.yaml.
Update CONTEXT.md's Transmission Mode entry and a stale comment in
disease.h that assumed index-based matching.
Follow-up to bc18375's finalizeDiseaseModeAlignment fix; review surfaced
two ways the same silent-misattribution failure mode could recur.

1. Duplicate/missing disease mode names aliased silently: linear-search
   name matching broke on first match, so two transmission_params.modes
   entries sharing a name (or both omitting one, defaulting to "default")
   collided and the second mode's matrix silently aliased onto the
   first's. finalizeDiseaseModeAlignment now throws on duplicate disease
   mode names instead of aliasing.

2. Ambiguous "not finalized" sentinel: translateModeIndex used an empty
   mode_index_translation_ to mean "finalize not yet called, fall back to
   raw positional indexing". But finalizeDiseaseModeAlignment produces the
   same empty vector when called with zero disease modes, so that state
   was indistinguishable from "never finalized" and would incorrectly
   revert to unsafe raw positional lookups. Added
   disease_mode_alignment_finalized_ as an explicit sentinel, decoupling
   "finalized" from vector emptiness.

Add regression tests for both cases; update CONTEXT.md's Transmission
Mode glossary entry for the uniqueness requirement.
@gavdoubleu
gavdoubleu requested a review from mtcorread as a code owner July 22, 2026 19:38
gavdoubleu and others added 4 commits July 23, 2026 10:32
Fix Lint Code Base CI failure on PR IDAS-Durham#20; whitespace-only, matches the
CI-pinned clang-format version rather than the locally-installed one.
Field name was misleading: it scales a transmission mode's contribution to
force-of-infection (a per-route, per-mode constant), not an individual's
biological susceptibility (which is Person::getSusceptibility, driven by
waning immunity/vaccine efficacy). Renamed the TransmissionMode struct
member, disease_loader.cpp YAML key, all disease/contact-matrix config
files, tests, and USER_GUIDE.md docs to avoid confusing the two concepts.
Longer identifier pushed two lines over the column limit; rewraps them
to satisfy the CI clang-format check.
Internal design notes don't belong on main; git history retains them.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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