Dedupe triplicated wire-size/pack/unpack field lists via WireRecord#1
Open
gavdoubleu wants to merge 9 commits into
Open
Dedupe triplicated wire-size/pack/unpack field lists via WireRecord#1gavdoubleu wants to merge 9 commits into
gavdoubleu wants to merge 9 commits into
Conversation
…IDAS-Durham#16) * Add conda environment.yml 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. * Consistent sentinels for event-logging IDs, encounter types, symptoms Event-logging sentinel/fallback choices had drifted: bare -1/255 literals repeated with no named constant, and infector_symptom_id used 0 both as the real "recovered" registry id and as Disease::getSymptomId's own error fallback, so seed-infection rows (no real infector) were indistinguishable from a recovered infector. - Add kInvalidPersonId/kInvalidVenueId (-1), kUnknownEncounterTypeId (255), kNoSymptomId (255) in core/types.h; replace bare literals across PendingInfection, PersonLocation, config.h encounter-type fields. - Narrow infector_symptom_id/old_symptom_id/new_symptom_id uint16_t -> uint8_t end to end (event structs, HDF5 CompType registrations, logger API), freeing symptom id 0 to mean only "recovered". Disease's internal uint16_t representation is untouched; narrowing happens only at the logInfection/logSymptomChange call boundary via explicit static_cast. - Standardise "nothing to report" string fallbacks in lookup tables: "unknown" -> "could not resolve" (value existed but couldn't be resolved), "" -> "not applicable" (nothing to report). Verified: full build passes, test_integration and test_simulator both pass, and a standalone EventLogger spot-check confirms the HDF5 output now has uint8 symptom-id columns with 255 (not 0) for seed infections. * Fix leftover uint16_t/default-value gaps from event-logging sentinel narrowing Commit dbccfd1 narrowed infector_symptom_id (and related event-logging fields) to uint8_t and introduced kUnknownEncounterTypeId/kNoSymptomId sentinels, but two call sites weren't updated to match: - interaction_manager_partial_presence.cpp: unpacking MPI-packed candidate data cast infector_symptom_id to uint16_t, then relied on an implicit second narrowing to fit PendingInfection's now-uint8_t field. Cast directly to uint8_t instead, matching the field type and the explicit style used at the sibling call site in interaction_manager_venue.cpp. - infection_seed.cpp: logInfection call for seed infections omitted the encounter-type and symptom-id sentinel arguments added by dbccfd1 (no infector exists for seeded infections), leaving them at compiler/default-initialised values instead of the intended kUnknownEncounterTypeId/kNoSymptomId sentinels. * Finish sentinel-consistency cleanup from dbccfd1 Leftover raw `255` comparisons (interaction_manager.cpp, interaction_manager_partial_presence.cpp, activity_manager_selection.cpp) and duplicated fallback strings ("could not resolve", "not applicable" in event_writer_lookups.cpp, event_merger_lookups.cpp) sat outside the single point of definition dbccfd1 meant to establish, reintroducing the same drift risk it fixed elsewhere. Replace with kUnknownEncounterTypeId and new kCouldNotResolve/kNotApplicable constants (event_types.h). * Rename kUnknownEncounterTypeId to kDefaultEncounterTypeId; split kUnknownVenueTypeId encounter_type_id is 255 for every ordinary, non-coordinated encounter (the vast majority), only getting a real encounter_type_names index for coordinated encounters. "Unknown" read as an error/undefined state, but this is the expected default for the staple case, so rename to kDefaultEncounterTypeId (still 255, purely a naming fix, no behaviour change). Considered promoting "uncoordinated" to a real registry entry instead, but rejected: interaction_manager.cpp and interaction_manager_venue_bins.cpp use encounter_type_id < encounter_type_names.size() as an implicit "is this coordinated" test, and the partial-presence v1 invariant relies on the sentinel staying out of the registry's range, so a real in-range default id would corrupt encounter stats. Also split kUnknownEncounterTypeId's accidental reuse in activity_manager_selection.cpp (testing a venue-type lookup miss from getVenueTypeId, an unrelated concept) into its own kUnknownVenueTypeId constant. * Finish sentinel-constant rollout in interaction_manager Three venue_type_id defaults in the interaction_manager module still used the raw literal 255, left over from the event-logging sentinel-consistency cleanup (dbccfd1..ac7ad42) that introduced kUnknownVenueTypeId. Swap them in for consistency: ParentAggregate::parent_venue_type_id, processVenueTransmissions' local venue_type_id, and resolveVenueTypeAndMatrix's output default. * Fix MPI wire-format desync in PendingInfection pack/unpack infector_symptom_id narrowed uint16_t->uint8_t (dbccfd1..ac7ad42) shrank the packed record by 1 byte, but INFECTION_SIZE stayed a hardcoded 25 and unpackAndApplyIncoming's local still declared uint16_t. Alltoallv slots stayed 25 bytes/record while packing wrote 24, so any batch of >1 PendingInfection to the same destination rank desynced from the second record onwards, corrupting person_id/infector_id/venue etc with no crash. Fix: derive INFECTION_SIZE/PROPOSAL_WIRE_SIZE/REPLY_WIRE_SIZE from sizeof(decltype(field)) of the actual struct members instead of literals, and make unpackAndApplyIncoming's locals decltype-derived too, so a future struct field-type change can't silently drift pack/unpack apart again. EncounterReply's status_byte stays a manual uint8_t (deliberate enum transcode, not drift). Add regression test: two PendingInfection records routed to the same destination rank in one round, asserting both round-trip intact. * Fix resolveInfectorSymptomId no-infector sentinel mismatch Was returning raw 0 for "no infector", ambiguous with a real symptom stage 0 from getCurrentSymptomId. infection_seed.cpp already logs kNoSymptomId (255) for the equivalent seed-infection case; align resolveInfectorSymptomId's two no-infector paths (infector_id < 0, visitor not found) to the same sentinel so venue/partial-presence infections aren't misread by downstream tooling. * Shorten kCouldNotResolve to fit PersonRecord::sex[16] "could not resolve" (18 chars) truncated to "could not resol" when strncpy'd into the 16-byte sex field, silently corrupting HDF5 output for Sex::UNKNOWN. Shortened sentinel to "unresolved" (10 chars) so it fits all fixed-size buffer call sites without truncation. * Finish sentinel rollout for cached_virtual_venue_type_id Sibling field cached_encounter_type_id was converted to kDefaultEncounterTypeId in this branch's rollout, but cached_virtual_venue_type_id was left as bare 255 despite serving the same "not yet resolved" role. Use kUnknownVenueTypeId instead. Field stays int, not uint8_t: it's compared against EncounterProposal::venue_type_id and populated from ContactMatrices::matrix_name_to_id, both int, so int is the correct type for this subsystem. * Guard uint16_t->uint8_t symptom-id narrowing at Disease load infector_symptom_id/old_symptom_id/new_symptom_id were narrowed to uint8_t for event logging and cross-rank transmission, but symptom-tag ids are assigned unbounded from YAML config with no cap. >255 tags would silently wrap the id mod 256 at every downstream call site. Throw at load time instead if symptom_tags count reaches kNoSymptomId (255, the "not applicable" sentinel). * Apply clang-format to fix CI lint failure Reflow arg lists in infectPerson, filterAndSortActiveLocations, resolvePartialPresenceInfections, buildPersonRecords to satisfy 80-col clang-format check; no semantic change. --------- Co-authored-by: gavdoubleu <gavin.a.woolman@durham.ac.uk> Co-authored-by: Martha Correa-Delval <165719743+mtcorread@users.noreply.github.com>
Proposal/Reply/PendingInfection/CoordinatedEncounter/Visitor each hand-listed their fields separately in a size constant, pack, and unpack. Add WireRecord seam (single field list -> size/pack/unpack derived); refactor all five to use it. status_byte, variable-length tails stay hand-composed around it. applyOnePendingInfection now takes const PendingInfection& instead of 8 args. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ngInfection Comment claimed a post-unpack lookup set home_array_index; no such lookup exists, it was always hardcoded to -1 (already its default, since it's excluded from kInfectionWire). Corrected comment, dropped the redundant assignment/copy.
WireRecord (f8ca6ae) derives each MPI record's wire size/pack/unpack from a single field list, but nothing checked that list still covers every wire-relevant field of its struct - a field added later and forgotten in the list would silently stop going over the wire, with no build or test failure. Add a static_assert next to each kXWire definition: - EncounterProposal/EncounterReply/PendingInfection are all-scalar, so sizeof(T) is a workable drift proxy. - CoordinatedEncounter and VisitorData end in a std::set/std::vector tail packed manually outside WireRecord, so sizeof(T) is dominated by the container's own layout instead; use offsetof(T, tail_member) up to where the WireRecord-covered header ends instead. Coarser than an exact field-by-field checksum (a same-or-smaller field landing in existing trailing padding could still slip through unnoticed), but needs no hand-maintained field list, so it does not reintroduce the triplicated-listing failure mode WireRecord was written to remove. Verified by temporarily adding an unlisted field to EncounterProposal and confirming the build breaks.
…time check CoordinatedEncounter is non-standard-layout (std::set<PersonId> member), so static_assert(offsetof(CoordinatedEncounter, participants) == 32) relied on conditionally-supported behaviour: compiles under GCC/Clang w/ a -Winvalid-offsetof warning today, but the computed offset isn't a portable guarantee across standard libraries (README lists Clang 10+ as supported, not just GCC/libstdc++). Swap it for a runtime check in test_domain_communicator_detail.cpp using well-defined pointer subtraction on a real object, which needs no standard-layout assumption. Also add a static_assert(is_standard_layout_v<VisitorData>) guard next to the still-legitimate offsetof tripwire in domain_communicator.cpp, so VisitorData gaining a similarly non-standard-layout member fails loudly at compile time instead of quietly regressing the same way.
Reflow arg/initialiser-list wrapping and static_assert indentation across wire-record and event-logging call sites; no semantic change.
gavdoubleu
force-pushed
the
wire_record_dedup
branch
from
July 15, 2026 17:45
c22d97d to
4995816
Compare
* Add conda environment.yml 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. * Guard MPI-only venue-ownership check in runSlotTransmission Non-MPI builds only forward-declare DomainManager, so the unguarded domain_mgr_->getDomain() call fails to compile. Wrap in #ifdef USE_MPI, matching every other domain_mgr_ call site in this file. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: gavdoubleu <gavin.a.woolman@durham.ac.uk> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
… rollout (IDAS-Durham#19) * Add conda environment.yml 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. * Align june_events sentinel handling w/ engine's consistent-sentinel rollout IDAS-Durham#16 narrowed infector_symptom_id uint16_t->uint8_t & made the engine write kNoSymptomId (255) directly for no-infector rows (seed/fomite/compartmental), replacing the old ambiguous 0 ("recovered"). june_events predated that fix: decode/sentinels.py only had one generic UNSET_REGISTRY_INDEX=255, and load_enriched.py worked around the ambiguity with a manual NaN-masking pass keyed on infector_id==-1 before registry decode. - decode/sentinels.py, decode/__init__.py: split UNSET_REGISTRY_INDEX into DEFAULT_ENCOUNTER_TYPE_ID and NO_SYMPTOM_ID, mirroring kDefaultEncounterTypeId and kNoSymptomId in include/core/types.h even though both are 255 - the engine no longer treats them as one concept, so neither should this module. - load_enriched.py: drop _mask_infector_symptom_for_no_infector; register infector_symptom_id's sentinel/label in REGISTRY_SENTINELS/ UNSET_LABEL_OVERRIDES instead, same path already used for encounter_type_id. - tests/test_load_enriched.py: update no-infector fixture row to uint8/255 (was uint16/0) to match the engine's actual wire format post-IDAS-Durham#16. - CONTEXT.md, PDR.md: document the two distinct 255 sentinels and flag that tests/fixtures/simulation_events_fixture.h5 still predates IDAS-Durham#16 (uint16 infector_symptom_id, 0 for no-infector) until rebuilt via build_fixture.py against a current simulation_events.h5. --------- Co-authored-by: gavdoubleu <gavin.a.woolman@durham.ac.uk> Co-authored-by: Martha Correa-Delval <165719743+mtcorread@users.noreply.github.com>
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
2 participants
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.
Why
A
/code-review mediumpass overevent_logging_consistent_sentinels(finding IDAS-Durham#4, PLAUSIBLE) flagged that the MPI wire-format code insrc/parallel/domain_communicator*.cpphand-lists each record type's fields three times: once in a*_WIRE_SIZE/magic-number constant, once inpack*, once inunpack*. A recent fix on that branch made the size constants derive from fieldsizeofinstead of hand-maintained literals, closing the risk of the size silently desyncing from field types — but the field list itself was still duplicated by hand across three call sites per record type. A field added to a struct but forgotten in one of those three places breaks wire format undetected until a rank hangs or garbles data.Two more instances of the same pattern turned up beyond the original three: the Visitor exchange in
domain_communicator.cpp(VISITOR_WIRE_HEADER = 33, an unverified magic number), andCoordinatedEncounter's finalized-encounter exchange (entry_size = 29 + participant_count * 4).What
Adds
WireRecord<T, Members...>(include/parallel/domain_communicator_detail.h), a reusable seam that derives wire size, pack, and unpack from a single field list per record type, built on the existingpackField/unpackFieldhelpers. Fields needing custom handling (theEncounterReply::statusenum transcode, and the variable-length tails on Visitor/CoordinatedEncounter) stay outsideWireRecordand are composed manually around it.Refactors all five record types onto this seam:
EncounterProposal,EncounterReply,PendingInfection,CoordinatedEncounter(finalized-encounter),Domain::VisitorData. Also changesapplyOnePendingInfectionto takeconst PendingInfection&instead of 8 positional args that mirrored the struct's fields by hand (same duplication risk, one level up).Written test-first (TDD):
tests/test_domain_communicator_detail.cppunit-testsWireRecordin isolation, nompirunneeded.Stacked on
event_logging_consistent_sentinels(IDAS-Durham#16) since this refactor depends on the sentinel/narrowing changes already on that branch (e.g.infector_symptom_idasuint8_t,kNoSymptomId). Retarget tomainonce IDAS-Durham#16 merges.Test plan
test_domain_communicator_detail(new, plain ctest, no MPI runtime)test_mpi_communication,test_mpi_advanced,test_mpi_transmission_modes,test_coordinated_encountersundermpirun -np 2— confirm byte-identical wire format post-refactorctestsuite (44/44 pass except a pre-existing, unrelated failure:test_mpi_full_reproducibilityfails on missingworld_2021.h5fixture)