Skip to content

fix(Uganda): the member/visitor coin flip, and a cluster key that is one (GH #323, config-only)#634

Open
ligon wants to merge 2 commits into
developmentfrom
fix/323-uganda-config
Open

fix(Uganda): the member/visitor coin flip, and a cluster key that is one (GH #323, config-only)#634
ligon wants to merge 2 commits into
developmentfrom
fix/323-uganda-config

Conversation

@ligon

@ligon ligon commented Jul 21, 2026

Copy link
Copy Markdown
Owner

Uganda's half of GH #323, config-only. Refs #323.

Scope is set by D1 of slurm_logs/DESIGN_grain_collapse_sites_2026-07-13.orgcore does not aggregate. No file under lsms_library/*.py is touched, and no aggregation: key is added (dead config, and it contradicts the policy it was meant to serve). The rescued WIP snapshot's +155 lines of country.py, +60 of local_tools.py and its new where: primitive are all dropped; what they were for is delivered by machinery that has since landed centrally (reduce_to_agreed, PR #618) or by the df_edit hook that already existed.


1. people_last7days (2018-19, 2019-20) — a coin flip, and it lost

From 2018-19 GSEC15A went LONG: two rows per hhid keyed by CEA01 ∈ {Household members, Visitors}, with CEA01A-D the counts for the selected category. CEA01 sat commented out in the YAML, so both rows collided on the declared (i, t) and groupby().first() kept whichever the file listed first.

2018-19 2019-20
rows destroyed 3,147 → 0 3,038 → 0
served VISITOR counts 1,636 / 3,242 (50.5%) → 0 1,507 / 3,078 (49.0%) → 0
zero-people households 1,053 → 64 1,011 → 16
mean people / household 2.81 → 4.51 2.77 → 4.53

(at source; through the API after the panel-id walk the means are 4.60 / 4.55 and the zero-people households are 3 / 4. Every earlier wave reads 5.0–5.4.)

CEA01 is now extracted as the temporary myvar _category and the member rows selected by the country hook uganda.people_last7days(), which makes (i, t) genuinely unique so the collapse never fires. Members-only is not a judgement call: the three earlier waves that declare this table map Men/Women/Boys/Girls onto the source's member block and ignore the parallel visitor block — confirmed against the Stata variable labels (h14q1 = "male adults hh members" vs h14q5 = "male adults visitors"). The hook raises if a future long-form wave forgets _category.

2. cluster_features (all 8 waves)

Every wave reads GSEC1, the household cover page, so the frame is household grain while the table is declared (t, v). The reduction is intended (GH #161); it was undeclared, so it fell through to groupby().first() — which skips NA per column and can therefore return a composite row existing in no source record. Three real defects rode on it.

(a) v was not a cluster key in 2018-19 / 2019-20. parish_code is broken (19 distinct values for 751 parishes) so v was the free-text parish name — and 20 / 23 parish names recur across districts as coded, CENTRAL in ten of them. v is now the DISTRICT/PARISH composite, declared identically in sample and cluster_features so the two cannot drift. Clusters spanning >1 district: 20 → 0 and 23 → 0. Deliberately not keyed finer — adding subcounty splits 10 more groups, and the ones inspected are spelling artefacts of one subcounty (NYENGA / NYENGA DIVISION, centroids 0.0 km apart).

"Spanning >1 district: 23 → 0" is zero by construction and is NOT evidence. It checks that the key was applied; it cannot see the opposite error. The review found it: the district component over-split three real 2019-20 parishes. Group GSEC1 by its own (county, subcounty, parish) triple and count distinct v — 2018-19: 0; 2019-20: 3 (NAMTUMBA/NAMUTUMBA, a misspelling occurring once in 3,098 households; MUBENDEKASSANDA, carved out 2019; GULUOMORO, carved out 2016 — each with a straggler household still coding the parent, and each pair agreeing on region). Four households sat in a 1–2 household phantom cluster instead of the 10–11 household cluster they belong to. Fixed by a three-entry _V_ALIASES table in 2019-20/_/mapping.py, keyed on the (district, parish) pair (Mubende and Gulu are still real, populated districts here — 29 / 37 hh). District is now row-valued in that wave's cluster_features and resolved through the same table, so the merge does not trade three over-splits for three <NA> districts. 827 → 824 clusters. Two further one-household district misspellings (PALISA, RUKINGIRI) are deliberately left alone — the correct spelling holds no household in the affected parish, so they split nothing.

(b) 2009-10's 565 out-of-frame households had no cluster at all. comm is the 2005-06 EA, so movers and split-offs carry a blank one and were deleted outright by groupby(dropna=True) — 339 (t, v) pairs sample() knew about had no cluster_features row. Its YAML now reaches the same fill_v_with_coord_bin transformer sample already uses, declaratively via the existing derived: block (v moved to a myvar with final_index: [t, i], because derived: runs after set_index). Same coordinates, same defaults, so the labels agree by construction rather than by coincidence. 541 recovered; the 24 with neither comm nor GPS are dropped, with a count.

(c) the projection is now explicit. uganda.cluster_features() hands it to reduce_to_agreed (PR #618): agree → the value, silently; NaN-vs-value → absence, not contradiction, so the observed value survives; genuine disagreement → <NA> plus a GrainConflictWarning naming the clusters.

Consumers will see <NA> where they previously saw a value, and in the two comm panel waves it is a third of a column, not a handful of cells: District is <NA> for 27.8% of 2010-11 and 33.0% of 2011-12 clusters, Latitude/Longitude for 41.2% / 49.8%. That is the point — the old value was an arbitrary pick, or a composite, among disagreeing households. What remains is two properties of the survey: panel dispersal (comm is the EA of origin, so a mover's district differs from its EA-mates' — 93 clusters in 2010-11, 106 in 2011-12) and genuine sub-parish variation (a parish holds several EAs, urban and rural — 70 / 65 / 122 / 122 clusters conflict on Rural in 2013-14 / 2015-16 / 2018-19 / 2019-20). Per-household Rural/Region remain exact in sample().

GPS is not averaged. Where within-cluster coordinates vary they vary by a median of 4.6–42.9 km and up to 584 km — a broken key, not a scatter about a centroid. Same argument that retired core's .mean() on 2026-07-13. (The rescue snapshot proposed median; the measurement says no.)

Measured, cold build (LSMS_NO_CACHE=1)

wave destroyed NaN-key deleted clusters
2005-06 2,551 → 0 368 → 368 (now loud) 322 → 322
2009-10 571 → 0 565 → 24 (loud) 317 → 656
2010-11 1,392 → 0 0 324 → 320
2011-12 1,898 → 0 0 321 → 321
2013-14 1,012 → 0 1 → 1 (loud) 706 → 706
2015-16 1,014 → 0 0 875 → 875
2018-19 1,452 → 0 0 751 → 781
2019-20 947 → 0 (Site 2) 1 → 1 (loud) 793 → 824
total 10,837 → 0 935 → 394 4,409 → 4,805

The surviving NaN-key rows are still deleted — decision D2: Site 3 deletes and reports, it does not retain. What changed is that they are deleted out loud, with a count, at the place that knows why.

Four 2010-11 clusters vanish entirely: their households agree on nothing, so every cell blanks and the framework's dropna(how='all') removes the empty row. 42 households reference a v with no cluster_features row. That is the honest outcome — comm is not identifying a place there — and a fabricated composite used to fill the gap.

3. The invariant that had to hold

sample.v and cluster_features.v must be the same key or _join_v_from_sample matches nothing. Verified per wave after the change: 0 cluster_features (t, v) pairs unknown to sample, and 0 orphaned sample clusters except the 4 above.

4. Deferred (deliberately)

5. Tests

  • tests/test_uganda_323_grain.py27 tests, all config-level; exercises no core aggregation. Salvaged from the rescue snapshot minus the parts that tested the stripped core machinery, plus hook-contract tests (lossless collapse is silent; NaN is absence; disagreement is <NA> and loud; a missing v raises), plus the two over-split tests added after review.
  • Nothing in the module turns a build failure into a skip any more. The original _uganda_or_skip / built fixtures wrapped everything in except Exception: pytest.skip(...), which would have made the end-to-end half vacuous the moment the build broke. Replaced by tests/conftest.py's aws_creds_available() guard on the uga fixture alone: the data-free CI job now runs 8 tests and skips 19.
  • Negative control for the new fix: emptying _V_ALIASES in place (git stash does nothing to a change already committed on the branch) fails test_v_does_not_over_split_one_real_parish[2019-20] and test_the_2019_20_alias_table_is_the_thing_being_tested. The [2018-19] parametrisation passes either way, and its docstring says so.
  • 323 guard suite: 76 passed. test_uganda_invariance: the three refreshed entries plus household_roster (untouched by the re-keying) pass.
  • tests/fixtures/uganda_baseline.json refreshed for exactly three entries (var/cluster_features — now 4,809 rows, var/sample, var/people_last7days) by a surgical update that would have reported — and refused — any unrelated drift. Only 2 of the 53 baseline entries can carry v at all, because cached parquets are written before _join_v_from_sample.

Withdrawn (was false). An earlier version of this body and of .coder/ledger/323-uganda.md claimed tests/test_gh323_explicit_reducers.py::test_core_does_not_dispatch_the_reducers was a pre-existing failure on pristine origin/development. It is not — 22 passed at 45aee170 (this branch's base) and at 4c236d11 (current development); PRs #635/#640 replaced the substring guard with an AST one. The ledger also said the baseline fixture was refreshed for "exactly two entries"; it was three. Both corrected in the tracked files.

Recorded, not fixed (scope). 32 of 2019-20's 824 clusters span more than one county/subcounty spelling. Almost all are the data-entry artefacts the key deliberately ignores, but KAKUMIRO/KIKWAYA (8 hh in BUGANGAIZI/KIKWAYA, 1 in BUYANJA/KAKINDO) is a genuine second parish of the same name inside one district — untouched here, and unquantified for the other waves. Noted in CONTENTS.org and the ledger so the silence is not read as a clean bill.

Diagnosis preserved in Uganda/_/CONTENTS.org and .coder/ledger/323-uganda.md.

🤖 Generated with Claude Code

…one (GH #323, config-only)

Config-only per D1 of slurm_logs/DESIGN_grain_collapse_sites_2026-07-13.org --
core does not aggregate.  No file under lsms_library/*.py is touched, and no
`aggregation:` key is added (dead config, and it contradicts the policy it was
meant to serve).  The rescued WIP snapshot's +155 lines of country.py, +60 of
local_tools.py and its `where:` primitive are all DROPPED; what those patches
were for is delivered by machinery that has since landed centrally
(reduce_to_agreed, PR #618) or by the df_edit hook that already existed.

people_last7days (2018-19, 2019-20) -- 6,185 destroyed -> 0

  From 2018-19 GSEC15A went LONG: two rows per hhid keyed by CEA01 in
  {'Household members', 'Visitors'}, CEA01A-D being the counts "for selected
  category".  CEA01 sat COMMENTED OUT in the YAML, so both rows collided on the
  declared (i, t) and groupby().first() kept whichever the file listed first.

  That is a coin flip, and it lost.  1,636 of 3,242 households (50.5%) in
  2018-19 and 1,507 of 3,078 (49.0%) in 2019-20 list Visitors first, so those
  households were served VISITOR counts; 1,053 and 1,011 came back with ZERO
  people; mean people/HH read 2.81 / 2.77 against the 5.0-5.4 of every earlier
  wave.

  CEA01 is now extracted as the temporary myvar `_category` and the member rows
  selected by the country hook uganda.people_last7days(), which makes (i, t)
  genuinely unique so the collapse never fires.  Members-only is not a
  judgement call: the three earlier waves that declare this table all map
  Men/Women/Boys/Girls onto the source's member block and ignore the parallel
  visitor block -- confirmed against the Stata variable labels (h14q1 "male
  adults hh members" vs h14q5 "male adults visitors", etc.).

  Mean people/HH 2.81 -> 4.51 (2018-19), 2.77 -> 4.53 (2019-20) at source.
  The hook RAISES if a future long-form wave forgets `_category`, rather than
  silently returning visitor counts for half its households.

cluster_features (all 8 waves) -- 10,837 destroyed -> 0; 935 NaN-key -> 394, loud

  Every wave reads GSEC1, the household COVER PAGE, so the frame is household
  grain while the table is declared (t, v).  The reduction is intended (GH #161);
  it was UNDECLARED, so it fell through to groupby().first() -- which skips NA
  per column and can therefore return a COMPOSITE row that exists in no source
  record.  Three real defects rode on it:

  * v was not a cluster key in 2018-19 / 2019-20.  parish_code is broken (19
    distinct values for 751 parishes) so v was the free-text parish NAME -- and
    20 / 23 parish names recur across districts, CENTRAL in TEN of them.  v is
    now the DISTRICT/PARISH composite, declared identically in `sample` and
    `cluster_features` so the two cannot drift.  Clusters spanning >1 district:
    20 -> 0 and 23 -> 0.  Deliberately not keyed finer: adding subcounty splits
    10 more groups, and the ones inspected are spelling artefacts of a single
    subcounty (NYENGA / "NYENGA DIVISION", centroids 0.0 km apart).

  * 2009-10's 565 out-of-frame households (movers, split-offs; blank comm,
    because comm is the 2005-06 EA) were DELETED outright by groupby(dropna=True),
    so 339 (t, v) pairs sample() knew about had no cluster_features row at all.
    Its YAML now reaches the SAME fill_v_with_coord_bin transformer sample
    already uses, declaratively via the existing `derived:` block (v moved to a
    myvar with final_index [t, i], because `derived:` runs after set_index).
    Same coordinates, same defaults, so the labels agree by construction.
    541 recovered; the 24 with neither comm nor GPS are dropped, with a count.

  * the projection is now performed by uganda.cluster_features(), which hands it
    to reduce_to_agreed (PR #618): agree -> the value, silently; NaN vs value ->
    absence, not contradiction, so the observed value survives; genuine
    disagreement -> <NA> plus a GrainConflictWarning naming the clusters.

  Consumers will see <NA> where they previously saw a value.  That is the point:
  the old value was an arbitrary pick among disagreeing households.  What
  remains is two properties of the SURVEY -- panel dispersal (comm is the EA of
  ORIGIN, so a mover's district differs from its EA-mates': 93 clusters in
  2010-11, 106 in 2011-12) and genuine sub-parish variation (a parish holds
  several EAs, urban and rural: 65-126 clusters per wave for Rural).

  GPS is NOT averaged.  Where within-cluster coordinates vary they vary by a
  MEDIAN of 4.6-42.9 km and up to 584 km -- a broken key, not a scatter about a
  centroid.  Same argument that retired core's .mean() on 2026-07-13.

  Four 2010-11 clusters vanish: their households agree on NOTHING, so every
  cell blanks and the framework's dropna(how='all') removes the empty row.
  42 households reference a v with no cluster_features row.  Honest -- comm is
  not identifying a place there -- and a fabricated composite used to fill it.

Measured cold (LSMS_NO_CACHE=1), per wave:

  wave      destroyed ->  NaN-key ->        clusters
  2005-06     2,551 -> 0   368 -> 368 loud   322 -> 322
  2009-10       571 -> 0   565 ->  24 loud   317 -> 656
  2010-11     1,392 -> 0     0                324 -> 320
  2011-12     1,898 -> 0     0                321 -> 321
  2013-14     1,012 -> 0     1 ->   1 loud   706 -> 706
  2015-16     1,014 -> 0     0                875 -> 875
  2018-19     1,452 -> 0     0                751 -> 781
  2019-20       947 -> 0     1 ->   1 loud   793 -> 827   (Site 2)
  total      10,837 -> 0   935 -> 394       4,409 -> 4,808

  The surviving NaN-key rows are still DELETED -- decision D2, Site 3 deletes
  and reports.  What changed is that they are deleted OUT LOUD, with a count,
  at the place that knows why.

sample.v == cluster_features.v verified for every wave: 0 cluster_features
(t, v) pairs unknown to sample; 0 orphaned sample clusters except the 4 above.

Tests: tests/test_uganda_323_grain.py (24, all config-level, exercises no core
aggregation).  tests/fixtures/uganda_baseline.json refreshed for exactly three
entries (var/cluster_features, var/sample, var/people_last7days) by a surgical
update that would have reported -- and refused -- any unrelated drift.
Full Uganda slice: 42 passed, 72 skipped.

Pre-existing failure, NOT caused or fixed here (core is out of scope):
tests/test_gh323_explicit_reducers.py::test_core_does_not_dispatch_the_reducers
fails on pristine origin/development, because PR #617's private
country._collapse_to_cluster_grain contains PR #618's banned substring.

Diagnosis preserved in Uganda/_/CONTENTS.org and .coder/ledger/323-uganda.md.

Refs #323.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@ligon
ligon force-pushed the fix/323-uganda-config branch from b908a32 to a5bd644 Compare July 22, 2026 02:52
@ligon

ligon commented Jul 22, 2026

Copy link
Copy Markdown
Owner Author

Adversarial review — PR #634 (Uganda, GH #323)

Verdict: FIX_FIRST. One real defect, small but of exactly the class this PR
claims to have eliminated. Everything else I attacked held: every headline
number in the PR body reproduces cold and independently
, the fixture hashes
reproduce byte-exactly, and the test file is a strong negative control.

All measurements below are cold builds (LSMS_DATA_DIR = fresh isolated dir,
dvc-cache symlinked; LSMS_COUNTRIES_ROOT + PYTHONPATH on the worktree),
PR head a5bd6443 vs. base origin/development 45aee170.


1. HIGH-value finding — the composite v splits 3 REAL clusters in 2019-20

The PR's evidence for the new key is "clusters spanning >1 district: 23 → 0".
That number is zero by construction under a key that contains the district —
it cannot detect the opposite error, over-splitting. The PR ran the
spelling-artefact audit on the subcounty component it rejected, but not on
the district component it adopted. Running it finds three cases where two
composite vs are one place:

parish district groups county subcounty why it is one place
NAWANSAGWA NAMTUMBA (1 hh) / NAMUTUMBA (10 hh) BUSIKI (both) NAMUTUMBA (both) plain misspelling — NAMTUMBA occurs once in 3,078 households
KIJUNA MUBENDE (1 hh) / KASSANDA (9 hh) KASSANDA (both) KITUMBI (both) Kassanda District was carved out of Mubende in 2019; one HH still codes the parent
PALWO GULU (2 hh) / OMORO (9 hh) OMORO (both) BOBI (both) Omoro District was carved out of Gulu in 2016; two HHs still code the parent

So 2019-20's 828 composite keys describe 825 places; ~4 households are put
into a 1–2 household phantom cluster instead of the 10–11 household cluster they
belong to. sample and cluster_features stay mutually consistent (both use the
same formatter), so nothing orphans — the damage is a fragmented cluster grain,
not a broken join.

Same audit on 2018-19: clean. All 20 splits there are genuinely distinct
parishes — minimum centroid separation across the two district groups is
13.1 km (BUKASA, Kampala vs. Wakiso), median 98 km, max 441 km (CENTRAL).
The 2018-19 half of this change is sound.

This directly contradicts the standard the PR sets for itself when it rejects
subcounty_name: "Keying on it would fragment real parishes on data-entry
noise — a new bug traded for the old one."
The same is true, at smaller scale,
of the district it did adopt. Note also the wave carries three district
misspellings in total (NAMTUMBA/NAMUTUMBA, PALISA/PALLISA,
RUKINGIRI/RUKUNGIRI), each appearing exactly once; only the first lands in a
multi-household parish.

Cheapest fix: a small alias map in 2019-20/_/mapping.py::v (three entries, or
two if you only normalise the misspelling and leave the district-split lag), or
qualify by s1aq02a (county) instead of district where the two disagree — and
a test that asserts no two composite vs share a (county, subcounty, parish)
triple. Whatever the choice, it should be written down, because the metric
currently offered as evidence structurally cannot catch it.

I did not find whitespace/case artefacts: after .strip().upper() the
district and parish vocabularies are already collapsed (119/751 in 2018-19,
123/794 in 2019-20 — identical under re.sub(r'\s+',' ')), so no doubled-space
analogue of the KAGADI TOWN COUNCIL case exists in the two key components.


2. What I tried to break and could not

Every pre- and post-fix number in the PR body reproduces exactly. Cold build
on base gives grain_reports('Uganda'):

cluster_features destroyed: 2551, 571, 1392, 1898, 1012, 1014, 1452, 947  → 10,837  ✓
cluster_features nan_key  :  368, 565,    0,    0,    1,    0,    0,   1  →    935  ✓
clusters (base)           :  322, 317,  324,  321,  706,  875,  751, 793  →  4,409  ✓
people_last7days destroyed:                          3,147 / 3,038                  ✓
sample (t,v) with no cluster_features row:            339                            ✓

and on the PR head: grain_reports('Uganda') is [], clusters
322, 656, 320, 321, 706, 875, 781, 8274,808 ✓, orphaned sample clusters
4, all 2010-11 ✓, cluster_features (t,v) unknown to sample: 0 ✓.

people_last7days, re-derived straight from the .dta, bypassing the library:
6,484 rows / 3,242 households (2018-19); Visitors listed first for 1,636
(50.5%) / 1,507 (49.0%); first-listed mean 2.806 / 2.767; member-row mean 4.512 /
4.534; all-zero member rows 64 / 16. Through the API: 4.599 / 4.552, zero-person
3 / 4. Every figure in the PR table. Members-only is corroborated: the Stata
labels are unambiguous (h14q1 "male adults hh members" / h14q5 "male adults
visitors"; h15a1-4 vs h15a5-8; T6FQ01a-d vs T6FQ01e-h) and all three
earlier waves' YAML maps Men/Women/Boys/Girls onto columns 1–4.

"GPS is not averaged" — I checked the measurement, not the prose. Rebuilt
2005-06's household-grain frame from GSEC1 + the 2009 geovar myself: exactly
70 clusters have >1 coordinate (matching the emitted warning), max
within-cluster separation median 17.2 km, p25 10.7 km, max 542.9 km, only 1%
under 1 km
. This is not a scatter about a centroid; blanking rather than
averaging is right, and the claim is not inflated.

2009-10 recovery: GSEC1 has 2,975 rows, 565 blank comm, of which
541 have a geovar fix and 24 do not — exactly as claimed. The
coalesce_coord_bin labels agree with sample's set-for-set (656 = 656, 0
in either direction), i.e. by construction, not coincidence.

Fixture honesty. I recomputed _fingerprint() on my own cold-built
parquets: var/cluster_features [4812,5] hash 0d7424ed…, var/sample
[24362,6] hash a29dca76…, var/people_last7days [15471,4] hash
44455b3c…all three identical to the committed fixture. The 4812-vs-4808
gap is the framework's post-read dropna(how='all') on the four blanked 2010-11
clusters, exactly as documented. No other baseline entry carries v (checked all
53), so "exactly three entries" is structurally right.

Test discrimination (the negative control). Copied tests/test_uganda_323_grain.py
onto pristine origin/development and ran it: 15 failed, 6 errors, 3 passed
of 24. The 3 survivors are the two index-uniqueness assertions (vacuous after the
collapse — the author says so in the docstrings and adds the pre-collapse
variants that do fail) plus the cluster_features ⊆ sample direction, which
held before too. I also checked the test_no_grain_collapse_report pair on a
warm base cache in case it was vacuous there — it still fails, because the
grain report is replayed from the parquet stamp. Good.

Conservation / blast radius. household_roster (144,704), food_expenditures
(247,049), housing (24,210), household_characteristics (24,364) and
household_characteristics(market='Region') (24,297, 0 NaN m) are row-for-row
identical between base and head
, so the v re-keying moves labels, not mass,
and the new cluster_features <NA>s do not leak into the market index.

Scope / policy. No file under lsms_library/*.py outside countries/; no
aggregation: key anywhere in the config (only prose in the ledger explaining
why it is forbidden); no reducer in core; the projection is an explicit country
df_edit hook delegating to the already-landed reduce_to_agreed — D1-compliant.
git log origin/development shows no already-merged Uganda #323 work, so this is
not a duplicate. Full 323 guard suite on head: test_gh323_explicit_reducers,
test_gh323_grain_contract, test_grain_collapse*, test_uganda_v_grain_invariants
76 passed. Fully cold pytest of the Uganda grain modules — 49 passed.


3. Minor notes

  • The PR body's "pre-existing failure" claim is stale.
    tests/test_gh323_explicit_reducers.py::test_core_does_not_dispatch_the_reducers
    passes on pristine origin/development @ 45aee170 (22/22) — PR fix(#323): the D1 guard had its polarity inverted — guard choosing, not naming #640/fix(#323): the D1 guard checks coupling via AST, not a substring #635
    replaced the substring guard with an AST one. Drop that paragraph.
  • Consumer-visible <NA> is larger than the prose suggests in two waves:
    post-change cluster_features.District is <NA> for 27.8% of 2010-11 and
    33.0% of 2011-12 clusters, Latitude/Longitude for 41%/50%. This is
    the documented and, I think, correct policy — but it is a third of a column,
    not a handful of cells, and the CONTENTS.org note would be more useful with the
    percentages in it.
  • 2019-20 district/s1aq04a has exactly 1 blank household, which is the
    "1 → 1 (loud)" row of the table. Confirmed.

…raw two false claims

Adversarial review of PR #634 (FIX_FIRST).  One real defect, plus tracked text
that asserted things which do not reproduce.

1. The composite `v` over-split three real 2019-20 parishes
--------------------------------------------------------------

The only evidence the branch offered for the DISTRICT/PARISH key was "clusters
spanning more than one district: 20 -> 0, 23 -> 0".  That number is ZERO BY
CONSTRUCTION under a key that contains the district: it checks that the key was
applied, and structurally cannot see the opposite error.  The branch ran the
spelling-artefact audit on the subcounty component it REJECTED but not on the
district component it ADOPTED.

Audit that can see it: group GSEC1 by its own (county, subcounty, parish) triple
and count distinct v.  2018-19: 0.  2019-20: 3 --

  BUSIKI/NAMUTUMBA/NAWANSAGWA   NAMTUMBA (1 hh)  / NAMUTUMBA (10)
  KASSANDA/KITUMBI/KIJUNA       MUBENDE  (1 hh)  / KASSANDA  (9)
  OMORO/BOBI/PALWO              GULU     (2 hh)  / OMORO     (8)

NAMTUMBA is a misspelling occurring once in 3,098 households; Kassanda District
was carved out of Mubende in 2019 and Omoro out of Gulu in 2016, and a straggler
household still codes the parent.  All three pairs also agree on `region`.  Four
households were being put in a 1-2 household phantom cluster instead of the
10-11 household cluster they belong to -- the same "new bug traded for the old
one" the file cites when declining to key on subcounty.

Fixed with a three-entry `_V_ALIASES` table in 2019-20/_/mapping.py, keyed on
the (district, parish) PAIR: Mubende and Gulu are still real, populated
districts in this wave (29 / 37 hh), so a bare district rename would be wrong.
`District` is now declared row-valued in that wave's cluster_features and
resolved through the same table -- otherwise reduce_to_agreed would blank
District for exactly the three clusters just merged, trading three over-splits
for three <NA>s whose value we have just argued we know.

  2019-20 clusters 827 -> 824; total 4,808 -> 4,805 (parquet 4,812 -> 4,809).
  cluster_features District <NA> in 2019-20: 0.  grain_reports('Uganda'): [].
  cf (t,v) unknown to sample: 0; orphaned sample clusters: 4 (2010-11, as
  documented).  Cold build, isolated LSMS_DATA_DIR.

Two further one-household district misspellings (PALISA, RUKINGIRI; also PALISA
in 2018-19) are deliberately NOT aliased: the correct spelling holds no
household in the affected parish, so they split nothing.  Written down rather
than silently skipped.

2. Tests that discriminate, and fixtures that cannot go vacuous
--------------------------------------------------------------

* test_v_does_not_over_split_one_real_parish[2018-19, 2019-20] -- the audit
  above, against GSEC1.  Negative control: emptying `_V_ALIASES` in place (git
  stash does nothing to a committed change) fails the 2019-20 parametrisation.
  The 2018-19 one passes either way; its docstring says so.
* test_the_2019_20_alias_table_is_the_thing_being_tested -- data-free companion,
  so the fix is pinned in the credential-free CI job too.  Also asserts an
  unaliased pair passes through untouched.
* The module no longer converts a build failure into a skip.  `_uganda_or_skip`
  and the `built` fixture wrapped everything in `except Exception:
  pytest.skip(...)`, which would make the end-to-end half of the file vacuous
  the moment the build broke.  Replaced by tests/conftest.py's
  aws_creds_available() guard on the `uga` fixture only -- so the data-free job
  runs 8 tests and skips 19, instead of skipping on any error.

3. False claims withdrawn (CONTENTS.org, .coder/ledger/323-uganda.md, PR body)
--------------------------------------------------------------

* "Pre-existing failure ... test_gh323_explicit_reducers.py::
  test_core_does_not_dispatch_the_reducers fails on pristine
  origin/development."  It does not: 22 passed at 45aee17 (this branch's base)
  AND at 4c236d1 (current development).  PRs #635/#640 replaced the substring
  guard with an AST one.  Withdrawn.
* The ledger said the baseline fixture was refreshed for "exactly two entries".
  It was three (cluster_features, sample, people_last7days).  Corrected, with
  the reason only two of 53 entries can carry `v` (cached parquets are written
  before _join_v_from_sample).
* "65-126 clusters per wave for Rural" -> re-derived per wave: 70 / 65 / 122 /
  122 for 2013-14 / 2015-16 / 2018-19 / 2019-20.
* "23 of 2019-20's 794 parish names" -> 793, and "as coded".

4. Recorded, not fixed (scope)
--------------------------------------------------------------

* Consumer-visible <NA> is a third of a column in the two `comm` panel waves,
  not a handful of cells: District 27.8% (2010-11) / 33.0% (2011-12),
  Latitude/Longitude 41.2% / 49.8%.  Now in CONTENTS.org and the ledger.
* 32 of 2019-20's 824 clusters span more than one county/subcounty SPELLING.
  Almost all are the data-entry artefacts the key deliberately ignores, but
  KAKUMIRO/KIKWAYA (8 hh in BUGANGAIZI/KIKWAYA, 1 in BUYANJA/KAKINDO) is a
  genuine second parish of the same name inside one district.  Untouched, and
  unquantified for the other waves -- noted so the silence is not read as a
  clean bill.

Verified: tests/test_uganda_323_grain.py 27 passed; the 323 guard suite 76
passed; test_uganda_invariance's three refreshed entries plus household_roster
(untouched) pass.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Su5JKX3wKChyfMAdXrdCTr
@ligon

ligon commented Jul 22, 2026

Copy link
Copy Markdown
Owner Author

Review applied — 9e8ac9de

Thanks; the HIGH finding is real, I reproduced it independently, and it is fixed. Two of the three "minor notes" were also false claims in git-tracked files, which is worse than a false claim in a PR thread, so those are withdrawn in CONTENTS.org and .coder/ledger/323-uganda.md as well as here.

1. The over-split — confirmed, fixed, and the metric is now labelled

Re-derived from GSEC1 without going through the library: grouping by the survey's own (county, subcounty, parish) triple and counting distinct composite v gives 2018-19: 0, 2019-20: 3 — your three cases exactly.

county / subcounty / parish district as coded corroboration
BUSIKI / NAMUTUMBA / NAWANSAGWA NAMTUMBA 1 hh, NAMUTUMBA 10 NAMTUMBA occurs once in 3,098 households
KASSANDA / KITUMBI / KIJUNA MUBENDE 1, KASSANDA 9 Kassanda carved out of Mubende, 2019
OMORO / BOBI / PALWO GULU 2, OMORO 8 Omoro carved out of Gulu, 2016

One correction to your table: I get PALWO as GULU 2 / OMORO 8, not 2 / 9. The eleventh household in that parish spells the subcounty BOBBI, so it lands in a different (county, subcounty, parish) triple; the total is 11 households either way.

All three pairs also agree on region (and on urban but for one blank), which is the extra evidence that these are one place recorded two ways.

Fix: a three-entry _V_ALIASES table in 2019-20/_/mapping.py, keyed on the (district, parish) pair rather than on the district. A bare district alias would be wrong for two of the three — Mubende and Gulu are still real, populated districts in this wave (29 and 37 households).

One thing your suggestion doesn't mention that turned out to be load-bearing: aliasing v alone would have made the merged clusters carry households whose raw district strings disagree, so reduce_to_agreed would blank District for exactly the three clusters just merged — trading three over-splits for three <NA>s whose value we had just finished arguing we know. District is therefore now declared row-valued ([district, s1aq04a]) in that wave's cluster_features and resolved through the same table. Post-fix cluster_features.District is <NA> for 0 of 2019-20's clusters.

827 → 824 clusters; total 4,808 → 4,805; var/cluster_features.parquet 4,812 → 4,809. Cold build in an isolated LSMS_DATA_DIR: grain_reports('Uganda') [], cluster_features (t,v) unknown to sample 0, orphaned sample clusters 4 (all 2010-11, as documented).

And the metric is now labelled as one. _V_ALIASES, the 2018-19 v docstring, the YAML comment, CONTENTS.org and the ledger all now say in terms that "clusters spanning >1 district: 23 → 0" is zero by construction, verifies only that the key was applied, and cannot see over-splitting — with the audit that can, and its result for both waves, next to it.

Tests (tests/test_uganda_323_grain.py, now 27):

  • test_v_does_not_over_split_one_real_parish[2018-19, 2019-20] — the triple audit against GSEC1.
  • test_the_2019_20_alias_table_is_the_thing_being_tested — data-free, so it also runs in the credential-free CI job; also asserts an unaliased pair passes through untouched.
  • Negative control: emptying _V_ALIASES in place (a git stash does nothing to a change already committed on the branch) fails both, while [2018-19] passes either way — its docstring says so rather than pretending otherwise.

The two further one-household district misspellings you spotted (PALISA, RUKINGIRI; PALISA also in 2018-19) are deliberately not aliased — I checked, and the correctly-spelled district holds no household in the affected parish, so they split nothing. That decision is written down rather than left as silence.

2. Your "minor" note 1 was a false claim in a tracked file — withdrawn

Confirmed and it is worse than minor, because the same paragraph was in .coder/ledger/323-uganda.md. test_gh323_explicit_reducers.py gives 22 passed at 45aee170 (this branch's base) and at 4c236d11 (current development). Both copies withdrawn, with the correction recorded in the ledger rather than silently deleted.

While removing it I found a second false claim I had written in the ledger: it said the baseline fixture was refreshed for "exactly two entries". It was three. Corrected, together with the reason only two of the 53 entries can carry v at all (cached parquets are written before _join_v_from_sample — which is also the reason the other 51 cannot move).

3. Note 2 — percentages added, re-derived

District <NA>: 27.8% of 2010-11 and 33.0% of 2011-12 clusters; Latitude/Longitude 41.2% / 49.8%. Your figures. Now in CONTENTS.org, the ledger and the PR body. I also re-derived "65–126 clusters per wave for Rural" — which I could not reproduce — and replaced it with the per-wave numbers: 70 / 65 / 122 / 122 for 2013-14 / 2015-16 / 2018-19 / 2019-20.

4. One thing you did not flag, which I fixed anyway

The module's fixtures wrapped everything in except Exception: pytest.skip(...). You measured them on origin/development, where the build succeeds (with wrong values), so they failed properly and the negative control was sound. But the guard would have turned the whole end-to-end half of the file into skips the moment the build actually broke — the exact silence this PR is about. Replaced with tests/conftest.py's aws_creds_available() guard on the uga fixture alone. Simulated data-free run: 8 passed, 19 skipped, instead of the previous "skip on any error".

5. Recorded, not fixed

32 of 2019-20's 824 clusters still span more than one county/subcounty spelling. Almost all are the data-entry artefacts the key deliberately ignores, but one — KAKUMIRO/KIKWAYA, 8 hh in BUGANGAIZI/KIKWAYA and 1 in BUYANJA/KAKINDO — is a genuine second parish of the same name inside a single district. The composite key does not make it worse and fixing it is a different job; it is written into CONTENTS.org and the ledger, including that it is unquantified for the other waves, so the silence is not read as a clean bill.

Verification

27 passed in test_uganda_323_grain.py; 76 passed in the 323 guard suite (test_gh323_explicit_reducers, test_gh323_grain_contract, test_grain_collapse*, test_uganda_v_grain_invariants); test_uganda_invariance passes on the three refreshed entries plus household_roster, which is untouched by the re-keying. All cold, isolated LSMS_DATA_DIR with dvc-cache symlinked, LSMS_COUNTRIES_ROOT on the worktree.

Disputed: nothing substantive. The only correction to the review is PALWO = GULU 2 / OMORO 8 rather than 2 / 9.

@ligon

ligon commented Jul 22, 2026

Copy link
Copy Markdown
Owner Author

Independent verification of the remediation — 9e8ac9de

I re-ran the review against the pushed head rather than reading the reply. Every
number below is my own, from a cold build in an isolated LSMS_DATA_DIR
(dvc-cache symlinked), LSMS_COUNTRIES_ROOT + PYTHONPATH on a fresh worktree
at 9e8ac9de with lsms_library.__file__ asserted to resolve there. No dvc CLI.

Verdict: READY. All four review findings are addressed; the one dispute is
correct and I reproduced it; the negative control discriminates when I run it
myself. One cosmetic staleness left, in the PR body only — see the end.

HIGH-1 — over-split v in 2019-20: fixed, and the fix is what is under test

Re-derived straight from GSEC1 with no library key logic at all (raw
district + '/' + parish, grouped by the survey's own (county, subcounty, parish) triple):

wave households parish names raw composites over-split triples
2018-19 3,176 751 781 0
2019-20 3,098 793 827 3
('BUSIKI','NAMUTUMBA','NAWANSAGWA') -> {'NAMUTUMBA/NAWANSAGWA': 10, 'NAMTUMBA/NAWANSAGWA': 1}
('KASSANDA','KITUMBI','KIJUNA')     -> {'KASSANDA/KIJUNA': 9,       'MUBENDE/KIJUNA': 1}
('OMORO','BOBI','PALWO')            -> {'OMORO/PALWO': 8,           'GULU/PALWO': 2}

The dispute is correct: PALWO is GULU 2 / OMORO 8, not 2 / 9. My review
table was wrong; the reply's reading is the one that reproduces.

Cold build at head: 4,805 clusters (322 / 656 / 320 / 321 / 706 / 875 / 781 /
824), var/cluster_features.parquet 4,809 rows, grain_reports('Uganda')
[], cluster_features (t, v) unknown to sample 0, orphaned sample
clusters 4 — all 2010-11, as documented.

Negative control, run by me, not read. I reverted
2019-20/_/mapping.py and 2019-20/_/data_info.yml to a5bd6443 (the
reviewed head — a real removal of the change, not a git stash), kept the current
test file, and re-ran:

FAILED test_v_does_not_over_split_one_real_parish[2019-20]
  -> 3 (county, subcounty, parish) triple(s) are split across more than one v
FAILED test_the_2019_20_alias_table_is_the_thing_being_tested
  -> NAMTUMBA/NAWANSAGWA -> 'NAMTUMBA/NAWANSAGWA', but it is the same place as ...
PASSED test_v_does_not_over_split_one_real_parish[2018-19]      (documented as non-discriminating)
PASSED test_v_is_an_actual_cluster_key[2018-19, 2019-20]        (the zero-by-construction direction)

Restored byte-identically afterwards (git diff HEAD empty). So the two new tests
fail without the fix and pass with it, and the pre-existing test that cannot see
this error is now labelled as such in its sibling's docstring. That was the point
of the finding and it is met.

The under-specification the reply raises against my suggested fix is real — I
built the counterfactual.
Aliasing v alone and leaving District as the raw
column, then running it through uganda.cluster_features -> reduce_to_agreed:

clusters 824, District <NA>: 3
  [('2019-20','KASSANDA/KIJUNA'), ('2019-20','NAMUTUMBA/NAWANSAGWA'), ('2019-20','OMORO/PALWO')]

Exactly the three clusters just merged. With the row-valued District declaration
as shipped: 0 of 824. My suggested fix would have traded three over-splits for
three <NA>s; the reply is right that this was load-bearing and not optional.

Supporting claims I re-derived rather than accepted: MUBENDE 29 hh / GULU 37 hh
(so a bare district rename would indeed be wrong); PALISA/KOBIUN and
RUKINGIRI/NYABITETE each hold 0 households under the correct spelling in
both waves, so declining to alias them splits nothing; 1 blank district in
2019-20 (minor note 3); CENTRAL in 10 districts in both waves; 20 / 23
recurring parish names; 32 of 824 clusters spanning >1 county/subcounty spelling,
KAKUMIRO/KIKWAYA = {('BUGANGAIZI','KIKWAYA'): 8, ('BUYANJA','KAKINDO'): 1}.

MINOR-1 — withdrawn correctly

tests/test_gh323_explicit_reducers.py: 22 passed at 45aee170 and 22
passed
at 4c236d11, run by me in two clean detached worktrees. The claim is
gone from the PR body and is recorded in .coder/ledger/323-uganda.md as a dated
correction rather than silently deleted — which is the right disposal for a false
claim that was in a tracked file. The self-reported second error ("exactly two
entries" -> three) checks out: git diff 45aee170..HEAD -- tests/fixtures/uganda_baseline.json
touches exactly three entries.

MINOR-2 / MINOR-3 — in the tracked docs, and they reproduce

My cold build: District <NA> 27.8% (2010-11) / 33.0% (2011-12),
Latitude/Longitude 41.2% / 49.8%. Present as a table in CONTENTS.org
(with Region/Rural added). The re-derived Rural-conflict counts also
reproduce exactly, from the hook's own warning:

2013-14: 139/706 {'Rural': 70,  'Latitude': 82,  'Longitude': 71}
2015-16: 150/875 {'Rural': 65,  'Latitude': 106, 'Longitude': 106}
2018-19: 221/781 {'Rural': 122, 'Latitude': 163, 'Longitude': 163}
2019-20: 122/824 {'Rural': 122}

i.e. 70 / 65 / 122 / 122, replacing the unreproducible "65–126".

Tests, fixtures, scope

  • tests/test_uganda_323_grain.py27 passed; 323 guard suite — 76 passed.
  • Data-free simulation done properly (moved countries/.dvc/s3_creds aside +
    LSMS_SKIP_AUTH=1; leaving it in place silently keeps creds available and
    gives a misleading 27-passed): 8 passed, 19 skipped, and the two data-free
    tests that pin the alias table are among the 8. No except Exception: pytest.skip remains in the module.
  • Fixture honesty: I recomputed _fingerprint() on my own cold parquets —
    var/cluster_features [4809,5] 35e1a58c…, var/sample [24362,6]
    d68ae382…, var/people_last7days [15471,4] 44455b3c… — all three
    byte-identical to the committed fixture. people_last7days is unchanged from
    the reviewed head, as claimed. I then built household_roster, housing and
    food_acquired and compared every baseline entry present on disk: 14 entries,
    0 drift
    .
  • Scope holds: the diff touches only .coder/ledger/, lsms_library/countries/Uganda/**
    and tests/**. No file under lsms_library/*.py outside countries/. The only
    occurrences of aggregation: are ledger prose explaining that it is forbidden —
    no such key in any YAML.

One thing left, non-blocking

PR body §4 still reads "2019-20 has no df_geo, so all 827 of its clusters
have no coordinates"
. Post-fix it is 824, which is what §2's own table,
CONTENTS.org (line 480) and the ledger all say. Every git-tracked file is
consistent at 824 — this is a leftover in the PR description only. Worth a one-word
edit; not a reason to hold the PR.

Confirmed independently: 2019-20 has 0 of 824 clusters with a non-null Latitude.

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