Skip to content

Address people by an ancestor geographical unit, and date symptom policies - #25

Merged
mtcorread merged 14 commits into
IDAS-Durham:mainfrom
gavdoubleu:feat/geo-ancestor-addressing
Sep 13, 2026
Merged

Address people by an ancestor geographical unit, and date symptom policies#25
mtcorread merged 14 commits into
IDAS-Durham:mainfrom
gavdoubleu:feat/geo-ancestor-addressing

Conversation

@gavdoubleu

Copy link
Copy Markdown
Contributor

Blocked by #24 — merge that first. Draft until then. This branch contains #24's commits, so until #24 lands the diff shown here includes them; the last 6 commits are this PR's own work. I rebase and force-push after #24 merges, then mark ready.

Series order: #24this#26.

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, with analysis_tools/symptom_policy_window_check.py to 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_reproducibility 46/46 pass. New test_selection_criterion_geography.cpp; test_policy.cpp extended for date windows.

@gavdoubleu
gavdoubleu force-pushed the feat/geo-ancestor-addressing branch from 179ba73 to 004ccf1 Compare August 21, 2026 14:46
gavdoubleu pushed a commit to gavdoubleu/JUNE2 that referenced this pull request Aug 24, 2026
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.
gavdoubleu added 8 commits August 28, 2026 21:02
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.
@gavdoubleu
gavdoubleu force-pushed the feat/geo-ancestor-addressing branch from 004ccf1 to 69d52ed Compare August 28, 2026 20:13
@mtcorread

Copy link
Copy Markdown
Collaborator

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.

@gavdoubleu
gavdoubleu marked this pull request as ready for review September 10, 2026 15:25
@gavdoubleu

Copy link
Copy Markdown
Contributor Author

@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.
@mtcorread

Copy link
Copy Markdown
Collaborator

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. start_date is 2020-02-01, which was a Saturday. day_type_cycle in schedules.yaml is positional from Monday (getDayTypeIndex is sim_day % 7), so every week in config_2021 is rotated five days: 1 Feb runs the weekday schedule, 6 and 7 Feb run the weekend one, and so on for the whole run. Nothing errors; the timeline is just quietly wrong. Your call how to fix it, either rotate the cycle to start [weekend, weekend, weekday, ...] or start on Monday 2020-02-03, but this PR doesn't merge until it's fixed.

What I pushed:

  1. Mode-only virtual matrices (corrects my own b45e8e2). That commit registered the inner mode names in matrix_name_to_id, but virtual_contact_matrix names the outer key of mode_matrices[name][mode], so a mode-only matrix still missed the lookup. Its test only passed because an id from an earlier resolve was left behind. The outer names are registered now, an unknown virtual_contact_matrix throws, and the test clears the stale id first.
  2. Policy window end. end_time = -1 meant "no end", but an end_date exactly one day before the start is also day -1, so a policy that had already ended stayed on for the whole run. The end is now std::optional<double>. Declaring both start_date and start_time (or both end keys) now throws instead of the date winning silently, and so does a window that ends at or before its stated start.
  3. Filters checked at load. Seed attribute filters, schedule-CSV rows, frequency-group rows and calendar attendee filters were only resolved lazily, where an unknown property or level silently matches nobody. resolveOrThrow also existed for outcome-rate rows, but nothing ever called Disease::resolve. All of these are now checked when the world loads. Outcome tables are reference data, so a geo_unit.<LEVEL> value naming a place this world doesn't have (a national table on a regional world) is reported on rank 0 at load and matches nobody. A name that exists at a different level still throws.

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.
@gavdoubleu

Copy link
Copy Markdown
Contributor Author

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.

@mtcorread
mtcorread merged commit 15d4374 into IDAS-Durham:main Sep 13, 2026
5 checks passed
mtcorread added a commit to gavdoubleu/JUNE2 that referenced this pull request Sep 13, 2026
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.
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.

2 participants