From 81331198c2ac35838f989b3a310a17ab290211a3 Mon Sep 17 00:00:00 2001 From: Ethan Ligon Date: Sun, 12 Jul 2026 19:32:58 -0700 Subject: [PATCH] fix(GhanaLSS): make the #323 index collapse declared-and-checked, not silent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GH #323: `_normalize_dataframe_index` collapses a non-unique DECLARED index with `groupby().first()`, silently discarding the dropped rows. GhanaLSS exhibits BOTH failure modes the line enables, from the same code -- so one uniform fix would have baked in a wrong value. Three distinct fixes: (A) cluster_features, 5 waves [1991-92, 1998-99, 2005-06, 2012-13, 2016-17] -- INTENDED de-duplication, made DECLARED + VERIFIED. Each wave points `file:` at a HOUSEHOLD-grain source and projects cluster-invariant columns, so 934,584 rows -> 1,000 clusters (2016-17) is the right answer, arrived at silently. Verified cluster-invariant in all five (max distinct per cluster = 1; zero violations). `aggregation:` was already declared by 9 countries and read by NOBODY -- grep for its consumption returned zero (see SkunkWorks/ grain_aggregation_policy.org item #4, "not yet built"). It was prose. So this implements it: a new `invariant` reducer that ASSERTS the column is constant within each index group and RAISES if it is not. Declaring it without implementing it would have been the "prose is not enforcement" trap. (B) cluster_features, 1987-88 + 1988-89 -- silently WRONG; column DROPPED. `Region` was wired to Y01A.DAT's REGION, the person's REGION OF BIRTH: - mapping.py's Region() and Birthplace() are byte-identical bodies over the same region_dict, and REGION is also mapped as roster Birthplace; - the waves' own `region` code list runs to 11=Nigeria, 12=Ivory Coast, 13=Togo, 14=Burkina Faso -- a Ghanaian enumeration area cannot be LOCATED in Nigeria; only a birthplace can; - REGION varies WITHIN one household in ~1/3 of households (up to 6 distinct values), and within a cluster in 167/170 and 174/176. Collapsed with .first(), a cluster inherited its first-listed person's birth region -- labelling 9/170 (1988-89) and 7/176 (1987-88) clusters "Nigeria". No cluster-invariant region source exists in either wave, so we DROP rather than guess: class-2 (silently missing) beats class-1 (silently wrong). Also drops 1987-88's `Age: AGEY` (a person attribute). 1987-88 was NOT in the reported cell list -- only because its parquet had never been built; fixing 1988-89 alone would have left the class alive. NB the fabrication is currently MASKED: Region is 100% NaN because get_categorical_mapping returns {} for these waves (a separate defect). Repairing that -- plausible, since it also breaks Birthplace -- would ARM the fabrication. Dropping Region costs zero information today and defuses it; re-adding it now RAISES via (A)'s invariant policy. (C) food_security 2016-17 -- phantom NaN key; re-keyed on `hid`. The last 110 rows of g7sec9c.dta have clust=NaN AND nh=NaN (all 8 FIES items NaN) but a valid `hid`. Keyed on [clust, nh] they collapsed onto one NaN tuple and were dropped outright. `hid` reconstructs the compound key with 100.00% fidelity on all 13,899 good rows and is byte-identical to the id built from [clust, nh]; the 110 are distinct households, disjoint from the answered set, and all 110 appear in sample(). FIES_score() already returns pd.NA when all 8 items are missing, so this does not fabricate a food-secure score of 0. EVIDENCE (wave-level parquet, the truth layer -- var/ is written POST-collapse) food_security 2016-17 : 110 NaN keys / 109 dups -> 0 / 0; 13,899 -> 14,009 uniquely-keyed households; the 13,899 pre-existing ids and their payload are byte-identical (0 lost). cluster_features : 1988-89's 14,754-dup collapse -> table removed. 967,119 dups across the 5 good waves -> DECLARED and verified (build RAISES if invariance ever breaks). GH #323 silent warnings on a cold GhanaLSS build: 8 -> 1. ROWS RECOVERED AT THE API: 0, honestly. One row per cluster IS the correct grain, and the 110 food_security rows are all-NA in every declared column so _finalize_result's dropna(how='all') drops them (as it already drops 7 all-NA households that DID have clust/nh). The value here is correctness, not row count -- the vanity metric would be a lie. REGRESSION: 757 real (country x wave x table) cells pushed through BOTH the base and fixed `_normalize_dataframe_index` on real cached data: 0 differ. All 9 existing `aggregation:` blocks are `{visit: first}` -- `visit` is an index LEVEL, never a column, and `first` != `invariant` -- so they are doubly unaffected. Guard proven fatal end-to-end through Country() (not swallowed). tests/test_currency.py::test_feature_ghana_per_wave still fails identically (`assert ['GHC'] == {'GHC'}`, GH #589, pre-existing on base -- not touched). NOT FIXED, reported instead (see .coder/ledger/323-ghanalss.md §8): - `sample` has 2 duplicate (i,t): panel_ids is many-to-one (1988-89 HH 204922 AND 204932 -> 1987-88,101332; 255718 AND 255728 -> 1987-88,114008). A DIFFERENT root cause; choosing the "true" match would be a guess. - get_categorical_mapping returns {} for 1988-89 (Birthplace/Relationship all-NA); 1998-99 cluster_features Region/Rural all-NA. Co-Authored-By: Claude Opus 4.8 (1M context) --- .coder/ledger/323-ghanalss.md | 184 ++++++++++++++++++ .../GhanaLSS/1987-88/_/data_info.yml | 37 +++- .../GhanaLSS/1988-89/_/data_info.yml | 44 ++++- .../GhanaLSS/2016-17/_/data_info.yml | 32 ++- .../countries/GhanaLSS/_/data_scheme.yml | 30 +++ lsms_library/country.py | 85 +++++++- tests/test_declared_aggregation_gh323.py | 184 ++++++++++++++++++ 7 files changed, 578 insertions(+), 18 deletions(-) create mode 100644 .coder/ledger/323-ghanalss.md create mode 100644 tests/test_declared_aggregation_gh323.py diff --git a/.coder/ledger/323-ghanalss.md b/.coder/ledger/323-ghanalss.md new file mode 100644 index 000000000..2ea664b26 --- /dev/null +++ b/.coder/ledger/323-ghanalss.md @@ -0,0 +1,184 @@ +# Prior-Art Ledger — GH #323 (GhanaLSS) + +**Search tier used:** ripgrep + git (gitnexus MCP tools were not available in this +environment; call-graph established manually — `_normalize_dataframe_index` has +exactly four call sites, all in `country.py`: 2115, 2715, 2929, 2983). + +## §1 Task, restated + +`_normalize_dataframe_index` (`lsms_library/country.py`) reduces a wave/country +frame to its DECLARED index (`data_scheme.yml` → `index:`). When that index is +non-unique it collapses the duplicates with `groupby().first()`, keeping one row +per tuple and silently discarding the rest. GhanaLSS was reported with 7 +affected cells (~982k duplicate rows). The task: fix GhanaLSS's instances +*without* leaving the class alive, and without moving any other country. + +GhanaLSS turns out to exhibit **both** failure modes the same line of code +enables — which is why one uniform fix would have been wrong: + +* **(A) benign-but-silent** — `cluster_features`, 5 waves. The source is + household-grain; the projected columns are cluster-invariant. The collapse is + a correct de-duplication, arrived at silently. +* **(B) silently WRONG** — `cluster_features`, 1987-88 + 1988-89. `Region` is + wired to a person-level **region of birth**. `.first()` fabricates a cluster's + region from its first-listed person. +* **(C) phantom NaN keys** — `food_security`, 2016-17. 110 households with + NaN `clust`/`nh` collapse onto one NaN tuple and are dropped. + +## §2 Existing machinery + +| symbol | path:line | what it does | tested? | reuse / extend / new | +|--------|-----------|--------------|---------|----------------------| +| `_normalize_dataframe_index` | `country.py:4100` | reorders/reduces to declared index; `groupby().first()` on duplicates | partly | **extend** (checked reducer) | +| `_ADDITIVE_MEASURE_COLUMNS` | `feature.py:101` | `food_acquired` → sum, not first (GH #501) | yes | precedent for a per-table policy | +| `aggregation:` block | 9 × `data_scheme.yml` | declared `{visit: first}` | **no** | **was INERT — never read by any code** | +| `_finalize_result` `dropna(how='all')` | `country.py:2217` | drops all-NA rows | yes | explains why (C) recovers 0 API rows | +| `get_categorical_mapping` | `local_tools.py:1138` | org-table → dict | yes | returns `{}` for 1988-89 (separate bug) | + +**Key prior-art finding:** `aggregation:` was already *declared* by 9 countries +and *skipped* as a scheme meta-key (`country.py:2387`, `diagnostics.py:230`) — +but **grep for its consumption returns zero**. It was pure prose. Adding +`aggregation:` to GhanaLSS without implementing it would have been precisely the +"prose is not enforcement" failure. See `SkunkWorks/grain_aggregation_policy.org` +(item #4, "not yet built"). + +## §3 Definitions & conventions in force + +- Design contract, `SkunkWorks/grain_aggregation_policy.org`: *"the + composition/access path — `country.py` (`_normalize_dataframe_index`) — NEVER + reduces grain."* Item #3 of its implementation order names this exact line: + *"Both core `.first()` collapses are the GH #323 / #325 silent-data-loss + footguns."* The full union+sentinel rewrite is a framework-wide change and is + **not** attempted here (it would move every country); this task implements the + narrower "declared or fatal" half. +- `cluster_features` owns `v`; declared index `(t, v)` — `CLAUDE.md`, canonical + `lsms_library/data_info.yml`. +- class-1 (silently WRONG) vs class-2 (silently MISSING): class-2 is strictly + safer. Drop loudly rather than guess. + +## §4 Invariants & assumptions + +- `groupby(level=…)` defaults to `dropna=True` → **NaN-keyed rows are dropped + outright, not merged into a phantom**. Verified: `food_security` 2016-17 loses + all 110. +- `_finalize_result`'s `dropna(how='all')` (`country.py:2217`) drops any row + whose every non-index column is NA. This is why **rows recovered = 0** (§6). +- The `.pth` trap is real: `PYTHONPATH` does **not** redirect `lsms_library` to a + worktree — `sys.path[0]` (cwd) wins. Verified empirically; defeated with an + explicit `sys.path.insert(0, WT)` + assert. + +## §5 Reuse decision + +Extend the existing, already-declared-but-inert `aggregation:` block rather than +invent a parallel key. New reducer `invariant`, keyed on **columns** (per the +design doc's `{column: reducer}` shape). All 9 existing blocks are +`{visit: first}` — `visit` is an index *level*, never a column, and `first` ≠ +`invariant`, so they are doubly unaffected. Proven: §6. + +## §6 Evidence + +**Instrument validated first** (the brief's trap): scanner reproduced Mali +`household_roster` 2014-15 dup = **32,026** and Guyana `housing` 1992 dup = +**311** exactly, then all 7 reported GhanaLSS cells exactly. + +### (B) `Region` is birthplace — five independent strands + +1. `mapping.py`'s `Region()` and `Birthplace()` are **byte-identical function + bodies** over the same `region_dict`; both waves map `REGION` as the roster's + `Birthplace`. +2. The waves' own `categorical_mapping.org` `region` code list runs 1..17 and + includes **11=Nigeria, 12=Ivory Coast, 13=Togo, 14=Burkina Faso, 15=Mali, + 16=Other Africa**. A Ghanaian enumeration area cannot be *located* in Nigeria. +3. `REGION` varies **within a single household** — 1,019/3,192 HH (1988-89) and + 1,033/3,136 (1987-88), up to **6 distinct values in one household**. +4. It varies within a cluster in 167/170 (1988-89) and 174/176 (1987-88). +5. `Y01A.DCT` order: MAR, SCOHAB, SID, **REGION, NAT**(ionality), LODGE — the + classic birthplace→nationality pair. + +**Averted fabrication** (decoding REGION as the repaired `region_dict` would): + +| wave | clusters labelled **"Nigeria"** | disagree with modal birthplace | +|------|-------------------------------:|-------------------------------:| +| 1987-88 | **7** of 176 | 49 of 176 | +| 1988-89 | **9** of 170 | 53 of 170 | + +**The landmine.** `Region` is currently **100 % NaN** (0/14,924) because +`get_categorical_mapping` returns `{}` for 1988-89 — an *unrelated* defect. So +the class-1 fabrication is **armed behind a class-2 bug**: repair `region_dict` +(a plausible future fix, since it also breaks `Birthplace`) and the fabrication +goes live. Dropping `Region` costs **zero** information today and defuses it. + +### (A) cluster-invariance — verified, all 5 waves + +`region` / `loc2` / `ez`: max distinct value per cluster = **1**; clusters with +>1 = **0**. So `.first()` returns the right answer; it was silent, not wrong. + +| wave | source | rows | clusters | +|------|--------|-----:|---------:| +| 1991-92 | POV_GH.DTA | 4,523 | 365 | +| 1998-99 | SEC0A.DTA | 5,998 | 300 | +| 2005-06 | aggregates/pov_gh5 | 8,687 | 580 | +| 2012-13 | PARTA/g6loc_edt | 16,772 | 1,200 | +| 2016-17 | g7sec8h.dta | 934,584 | 1,000 | + +### (C) `food_security` 2016-17 + +110 tail rows (13,899–14,008): `clust`=NaN, `nh`=NaN, **all 8 FIES items NaN**, +but a valid `hid`. `hid == f"{clust}/{nh:02d}"` with **100.00 %** fidelity on the +13,899 good rows, and is byte-identical to the id the framework builds from +`[clust, nh]`. The 110 parse to **110 distinct households, disjoint** from the +answered set, and **all 110 are present in `sample()`**. `FIES_score()` already +returns `pd.NA` when all 8 items are missing, so recovery does **not** fabricate +a food-secure 0. + +### The guard actually bites — validated where validation is NEEDED + +Testing the guard against the *live* 1988-89 tree is **vacuous** (Region is +all-NA → `nunique` = 0 → passes trivially). That would be validating where it is +free. Fed the **real birth-region values** (the world where `region_dict` is +repaired), it RAISES: + +``` +1987-88: 'Region' non-constant in 174 of 176 group(s) -> ValueError +1988-89: 'Region' non-constant in 167 of 170 group(s) -> ValueError +``` + +### Nothing else moved + +757 real (country × wave × table) cells pushed through **both** the BASE and FIX +`_normalize_dataframe_index` on real cached data: **0 differ**, 0 non-GhanaLSS +deltas. The change is data-inert everywhere else; it alters only *warnings*. + +## §7 Honest accounting — rows recovered = 0 + +The API row count does **not** move, and saying otherwise would be a vanity +metric: + +- (A) one row per cluster **is** the correct grain — nothing to recover. +- (B) 1988-89's `cluster_features` was already contributing **0 API rows** + (all-NA → `dropna(how='all')`). Removing it is a 0-row change. +- (C) the 110 recovered households are all-NA in every declared column, so + `_finalize_result`'s `dropna(how='all')` drops them — exactly as it already + drops 7 all-NA households that *did* have `clust`/`nh` populated. + +The value is **correctness**: a fabricated cluster `Region` deleted, a silent +967k-row collapse made declared-and-checked, and a 110-household phantom killed. + +## §8 Left undone (deliberately, not fixed) + +1. **`sample` 2 duplicate `(i, t)` tuples** — pre-existing, fires identically on + pristine base. Root cause is **different**: `panel_ids` is many-to-one — + 1988-89 HH `204922` **and** `204932` both map to `1987-88,101332`; `255718` + **and** `255728` both map to `1987-88,114008`. Two *distinct* households + collapse onto one id. My `invariant` reducer cannot apply (weight/strata + genuinely differ between two different households), and choosing the "true" + match is a guess I refuse to make. **Reported, not guessed.** +2. **`get_categorical_mapping` returns `{}` for GhanaLSS 1988-89** — both + `region_dict` and `relationship_dict` are empty, so `Birthplace` *and* + `Relationship` are all-NA for that wave. Separate defect; **not fixed here on + purpose** — fixing it would *arm* the (B) fabrication, which is precisely why + (B) is being removed first. 1998-99's `cluster_features` Region/Rural are + likewise all-NA (0/5,998), so that wave contributes 0 API rows today. +3. The framework-wide union+sentinel rewrite (`grain_aggregation_policy.org` + item #3). Out of scope for a single-country fix; the `invariant` reducer is + the "declared or fatal" half and is available to every country now. diff --git a/lsms_library/countries/GhanaLSS/1987-88/_/data_info.yml b/lsms_library/countries/GhanaLSS/1987-88/_/data_info.yml index b119e7324..71e9fb271 100644 --- a/lsms_library/countries/GhanaLSS/1987-88/_/data_info.yml +++ b/lsms_library/countries/GhanaLSS/1987-88/_/data_info.yml @@ -1,13 +1,36 @@ Country: GhanaLSS Wave: 1987-88 -cluster_features: - file: Y01A.DAT - idxvars: - v: CLUST - myvars: - Region: REGION - Age: AGEY +# cluster_features: DELIBERATELY NOT WIRED (GH #323). +# +# Identical defect to 1988-89 (see that wave's data_info.yml for the full +# argument), and it was NOT in the originally-reported affected-cell list only +# because this wave's cluster_features parquet had never been built -- fixing +# 1988-89 alone would have closed the instance and left the class. +# +# The block that used to live here read `Region: REGION` and `Age: AGEY` from +# Y01A.DAT, the PERSON-LEVEL ROSTER (15,492 persons / 3,136 households / 176 +# clusters): +# +# * `Region: REGION` is the person's REGION OF BIRTH, not the cluster's region. +# REGION varies WITHIN a household in 1,033 of 3,136 households (up to 6 +# distinct values in ONE household), and within a cluster in 174 of 176. The +# wave's own `region` code list includes 11=Nigeria, 12=Ivory Coast, +# 13=Togo, 14=Burkina Faso -- foreign countries a Ghanaian enumeration area +# cannot be located in. mapping.py's Region() and Birthplace() are the same +# function body over the same region_dict, and REGION is mapped as the +# roster's `Birthplace` below. +# * `Age: AGEY` is a PERSON's age. Collapsed to one row per cluster it made a +# cluster's "Age" the age of its first-listed person (range 17-80) -- a +# category error, meaningless as a cluster feature. +# +# No cluster-invariant region source exists in this wave (Y00A.DAT's LOC is a +# plausible Rural indicator but varies within cluster in 3 of 170 clusters, so it +# is NOT wired without verification). Dropping is safer than guessing: silently +# MISSING (class-2) beats silently WRONG (class-1). +# +# Enforced by `aggregation: {Region: invariant}` in `_/data_scheme.yml`: +# re-adding either column here RAISES at build time. sample: file: Y00A.DAT diff --git a/lsms_library/countries/GhanaLSS/1988-89/_/data_info.yml b/lsms_library/countries/GhanaLSS/1988-89/_/data_info.yml index cdedcc5b1..f9663f771 100644 --- a/lsms_library/countries/GhanaLSS/1988-89/_/data_info.yml +++ b/lsms_library/countries/GhanaLSS/1988-89/_/data_info.yml @@ -1,12 +1,44 @@ Country: GhanaLSS Wave: 1988-89 -cluster_features: - file: Y01A.DAT - idxvars: - v: CLUST - myvars: - Region: REGION +# cluster_features: DELIBERATELY NOT WIRED (GH #323). +# +# This wave has NO cluster-level region variable. The block that used to live +# here read `Region: REGION` from Y01A.DAT -- but Y01A.DAT is the PERSON-LEVEL +# ROSTER (14,924 persons / 3,192 households / 170 clusters) and its REGION column +# is the person's REGION OF BIRTH, not the cluster's region. Four independent +# strands establish this: +# +# 1. The same column of the same file is ALSO mapped, correctly, as the +# roster's `Birthplace: REGION` below -- and mapping.py's Region() and +# Birthplace() are literally the same function body over the same +# region_dict. +# 2. The `region` code list in this wave's categorical_mapping.org runs 1..17 +# and includes 11=Nigeria, 12=Ivory Coast, 13=Togo, 14=Burkina Faso, +# 15=Mali, 16=Other Africa. A Ghanaian enumeration area cannot be located +# "in Nigeria"; only a BIRTHPLACE can take those values. +# 3. REGION varies WITHIN a household in 1,019 of 3,192 households, with up to +# 6 distinct values in a single household. No household straddles 6 +# regions; its members can be BORN in 6. (It varies within a CLUSTER in +# 167 of 170 clusters.) +# 4. Y01A.DCT's variable order -- MAR, SCOHAB, SID, REGION, NAT (nationality), +# LODGE, MON, MEMB -- is the classic birthplace->nationality questionnaire pair. +# +# Because the declared cluster_features index is (t, v), the framework collapsed +# these 14,924 person-rows to 170 clusters via groupby().first(), which silently +# took the birth-region of the FIRST-LISTED PERSON of the FIRST-LISTED HOUSEHOLD +# and called it the cluster's region. +# +# There is no valid substitute: a sweep of all 93 .DAT files in this wave's Data/ +# found no cluster-invariant region-like column, and CLUST does not encode region. +# Taking the cluster's MODAL birthplace would be a guess that fails precisely +# where it matters (migrant-receiving clusters: Greater Accra, mining/cocoa +# areas), so we DROP the column rather than fabricate one: silently MISSING +# (class-2) is strictly safer than silently WRONG (class-1). +# +# This is now enforced, not merely documented: `_/data_scheme.yml` declares +# `aggregation: {Region: invariant}` for cluster_features, so re-adding +# `Region: REGION` here RAISES at build time instead of collapsing quietly. sample: file: Y00A.DAT diff --git a/lsms_library/countries/GhanaLSS/2016-17/_/data_info.yml b/lsms_library/countries/GhanaLSS/2016-17/_/data_info.yml index 26fccafb9..8ad0e51d4 100644 --- a/lsms_library/countries/GhanaLSS/2016-17/_/data_info.yml +++ b/lsms_library/countries/GhanaLSS/2016-17/_/data_info.yml @@ -73,9 +73,35 @@ food_security: # (binarized by the same-named formatting fn in mapping.py). file: g7sec9c.dta idxvars: - i: - - clust - - nh + # GH #323: key on `hid`, NOT on the [clust, nh] pair. + # + # g7sec9c.dta has 14,009 rows. The first 13,899 are unique on + # (clust, nh); the last 110 (rows 13,899-14,008) have clust=NaN AND + # nh=NaN -- but a perfectly valid `hid` -- and every one of their eight + # FIES items is NaN. Those are GLSS7 households that never answered + # Section 9C; the numeric clust/nh columns were simply left unpopulated + # where `hid` was. It is a defect in the World Bank source file. + # + # Keyed on [clust, nh], all 110 collapsed onto a single (t, NaN) tuple, + # so they were dropped outright by the duplicate-collapse (groupby's + # dropna) -- 110 distinct households silently vanishing into one phantom. + # + # `hid` is complete and reconstructs the compound key exactly: on all + # 13,899 well-keyed rows, hid == f"{clust}/{nh:02d}" with 100.00% + # fidelity, which is byte-identical to the id the framework builds from + # [clust, nh] (e.g. '70001/01'). So keying on `hid` leaves those 13,899 + # ids unchanged and additionally recovers the 110 as the distinct + # households they are (all 110 are present in sample(), and their 110 + # ids are disjoint from the 13,899). + # + # Their answers stay all-NA -- the truthful representation of "did not + # answer Section 9C". FIES_score() already returns pd.NA when all eight + # items are missing, so this does NOT fabricate a food-secure score of 0. + # (_finalize_result's dropna(how='all') then drops these all-NA rows from + # the API, exactly as it already does for 7 all-NA households that DID + # have clust/nh populated. The fix removes the phantom at the source; it + # is not expected to move the API row count.) + i: hid myvars: Worried: s9cq1 HealthyDiet: s9cq2 diff --git a/lsms_library/countries/GhanaLSS/_/data_scheme.yml b/lsms_library/countries/GhanaLSS/_/data_scheme.yml index e973c31b9..939b90e85 100644 --- a/lsms_library/countries/GhanaLSS/_/data_scheme.yml +++ b/lsms_library/countries/GhanaLSS/_/data_scheme.yml @@ -19,9 +19,39 @@ Index Info: Data Scheme: cluster_features: + # GH #323 -- DECLARED de-duplication, not a silent collapse. + # + # Every wired wave points `file:` at a HOUSEHOLD-grain source (the poverty + # aggregates / cover page / Section-8H food module) and projects columns that + # describe the CLUSTER, not the household. So the source carries one row per + # household (2016-17: one row per household x food item -- 934,584 rows for + # 1,000 clusters) while the declared index (t, v) is one row per cluster. + # Collapsing to (t, v) is therefore a DE-DUPLICATION of a cluster-invariant + # projection -- the right answer -- but before #323 it happened SILENTLY via + # groupby().first(), which is the same line of code that would happily + # fabricate a value if the column were NOT cluster-invariant. + # + # `invariant` makes that assumption CHECKED rather than assumed: the build + # RAISES if any of these columns ever varies within a (t, v) group. Verified + # cluster-invariant in all five wired waves at the time of writing (max + # distinct value per cluster = 1; zero clusters with >1): + # 1991-92 POV_GH.DTA 4,523 HH -> 365 clusters + # 1998-99 SEC0A.DTA 5,998 HH -> 300 clusters + # 2005-06 aggregates/pov_gh5 8,687 HH -> 580 clusters + # 2012-13 PARTA/g6loc_edt 16,772 HH -> 1,200 clusters + # 2016-17 g7sec8h.dta 934,584 rows-> 1,000 clusters + # + # 1987-88 / 1988-89 are deliberately NOT wired: their only candidate source + # (Y01A.DAT REGION) is the person's REGION OF BIRTH, not the cluster's region + # -- see those waves' data_info.yml for the full argument. index: (t, v) Region: str Rural: str + Ecological_zone: str + aggregation: + Region: invariant + Rural: invariant + Ecological_zone: invariant household_roster: index: (t, i, pid) Sex: str diff --git a/lsms_library/country.py b/lsms_library/country.py index aed888791..bc7444e41 100644 --- a/lsms_library/country.py +++ b/lsms_library/country.py @@ -4196,8 +4196,38 @@ def _normalize_dataframe_index( if 'Price' in df.columns and {'Expenditure', 'Quantity'} <= set(df.columns): df['Price'] = df['Expenditure'] / df['Quantity'].where(df['Quantity'] != 0) else: + # GH #323: a DECLARED `aggregation:` policy in data_scheme.yml makes + # the collapse explicit instead of silent. The `invariant` reducer + # says: "the source is FINER-grained than the declared index, but this + # column is CONSTANT within each index group, so collapsing is a + # verified DE-DUPLICATION -- not a merge of distinct entities." + # It is CHECKED, not asserted in prose: _assert_invariant_within_index + # RAISES if the column is ever non-constant within a group, which turns + # a silently-WRONG collapse (class-1) into a loud failure. + # + # Worked example -- GhanaLSS cluster_features: every wave points `file:` + # at a HOUSEHOLD-grain source and projects cluster-invariant columns + # (region/loc2), so 934,584 rows legitimately de-duplicate to 1,000 + # clusters. The SAME line of code, pointed at a person-level BIRTHPLACE + # column (GhanaLSS 1987-88/1988-89), instead fabricated a cluster's + # "Region" from the first-listed person -- this check would have caught it. + policy = (schema_entry or {}).get("aggregation") or {} + if not isinstance(policy, dict): + policy = {} + invariant_cols = [ + col for col, reducer in policy.items() + if reducer == "invariant" and col in df.columns + ] + if invariant_cols: + _assert_invariant_within_index( + df, invariant_cols, present_levels, table_name + ) + df = df.groupby(level=present_levels, observed=True).first() - if n_dropped: + # Only DECLARED columns are exempt from the warning; anything the + # policy does not cover is still a silent drop and must be surfaced. + undeclared = [c for c in df.columns if c not in invariant_cols] + if n_dropped and undeclared: # No aggregation policy for this table -- surface the loss # loudly rather than drop rows quietly (a table whose source # legitimately has multiple rows per index tuple needs an @@ -4205,8 +4235,59 @@ def _normalize_dataframe_index( warnings.warn( f"Canonical index over {present_levels} had {n_dropped} " f"duplicate tuple(s); collapsed via groupby().first(), dropping " - f"those rows (possible silent data loss — GH #323).", + f"those rows (possible silent data loss — GH #323). " + f"Columns without an aggregation policy: {undeclared}.", RuntimeWarning, ) return df + + +def _assert_invariant_within_index( + df: pd.DataFrame, + columns: list[str], + levels: list[str], + table_name: str | None = None, +) -> None: + """Verify each column is constant within every declared-index group. + + Backs the ``aggregation: {col: invariant}`` policy (GH #323). A column + declared ``invariant`` is one the survey records at a FINER grain than the + declared index but whose value does not vary within an index group (e.g. a + cluster's region, recorded once per household). Collapsing such a column + with ``.first()`` is a lossless de-duplication. + + Raises ``ValueError`` naming the offending groups if the invariant does not + hold -- a non-constant column means ``.first()`` would silently pick one + value from several distinct ones (class-1: silently WRONG). + """ + offenders: dict[str, pd.Series] = {} + for col in columns: + counts = df.groupby(level=levels, observed=True)[col].nunique(dropna=True) + bad = counts[counts > 1] + if len(bad): + offenders[col] = bad + + if not offenders: + return + + where = f" in {table_name!r}" if table_name else "" + lines = [ + f"aggregation policy declares these column(s){where} INVARIANT within " + f"{levels}, but they are NOT constant within their index group -- " + f"collapsing would silently pick one value from several (GH #323):" + ] + for col, bad in offenders.items(): + examples = ", ".join( + f"{lvl}={cnt} distinct values" for lvl, cnt in bad.head(3).items() + ) + lines.append( + f" {col!r}: non-constant in {len(bad)} of " + f"{df.groupby(level=levels, observed=True).ngroups} group(s); e.g. {examples}" + ) + lines.append( + "Either the column is wired to the WRONG source variable (one recorded at " + "a finer grain -- e.g. a person-level attribute mapped as a cluster " + "attribute), or it needs a real reducer instead of `invariant`." + ) + raise ValueError("\n".join(lines)) diff --git a/tests/test_declared_aggregation_gh323.py b/tests/test_declared_aggregation_gh323.py new file mode 100644 index 000000000..f893c9fba --- /dev/null +++ b/tests/test_declared_aggregation_gh323.py @@ -0,0 +1,184 @@ +""" +Regression tests for GH #323 -- ``_normalize_dataframe_index`` silently +collapsing a non-unique DECLARED index with ``groupby().first()``. + +The collapse has two failure modes, and GhanaLSS exhibits BOTH from the same +line of code: + +* **Benign but silent.** ``cluster_features`` sources are HOUSEHOLD-grain files + whose projected columns (region, urban/rural) are genuinely cluster-invariant, + so collapsing 934,584 rows to 1,000 clusters is a lossless de-duplication -- + the right answer, arrived at silently. +* **Silently WRONG.** GhanaLSS 1987-88 / 1988-89 wired ``Region`` to Y01A.DAT's + ``REGION`` column, which is the person's REGION OF BIRTH. Collapsed with + ``.first()``, a cluster inherits the birth-region of its first-listed person -- + labelling 9 of 170 (1988-89) and 7 of 176 (1987-88) Ghanaian enumeration areas + as being located in **Nigeria**, and disagreeing with even the cluster's modal + birthplace in 53 / 49 clusters respectively. + +The fix makes the collapse DECLARED and CHECKED rather than silent: a +``data_scheme.yml`` table may declare ``aggregation: {column: invariant}``, +which asserts the column is constant within every declared-index group and +RAISES if it is not. The same mechanism that blesses the benign case is the one +that catches the wrong case. + +NOTE on why the guard is tested against *synthetic* birthplace values below: in +the live 1988-89 tree ``Region`` is currently all-NA (an unrelated defect -- +``get_categorical_mapping`` returns an empty dict for that wave), so the guard +passes there *trivially*. Testing only that would be validating where +validation is free. The case that matters is the one where that masking bug is +repaired and the real birth-region values reach the collapse -- so the tests +below feed the guard exactly that shape. +""" +import warnings + +import pandas as pd +import pytest +import yaml + +from lsms_library.country import _normalize_dataframe_index +from lsms_library.paths import countries_root + + +class _SchemeLoader(yaml.SafeLoader): + """data_scheme.yml carries ``!make`` tags; ignore them for these tests.""" + + +_SchemeLoader.add_multi_constructor('!', lambda loader, suffix, node: None) + + +def _scheme(country): + path = countries_root() / country / '_' / 'data_scheme.yml' + return yaml.load(path.read_text(), Loader=_SchemeLoader) + + +def _data_info(country, wave): + path = countries_root() / country / wave / '_' / 'data_info.yml' + return yaml.load(path.read_text(), Loader=_SchemeLoader) + + +# -------------------------------------------------------------------------- +# The framework guard (the CLASS-level fix) +# -------------------------------------------------------------------------- + +_SCHEMA = { + 'index': '(t, v)', + 'Region': 'str', + 'aggregation': {'Region': 'invariant'}, +} + + +def test_invariant_reducer_raises_when_column_is_not_constant(): + """A column declared `invariant` that VARIES within its index group must raise. + + This is the 1988-89 shape: one row per PERSON, `Region` holding the person's + birthplace, collapsed to one row per cluster. Pre-fix this silently returned + the first person's birthplace as the cluster's region. + """ + df = pd.DataFrame({ + 't': ['1988-89'] * 5, + 'v': ['2001', '2001', '2001', '2002', '2002'], + # cluster 2001 holds three people born in three different regions + 'Region': ['Ashanti', 'Nigeria', 'Volta', 'Central', 'Central'], + }).set_index(['t', 'v']) + + with pytest.raises(ValueError, match='invariant|INVARIANT'): + _normalize_dataframe_index(df, _SCHEMA, '1988-89', 'cluster_features') + + +def test_invariant_reducer_collapses_silently_when_column_is_constant(): + """A genuinely cluster-invariant column de-duplicates without warning. + + This is the 1991-92..2016-17 shape: a household-grain source projecting a + cluster-level attribute. The collapse is correct, and now DECLARED, so it + must not emit the GH #323 warning. + """ + df = pd.DataFrame({ + 't': ['2016-17'] * 5, + 'v': ['7001', '7001', '7001', '7002', '7002'], + 'Region': ['Ashanti', 'Ashanti', 'Ashanti', 'Volta', 'Volta'], + }).set_index(['t', 'v']) + + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter('always') + out = _normalize_dataframe_index(df, _SCHEMA, '2016-17', 'cluster_features') + + assert len(out) == 2, 'should de-duplicate to one row per cluster' + assert out.loc[('2016-17', '7001'), 'Region'] == 'Ashanti' + assert not [w for w in caught if 'GH #323' in str(w.message)], \ + 'a DECLARED + verified de-duplication must not warn' + + +def test_undeclared_collapse_still_warns(): + """Without an `aggregation:` policy the collapse must still be surfaced. + + Guards against the fix accidentally silencing the warning for every table. + """ + df = pd.DataFrame({ + 't': ['x'] * 3, + 'v': ['1', '1', '2'], + 'Region': ['a', 'b', 'c'], + }).set_index(['t', 'v']) + + with warnings.catch_warnings(record=True) as caught: + warnings.simplefilter('always') + _normalize_dataframe_index(df, {'index': '(t, v)'}, 'x', 'cluster_features') + + assert [w for w in caught if 'GH #323' in str(w.message)], \ + 'an UNDECLARED collapse must still warn' + + +# -------------------------------------------------------------------------- +# The GhanaLSS instances +# -------------------------------------------------------------------------- + +def test_ghanalss_cluster_features_declares_its_aggregation(): + """The de-duplication must be DECLARED, not left silent.""" + entry = _scheme('GhanaLSS')['Data Scheme']['cluster_features'] + policy = entry.get('aggregation') + assert policy, 'cluster_features collapses a household-grain source; declare it' + assert policy.get('Region') == 'invariant' + assert policy.get('Rural') == 'invariant' + + +@pytest.mark.parametrize('wave', ['1987-88', '1988-89']) +def test_ghanalss_early_waves_do_not_map_birthplace_as_cluster_region(wave): + """Y01A.DAT's REGION is a person's BIRTHPLACE -- never a cluster's region. + + mapping.py's ``Region()`` and ``Birthplace()`` are the same function body over + the same ``region_dict``, and the wave's own code list runs to 11=Nigeria, + 12=Ivory Coast, 13=Togo, 14=Burkina Faso -- values a Ghanaian enumeration + area cannot take. These waves must therefore not declare cluster_features + at all (no cluster-invariant region variable exists in their 93 source files). + """ + info = _data_info('GhanaLSS', wave) + cf = info.get('cluster_features') + if cf is None: + return # correct: not wired + myvars = cf.get('myvars', {}) or {} + assert myvars.get('Region') != 'REGION', ( + f'GhanaLSS {wave}: cluster_features.Region is wired to Y01A.DAT REGION, ' + 'the person-level region of BIRTH (varies within a single household in ' + '~1/3 of households, up to 6 distinct values). Collapsed to one row per ' + 'cluster this fabricates the cluster region -- GH #323.' + ) + assert 'Age' not in myvars, ( + f'GhanaLSS {wave}: `Age` is a PERSON attribute; as a cluster feature it ' + "becomes the age of the cluster's first-listed person." + ) + + +def test_ghanalss_food_security_keys_on_complete_hid(): + """2016-17 food_security must key on `hid`, not the incomplete [clust, nh]. + + The last 110 rows of g7sec9c.dta have clust=NaN AND nh=NaN (with all eight + FIES items NaN) but a valid `hid`. Keyed on [clust, nh] those 110 distinct + households collapse onto a single NaN tuple and are dropped outright. + """ + info = _data_info('GhanaLSS', '2016-17') + idx = info['food_security']['idxvars'] + assert idx.get('i') == 'hid', ( + 'food_security must key on the complete `hid` column; keying on ' + '[clust, nh] silently merges 110 NaN-keyed households into one phantom ' + '(GH #323).' + )