Address people by an ancestor geographical unit, and date symptom policies - #25
Conversation
179ba73 to
004ccf1
Compare
Includes the stacked venue-gate (PR IDAS-Durham#24) and geo-ancestor-addressing (PR IDAS-Durham#25) work it builds on. Structured seed budgets are made globally exact and households seeded whole.
SelectionCriterion could only compare a person's own flat geo_unit_id, which in a GB world is an SGU (output-area) id. No filter could mean "this person is in Scotland", so a policy file could not represent devolved UK COVID-19 policy at all - England, Scotland and Wales ran materially different timetables from May 2020 onward. That made this a blocker for the my_june 2020 policy timeline, whose policies.yaml is already written against geo_unit.<LEVEL>. The data and the lookup already existed - WorldState::ancestorAtLevel, and the full /geography/* tree loaded on every rank by DomainLoader::loadGeography - they were simply unreachable from a criterion. geo_unit.<LEVEL> reaches them, spelt to match the level/name convention MAY's config.yaml and infection_seeds.yaml already use: the level named from the world's own geo_levels registry, the unit by human-readable name, never by raw integer id. Ids are an artefact of world-builder emission order, so a wrong-but-valid id filter fails silently. evaluate stays off the parent chain. resolve builds one byte per geographical unit - elsewhere / at a target / no ancestor here - and evaluate is a single array read. A naive per-evaluate ancestorAtLevel would cost ~6.5e9 hash probes across 65M people and 25 policies in precomputePolicyApplicability alone. The mask is keyed by geo_unit_id where ids are dense enough and by index into geo_units otherwise, at the same 4x ratio the venue type vector uses. No ancestor at the named level is absent, not missing: false whichever way the criterion is written, exactly as a Slot Venue Type with no Venue is. The one-off warning counts only inhabited units, since a unit coarser than the queried level has no ancestor there by construction and holds nobody. Failure is loud. A bad level name, a bad or ambiguous unit name, a name that only exists at another level (the message names that level, which catches "Yorkshire and the Humber" for "...The Humber"), a raw id, or an operator geography cannot answer all abort at config load. Errors are recorded at resolve and thrown at resolveOrThrow because evaluate resolves lazily under const_cast and must not throw from the hot path. The same asymmetry is closed across the eight criterion resolve sites: policy applies_to, activity exemptions, schedule selection, vaccination campaigns, preference profiles, infection-seed target groups and outcome-rates rows now all use resolveOrThrow with a context string. Previously a misspelt filter there evaluated false for everyone and the block quietly did nothing - in the same file as resolveVenueTypeMask, which throws on an unknown venue type for precisely this reason. This surfaces pre-existing dead filters in the untracked config_plague* configs, which select on bare occupation / num_travel_days / num_sailing_days rather than properties.<name> and so have never matched anyone. Left alone here: prefixing them changes which people those schedules select, which is a modelling decision. PropertyValue gains vector<std::string> and both criterion parsers fall back to it when a sequence does not parse as ints, since the consuming config needs "in" over the nine English region names. That also makes "in" work over string lists for every string-valued property. The schedule-assignment CSV path needed no change: filter.geo_unit.XLGU= Scotland already parses.
A symptom-triggered behaviour change was in force for the whole run or not at all: only TemporalPolicy carried start_time/end_time, and the Priority 1 loop ran symptom policies with no time check. Symptomatic self-isolation was a dated intervention (UK, 12 Mar 2020), not a background condition, so on config_2021's large day-0 seed six weeks of anachronistic isolation acted on a substantial infected population and changed the wave shape. - ActiveWindow value type, held by both SymptomPolicy and TemporalPolicy. TemporalPolicy::isActive replaced by window.contains, not wrapped. - Window is half-open [start, end): end_date is the first day the policy is NOT in force. Mirrors the simulation's own window, and corrects the old temporal end-date off-by-most-of-a-day (> to >=). Adjacent windows now abut with no slot where both are active. - Priority 1 loop gated on the window. Out of window clears Sticky Compliance and releases the freeze, but does not propagate follow_up_policy inheritance: a calendar edge is a change of government instruction, not symptom progression, so compliance is re-rolled against the superseding policy's own rate. - Recovery and death release the freeze. Previously a frozen traveller who recovered stayed pinned for the rest of the run, and dead people kept frozen_states_ entries into every later checkpoint. Epidemiology holds a PolicyManager pointer for it; absent, nothing is released. - resolveAll throws above 32 policies of either kind, naming the first that would never fire. The applicability masks are uint32_t and precomputePolicyApplicability silently truncated at 32, so a 33rd policy loaded, resolved and never fired. Cap not raised; expressing date ranges as separate policies multiplies policy counts, so it needs to fail loud. - Loader accepts start_date/start_time/end_date/end_time on symptom policies, with the precedence temporal policies already used. tests/test_policy.cpp 27 -> 37 cases, 233 -> 283 assertions. The rename of start_time/end_time to window.* in the other three test files is mechanical: every 10.0 in the suite is a window bound, never an evaluation time.
Puts the new windows to work on the config that motivated them, and moves the run onto the calendar its policies describe. - Run is now 2020-02-01 to 2020-08-01 (end exclusive, so through 31 Jul), the period the COVID-19 UK timeline covers. 12 Mar 2020 is day 40, 16 Mar is day 44. The infection seed date and the checkpoint on_dates entry follow the same calendar; both were still on 2024 dates and the checkpoint would otherwise never have fired. - isolate_when_mild / isolate_when_severe windowed [2020-03-12, 2020-03-16) at 0.4 / 0.8: the "contain" to "delay" switch, when anyone with a new continuous cough or fever was told to self-isolate. Nothing before that date, which is what the timeline says happened. - Household isolation from 2020-03-16 as its own pair at 0.7 / 0.9, with its own mild -> severe follow-up chain declared inside its window, so nothing inherits across the 16 Mar edge. A SymptomPolicy acts on the person carrying the symptom, so well household members are not confined; what the firmer instruction buys here is a higher compliance rate. Those rates are placeholders and want fitting before a run is read. - hospitalize_severe_cases left undated: clinical care, not an intervention of the period. - test_mpi_full_reproducibility.sh derives its end date from the config's own start_date instead of a hardcoded 2024-01-01, which the calendar move would have broken. - analysis_tools/symptom_policy_window_check.py demonstrates the boundary from simulation_events.h5. The engine records no policy-relocation event, so the observable signature of a symptom policy is the infections it prevents: two 50-day runs differing only in their symptom policies have identical infection streams on days 0-39 and first differ on day 40 (15584 infections against 17101).
`geo_unit.<LEVEL>` built its tri-state mask under one of two keyings: by `geo_unit_id` when ids are dense, by position in `world.geo_units` when sparse. The sparse keying forced `evaluate` to translate id -> index via `world->geo_unit_index`, so it needed a live `WorldState*`; lacking one it returned false. The `world` argument defaults to nullptr across the config API, and ActivityManager reaches `getScheduleTypeForPerson` with no world at all. So a schedule type filtered on `geo_unit.XLGU` matched nobody — silently, no warning, plausible-looking output — whenever the world file happened to have a sparse geo-unit id space. Same config, same data, different simulation, decided by an incidental property of the world. Keep both keyings, but make the sparse one carry its own sorted table of unit ids and binary-search it, so neither form asks for the world back. Both now go through geoMaskSlot(). The dense path is still the single array read it was designed to be; the search cost lands only on sparse worlds. `state == 2` (no ancestor at this level, failing both == and !=) is unchanged: it is a real answer, unlike the missing-world bail-out that was masquerading as it. Tests cover dense and sparse ids evaluated with no world passed.
08bde0c gave `PropertyValue` a `vector<string>` alternative for `geo_unit.<LEVEL>`, and both YAML parse sites reached it through an unconditional `catch (...)`: a sequence `value:` that failed int conversion became a list of strings, whatever the property was. Only GEO_ANCESTOR consumes `vector<string>`. The `in` operator handles `vector<int32_t>` against an int and nothing else; `==`/`!=` compare variant alternatives, so a string list never equals a string person property. A criterion that used to throw at load now loads clean and selects zero people: - property: properties.region operator: in value: ["North East", "Yorkshire"] # loaded fine, matched nobody - property: age operator: in value: [1.5, 2.5] # float list, same outcome `resolveOrThrow` does not catch either: its checks are about the property path and the operator, never about whether the parsed value's alternative is one the property can be compared against. So a typo in a `value:` list stopped being a load-time failure and became a policy, schedule type or infection seed that quietly applied to nobody, in a run that otherwise looked plausible. Restores the pre-branch contract, and no more: a list is whole numbers, except for `geo_unit.<LEVEL>` where it is unit names. Anything else throws at parse time, naming the property. - `SelectionCriterion::comparesAgainstUnitNames` is the lexical predicate, declared beside `resolve` and used by its own GEO_ANCESTOR arm, so the loaders can ask before a world exists and the two cannot drift. - `loaders/selection_criterion_value.h` holds the sequence parse both loaders now share. The two sites were verbatim copies and the mission this fixes turned on them staying in agreement. - Parse time rather than `resolveOrThrow`: the check needs no world, and it fires for every criterion rather than only those whose call site remembered to resolve loudly. Scalar parsing is untouched at both sites, including the divergence between them (policy_loader maps "true"/"false" to 1/0, config_loader does not). Nothing else changes. `in` is still implemented only for an int list against an int person value, so it remains silently empty on age and on every string-valued property — pre-existing, present on main, and separate.
The geo_unit.<LEVEL> resolver warns when a unit has no ancestor at the queried level, since people there match neither == nor !=. Its guard "only warn about units people live in" used people_by_geo_unit, which keys people under every ancestor unit too, so every unit coarser than the queried level tripped it — the exact case the guard excluded. On a GB world it fired for any criterion below the top level, i.e. the normal case, training users to ignore the one signal that catches a genuinely mis-specified geography. buildIndices now also records directly_inhabited_geo_units, the set of Person::geo_unit_id values, at no extra cost in the same loop. people_by_geo_unit is unchanged; its ancestor-inclusive contents are what make ancestor-keyed lookups work. Level-index comparison was rejected as a guard: level_id ordering is not consistently coarse-to-fine across worlds. Also gate the warning on rank 0. geo_units is global on every rank, so one rank's report flags the geography; previously stderr grew with rank count. Reaching logRank0 from core meant moving it out of config_detail into utils/mpi_logging, avoiding a core->loaders include.
The geo-ancestor mask and dated symptom policies were written against clang-format 22; CI pins 20. No behavioural change.
Main dropped every docs/adr/00xx cross-ref from comments (53a0ac6, reviewer request on PR IDAS-Durham#24), but this branch's new symptom-window test was written before that and carried one in. docs/adr/ no longer exists on main, so the pointer is dangling; the reasoning it cited is already spelt out in the same comment. Comment-only, no behaviour touched.
004ccf1 to
69d52ed
Compare
|
Added commit b45e8e2 to fix the mode-only virtual encounter regression introduced by the contact-matrix refactor. The regression test covers a virtual matrix defined only under mode_matrices and verifies that proposals are accepted. This does not address the separate clustered-seeding behaviour discussed in PR-24: household scoring still favours larger residences, and members are still considered in stored membership order. That needs a separate change and tests. |
|
@mtcorread ready for review now (was draft, now marked ready). All CI checks pass, clean fast-forward off main. |
The earlier fix registered the mode names (respiratory, physical_contact) in matrix_name_to_id, but an encounter's virtual_contact_matrix names the outer key of mode_matrices[name][mode], the same kind of name as a flat matrix. An encounter whose matrix exists only per mode therefore still missed the lookup and kept whatever id it already had. Two such encounter types would share the unknown id, and every proposal would be matched to the first of them. Register the outer names instead, and throw when a virtual encounter names no matrix at all rather than keeping the old id. The regression test now clears the id the world builder left behind before resolving, so it can only pass by resolving the per-mode matrix, and a second test covers the error.
ActiveWindow used end_time = -1.0 to mean "no end". An end_date exactly one day before the simulation start converts to day -1 too, so a policy that had already ended before the run was treated as never ending and stayed in force for the whole simulation. Store the end as std::optional<double>, so an absent end cannot collide with any real day. While here, stop the loader choosing silently between two ways of stating the same bound: declaring both start_date and start_time, or both end_date and end_time, now throws instead of letting the date win. A window that ends at or before its explicitly stated start also throws, since nobody can ever be in it. An end with no stated start is still accepted when it falls before the run: that policy is simply never in force.
Several filters were only resolved lazily on first evaluation, where anything the world cannot answer (an unknown property path, a person property the world does not carry, a misspelt geo level) quietly evaluates false and reads as "nobody qualifies" instead of as a config error: - infection seed attribute filters (resolved, but without the checks); - schedule-assignment CSV rows; - frequency-group CSV rows; - calendar-event attendee filters; - disease outcome-rate rows: OutcomeRates::resolve already called resolveOrThrow, but nothing ever called Disease::resolve. All of them now go through resolveOrThrow when the world is available. Outcome tables are reference data: a national table can run on a regional world, so a row may name a geographical unit the world does not contain. For those tables only, a geo_unit.<LEVEL> name that exists at no level is recorded rather than refused, matches nobody, and is reported on rank 0 at load. A name that exists at a different level is still an error, since that is a misspelt level rather than a missing place.
|
Gavin, I've pushed review fixes onto this branch. One thing needs you before it can merge. Blocking: config_2021 now starts on a Saturday. What I pushed:
Also merged main (#28) into the branch. Local ctest passes, and a 10-day config_2021 run exercising the new policy and outcome-table paths is identical at np 1 and np 2. |
start_date 2020-02-01 is a Saturday, but day_type_cycle was Mon-Fri/Sat-Sun positional from sim_day 0. getDayTypeIndex indexes by sim_day % 7, so the whole run was rotated five days: 1 Feb ran the weekday schedule, 6-7 Feb ran weekend, etc, silently, for the entire run. Rotate the cycle to [weekend, weekend, weekday x5] instead of shifting start_date, since start_date and the day-40/44 comments in simulation.yaml are pinned to the real COVID-19 UK timeline.
|
Fixed in 0118edf: rotated day_type_cycle in config_2021/schedules.yaml to [weekend, weekend, weekday x5] so index 0 lands on Saturday (start_date's actual weekday). Kept start_date at 2020-02-01 since the day-40/44 comments in simulation.yaml are pinned to the real COVID-19 UK timeline. Pushed to the branch. |
This brings in IDAS-Durham#25 and IDAS-Durham#29 from main, and git found no conflicts. Where the two sides touch, the reproducibility harness reads its start date from the config, as IDAS-Durham#25 made it, and keeps this branch's empty-seed failure and bash 3.2 fix. Seed attribute filters go through IDAS-Durham#25's load-time checks. config_2021 now starts on 1 February 2020, so the structured seeds dated 1 to 3 February fall inside the run, and test_mpi_structured_seed_reproducibility seeds people instead of comparing empty files.
Lets a criterion address people by an ancestor geographical unit, and gives symptom policies a date range.
What
Geo-ancestor addressing — a selection criterion can name a geographical unit above the person's own (e.g. select by region when people are indexed by area). Adds the ancestor mask to config, wired through the config loader, policy loader and infection seeds.
Symptom policy date ranges — symptom policies now take start/end dates rather than applying for the whole run.
config_2021's self-isolation policies are dated accordingly, withanalysis_tools/symptom_policy_window_check.pyto check the windows.Criterion validation — a criterion list value the property cannot read is now rejected at load rather than silently matching nothing; the geo-ancestor mask is made self-contained; config warnings fire only for units people actually inhabit, so a config no longer warns about unpopulated ones.
Testing
Builds clean,
ctest -E mpi_full_reproducibility46/46 pass. Newtest_selection_criterion_geography.cpp;test_policy.cppextended for date windows.