diff --git a/.claude/skills/add-feature/SKILL.md b/.claude/skills/add-feature/SKILL.md index 799b00f7e..7716788a2 100644 --- a/.claude/skills/add-feature/SKILL.md +++ b/.claude/skills/add-feature/SKILL.md @@ -423,8 +423,8 @@ The corresponding `data_scheme.yml` entry: - **Subdirectory structure:** Some waves use `Data/Cross_Sectional/` and `Data/Panel/` subdirectories; others have files directly in `Data/`. - **Multi-round files:** A single `.dta` containing multiple survey rounds (e.g., Tanzania 2008-15) cannot be handled by `data_info.yml` alone — use a `.py` script. - **Missing columns across waves:** Earlier survey instruments may not include all variables. Columns absent from a wave's `data_info.yml` entry will be NaN in the output — this is expected. -- **DVC-tracked files:** Pull data with `dvc pull {path}.dvc` before building. Run from the DVC root (`lsms_library/countries/`). -- **DVC lock contention:** If `dvc pull` hangs with `Unable to acquire lock`, clear stale locks: `rm -f lsms_library/countries/.dvc/tmp/*.lock lsms_library/countries/.dvc/tmp/rwlock`, then retry. For parallel work across agents, use git worktrees so each has its own DVC lock. +- **DVC-tracked files:** You do **not** pre-pull anything. `get_dataframe('../Data/file.dta')` materializes the blob on first read, lock-free and directly from S3. For a non-tabular file (questionnaire PDF, Excel codebook) use `data_access.get_data_file('{Country}/{wave}/Documentation/foo.pdf')`, which returns a local `Path`. **Never invoke the `dvc` CLI** — see `CLAUDE.md` §"Data Access". +- **DVC lock contention:** Should not arise, because the read path never takes `.dvc/tmp/lock`. If you are seeing it, something is shelling out to `dvc` — find and remove that call rather than working around it. **Do not `rm` the lock file**: it may belong to a live sibling process, and deleting it corrupts that writer. Worktrees do *not* give an agent its own DVC lock — the DVC repo is shared, which is exactly why the lock-free read path exists. - **Pre-ISA vs ISA waves:** Earlier waves (e.g., Malawi 2004-05 "IHS2") predate the LSMS-ISA standardization and often use completely different module letters and variable naming conventions. Module L might be "non-food expenditures" in 2004-05 but "durable goods" in 2010+. Always verify via the World Bank data dictionary — never assume module letters are stable across survey instruments. - **`convert_categoricals` — value-label decode (bit several features in 2026-06):** `get_dataframe()` (and the YAML path) decode Stata/SPSS value labels by **default** (`convert_categoricals=True`). This cuts both ways: - In a **script** (`materialize: make`) that assumes *numeric codes* — e.g. `df['itemcode'].astype('Int64').map(CODE_MAP)` or `pd.to_numeric(df['have_flag'])` — the default makes the column come back as the **label string** (`'Watches'`, `'Yes'`), so `int(...)`/`to_numeric` fails or NaNs out (→ empty melt, `No objects to concatenate`). Pass `get_dataframe(path, convert_categoricals=False)` to keep numeric codes. diff --git a/.claude/skills/add-feature/food-acquired/units/SKILL.md b/.claude/skills/add-feature/food-acquired/units/SKILL.md index 6b8d39d44..1fa026391 100644 --- a/.claude/skills/add-feature/food-acquired/units/SKILL.md +++ b/.claude/skills/add-feature/food-acquired/units/SKILL.md @@ -71,8 +71,10 @@ from the first source that has it: `slurm_logs/build_ehcvm_unit_codebook.py` if a new sibling is added. 3. **The questionnaire / codebook.** Survey documentation (Excel `Unites` - sheets, IHPS "CODES FOR UNIT" PDF pages) lists the code→label map. Pull it - from `{Country}/{wave}/Documentation/` via DVC (`.venv/bin/dvc pull …`) and + sheets, IHPS "CODES FOR UNIT" PDF pages) lists the code→label map. Fetch it + with `data_access.get_data_file('{Country}/{wave}/Documentation/foo.pdf')`, + which returns a local `Path` (lock-free; **never** shell out to `dvc` — see + `CLAUDE.md` §"Data Access"), then read with `pdftotext` or `pd.read_excel`. **Watch the scheme:** the questionnaire's generic list may use *different numbering* than the data (EHCVM's `Unites` sheet is 1–57; the data codes are 100–700 — they do not diff --git a/.claude/skills/add-feature/sample/SKILL.md b/.claude/skills/add-feature/sample/SKILL.md index 2d835007f..46184ecda 100644 --- a/.claude/skills/add-feature/sample/SKILL.md +++ b/.claude/skills/add-feature/sample/SKILL.md @@ -159,6 +159,89 @@ sample: - t ``` +Note `merge_on: i` — the **household** key, unique in both sub-frames. That is +what makes this merge a join. + +### The `dfs:` merge contract (GH #323 site 4) + +**`merge_on` must be unique in at least one sub-frame.** If both sub-frames are +finer-grained than the merge key, the merge is many-to-many and pandas emits a +*cartesian product within each key group* — it MANUFACTURES rows that exist in +no survey. The classic error is joining two **household**-grain frames on the +**cluster** key `v`: Ethiopia's `cluster_features` did exactly this and produced +65,508 rows in 2013-14 and 57,786 in 2015-16 from tables that have 433 and 432 +clusters. The downstream `groupby().first()` collapse then tidied it away, so +the table looked perfectly clean. + +`Wave._merge_subframes` now checks this exactly (a key value duplicated in +*both* sub-frames is the definition of the cartesian) and warns with the phantom +row count; `LSMS_GRAIN_STRICT` makes it fatal. That variable is read through the +same `_grain_strict()` predicate as sites 1–2, so all three sites agree on what +counts as "strict" (`1`/`true`/`yes`, case-insensitive) — and turning it on in +CI is gated on the whole of #323, not just this site. **Fix it by making the +merge correct — re-key to `i`, or reduce a sub-frame to the merge-key grain in a +wave script.** Never by aggregating the explosion away afterwards: core does not +aggregate (`SkunkWorks/grain_aggregation_policy.org`), and a reducer applied to +a cartesian only puts a signature on the corpse. + +#### `merge_how:` (optional, default `outer`) + +```yaml + merge_on: + - i + merge_how: left # default is `outer` +``` + +Declare `merge_how: left` when the **primary** sub-df (`dfs[0]`) is authoritative +for which rows exist and the others are strict enrichments. A geovariable file +that carries households the cover page does not is the usual case: under `outer` +those orphans arrive with a null value in every index level the cover owned. + +**Be accurate about what this buys you.** Measured on Ethiopia 2013-14 (geo file +carries 25 households the cover page does not): + +| | wave rows | null-`v` rows | delivered clusters | ΣLatitude | `District` dtype | +|---|---:|---:|---:|---:|---| +| `outer` | 5,287 | 25 | 433 | 4070.3702 | float64 | +| `left` | 5,262 | 0 | 433 | 4070.3702 | int8 | + +The orphans do **not** "collapse into one phantom cluster" — the downstream +cluster-grain collapse *deletes* them, because `groupby` drops null keys. The +delivered values are identical. So `merge_how: left` is not a data fix; it is +worth declaring because it stops manufacturing null-keyed rows for site 1 to +delete, and because it stops the merge widening an integer column. It has a +cost: under `outer` the site-1 grain report *told* you 25 geo households had no +cover page. `left` drops them silently. If that signal matters for a wave, keep +`outer` and let site 1 report it. + +#### A dropped sub-df that owned a required column is now a hard error + +The GH #515 fallback drops a secondary sub-df that fails to load and proceeds +with a warning. That is right for a file that is *unavailable* (nothing you can +edit fixes it). It is **wrong** for a file that loaded fine but does not carry +the column your YAML names — a typo, a casing mismatch (`lat_dd_mod` vs +`LAT_DD_MOD`), a renamed variable. Ethiopia lost `Latitude`/`Longitude` from +three of five waves exactly that way, behind a warning nobody read. If such a +drop leaves a column declared **required** in `data_scheme.yml` entirely absent, +`grab_data` now raises. Presence is judged after `derived:`, `drop:` and the +wave's `df_edit` hook, so a column the wave module supplies is not reported +absent. + +**Fix the column name.** In all ten cells this guard caught, the data existed: +Ethiopia and Nigeria were casing mismatches or a wrong key column, and Niger +2011-12's coordinates were in a *sibling* file in the same directory +(`NER_EA_Offsets.dta`, `LAT_DD_MOD`/`LON_DD_MOD`) while the file the YAML named +had none. + +**Know what this guard is and is not.** It is *"a mis-named column in a `dfs:` +sub-df is fatal"* — not *"a required column is never absent"*. It fires only +when a sub-df was dropped, so a wave with no `dfs:` block can serve a declared +column 100% absent and never trip it (Niger **2014-15** does: `Latitude` is +declared `float`, 0 of 270 rows populated, no raise). And `optional: true` is a +blunt escape hatch — `data_scheme.yml` is **country**-grain, so it disarms the +column for *every* wave and every script-path build of that country while the +check is per-wave. Use it only where the country genuinely never has the column. + ### Multi-round files (Tanzania 2008-15 pattern) When a single `.dta` file contains multiple survey rounds with a `round` column, the YAML path cannot handle it --- use a Python script. The script reads the file, maps round numbers to wave labels, and splits panel vs refresh households for `panel_weight`: diff --git a/.claude/skills/add-wave/SKILL.md b/.claude/skills/add-wave/SKILL.md index 4c28de572..4153ab938 100644 --- a/.claude/skills/add-wave/SKILL.md +++ b/.claude/skills/add-wave/SKILL.md @@ -29,7 +29,81 @@ from lsms_library.data_access import discover_waves discover_waves("Ethiopia") ``` -Returns a list of dicts annotated with `"local": True/False`. Waves marked `local=False` are on the WB but not yet in the repo. +Returns a list of dicts annotated with: + +| key | meaning | +|----------------|-------------------------------------------------------------------------| +| `local` | `bool` — `True` only when a wave dir *records* this catalog id. | +| `local_status` | `"yes"` / `"no"` / `"unknown"` — the honest tri-state (see below). | +| `local_waves` | the wave directories backing this entry. | + +Waves with `local_status == "no"` are on the WB but not yet in the repo. + +**Matching is on the WB catalog id, not on the wave label.** Each wave dir +records the catalog entry it came from in `Documentation/SOURCE.org` +(`#+CATALOG_ID:` — see `lsms_library/provenance.py`). Label matching used to +be the mechanism and was wrong in both directions: two different surveys can +share a year range (Nigeria's GHS-Panel W4 **3557** and Living Standards +Survey **3827** both span 2018–2019), and one catalog entry can span two of +our wave dirs (Uganda **1001** covers both `2005-06/` and `2009-10/`). + +`local_status == "unknown"` means a directory whose label matches exists but +has **no recorded WB catalog id** — so we cannot say whether it holds this +study or a different one with the same year range. Such rows carry +`local=False`: an unverified claim is treated as not-held, because wrongly +believing we hold a survey is the failure mode that hides missing data. + +To (re)stamp provenance across the tree: + +```sh +python scripts/backfill_wave_provenance.py --dry-run # report only +python scripts/backfill_wave_provenance.py # write SOURCE.org +``` + +Countries that are not WB datasets (`EthiopiaRHS`, `KenyaLPS`) are marked +`discoverable=False` in `_COUNTRY_CATALOG` and return `[]` with an explanatory +log — deliberately, rather than being silently absent. + +#### Which repositories get searched (GH #597) + +`discover_waves()` searches the collections listed in the country's +`repositories` field, **default `("lsms",)`**. The World Bank publishes whole +series outside `lsms` — Armenia's Integrated Living Conditions Survey (18 +waves) is in `central`, South Africa's General Household Survey (21 waves) is +in `datafirst` — and a lsms-only search cannot see them *at all*. + +**Adding a new country requires a one-time broad sweep** to learn which +repositories hold its series (`_wb_catalog_search(code, collection=None)` +searches everything). Curate the answer into config; do not infer it at +runtime. + +> **Widening a country to a second repository REQUIRES an `idno_pattern` that +> pins the survey series.** Dropping the collection filter inflates results +> 30–400× (Findex, DHS, Afrobarometer, enterprise surveys; `datafirst` alone +> returns 320 South African rows — censuses, election studies, school +> registers). Worse, it resurfaces studies we *already hold* under a different +> catalog id in another repository: `central` 3016 (`MWI_2010_IHS-III_..._A_ML`) +> is the same Malawi IHS3 as `lsms` 1003, and `datafirst` 902 (`ZAF_1993_PSLSD`) +> is the same 1993 survey as `lsms` 297. Nothing in the catalog metadata links +> those pairs, so id-matching cannot catch them — only the series pin can. +> A missing-wave list nobody trusts is worse than no list. + +```python +"Armenia": CountryCatalog("ARM", idno_pattern=r"_(HBS|ILCS)_", + repositories=("lsms", "central")), +"South Africa": CountryCatalog("ZAF", idno_pattern=r"_(IHS|GHS)_", + repositories=("lsms", "datafirst")), +"Liberia": CountryCatalog("LBR", idno_pattern=r"_(HIES|NHFS)_", + repositories=("lsms", "central")), +``` + +`idno_pattern` answers **"is this catalog row this country's?"** (identity), NOT +**"is this survey in remit?"**. The two diverge: Liberia's NHFS is in the +pattern because it is the catalog entry backing a wave dir we *hold*, but a +forest-resources survey is not in remit. Do not conflate them. + +Note `GET /api/collections` returns HTTP 400 — collection ids cannot be +enumerated via the API. Read them off the `repositoryid` field of search rows. ### Step 2: Add the wave @@ -125,14 +199,9 @@ DVC layers config files: `config.local` overrides `config`. This keeps the reade ## Batched DVC Operations -Always use batched `dvc add` + `dvc push` when adding multiple files. The `populate_and_push()` and `push_to_cache_batch()` functions do this automatically. On a cluster scratch filesystem, batched operations process 68 files in ~2.5 minutes vs ~90 minutes sequentially. +Publish blobs with `push_to_cache_batch()` (or `populate_and_push()` / `add_wave()`, which wrap it). These batch the underlying `dvc add` + `dvc push` for you: on a cluster scratch filesystem, batched operations process 68 files in ~2.5 minutes vs ~90 minutes sequentially. They also route through `_run_dvc_with_lock_retry()`, so concurrent writers queue on the global lock with backoff + jitter instead of failing. -If running DVC commands manually, follow CONTRIBUTING.org steps 8-9: -```bash -cd lsms_library/countries -dvc add Ethiopia/2021-22/Data/*.dta -dvc push -``` +**Do not run `dvc add` / `dvc push` by hand**, and never `rm` a DVC lock file — see `CLAUDE.md` §"Data Access". Hand-rolled invocations take the global `.dvc/tmp/lock` with no retry, which is how a parallel sweep gets a wave half-pushed. ## Data Loading in Scripts diff --git a/.coder/coverage/blessed.csv b/.coder/coverage/blessed.csv index e288837ba..378087be8 100644 --- a/.coder/coverage/blessed.csv +++ b/.coder/coverage/blessed.csv @@ -1 +1 @@ -country,feature,wave +country,feature,wave,blessed_by,date,note diff --git a/.coder/ledger/323-benin-togo.md b/.coder/ledger/323-benin-togo.md new file mode 100644 index 000000000..ab0912c66 --- /dev/null +++ b/.coder/ledger/323-benin-togo.md @@ -0,0 +1,157 @@ +# Prior-Art Ledger — GH #323 (Benin, Togo) + +**Search tier used:** ripgrep + git floor (gitnexus not consulted; the work is +config-scoped to two countries and the framework symbol was read directly). +The decisive prior art was found by `git diff origin/development...fix/323-cotedivoire`. + +## §1 Task, restated + +`_normalize_dataframe_index` (`lsms_library/country.py`) reorders a table's index +to the levels declared in the country's `_/data_scheme.yml`, and when the result +is NOT unique it collapses it with `groupby().first()` — silently discarding the +dropped rows. Benin and Togo both trip this on **`plot_inputs`**, and in exactly +one way, the same way CotedIvoire did (its case B): + +`plot_inputs` declares `index: (t, i, input, crop, u)`. `harmonize_input` maps +**every** seed label onto the single `input` value `Seed`, so `crop` is the only +level left to tell two seed line-items apart. But `harmonize_seed_crop` was +**non-injective**: four labels shared one `Autre crop` catch-all bucket, and +*three* of them actually occur in the EHCVM `s16b` roster of both countries. When +a household reports two of them in the same unit, both rows land on one index +tuple and `groupby().first()` throws one away. + +Measured (2026-07-13, `LSMS_NO_CACHE=1`, wave frame vs. API): + +| country | wave | wave rows | API rows (pre-fix) | destroyed | conflicting groups | +|---------|---------|-----------|--------------------|-----------|--------------------| +| Benin | 2018-19 | 10,605 | 10,534 | **71** | 64 | +| Togo | 2018 | 13,733 | 13,565 | **168** | 162 | + +Of Togo's 168, **165** are #323 destruction; the other **3** are *hollow* rows +(`Quantity`, `Purchased`, `Quantity_purchased` all ``) removed by the +framework's deliberate `dropna(how='all')` safety net — see §4. + +(The issue text quotes 68/61 and 156/153. Those are the same defect counted more +conservatively: they exclude the groups whose duplicate rows carry *identical* +values, where `first()` loses no information. Benin 64−3=61 groups / 71−3=68 rows; +Togo 162−9=153 / 165−9=156. Both accountings go to **0**.) + +## §2 Existing machinery (this task's area) + +| symbol | path:line | what it does | tested? | reuse / extend / new | +|--------|-----------|--------------|---------|----------------------| +| `_normalize_dataframe_index` | `lsms_library/country.py` | reorders to the declared index; `groupby().first()` collapse when non-unique — **the silent row-eater** | — | **untouched** (owned centrally by PR #614) | +| `dropna(how='all')` | `lsms_library/country.py:2217` | universal safety net: drops rows where every non-index column is NaN | — | untouched; explains Togo's residual 3 | +| `harmonize_seed_crop` | `lsms_library/countries/{Benin,Togo}/_/categorical_mapping.org` | EHCVM seed-type label → the seed's crop (the `s16b` roster has no crop column) | now yes | **extend** — split the catch-all | +| `harmonize_input` | same files | `s16bq01` label → canonical `input`; maps **all** seed labels to `Seed` | — | reuse as-is (correct: `input` is the input *identity*, the crop lives in `crop`) | +| `_finish_plot_inputs` | `Benin/2018-19/_/plot_inputs.py`, `Togo/2018/_/plot_inputs.py` | coerces dtypes, fills `CROP_NA` / `UNIT_NA` sentinels, sets the declared index | now yes | **extend** — add the uniqueness guard | +| CotedIvoire #323 fix | `fix/323-cotedivoire` (`origin`) | the identical defect + fix on the sibling EHCVM country | `tests/test_gh323_cotedivoire.py` | **the pattern followed here** | + +**Prior art is decisive.** CotedIvoire is EHCVM, the `s16b` labels are +standardized across EHCVM countries, and Benin's and Togo's +`harmonize_seed_crop` tables were copied *verbatim* from the Niger EHCVM +reference — so they inherited the identical bug. The fix here is CIV's fix, +label-for-label (`Autre céréale`, `Autre tubercule`), so the EHCVM family stays +consistent. + +## §3 Definitions & conventions in force + +- **The #323 doctrine**: duplicates on a declared index mean **the identifier is + broken or a level is missing** — *never* that a reducer should be declared. + Fix the index. (`docs/323-grain-collapse-sites`; `fix/323-cotedivoire` ledger.) +- **EHCVM index conventions**: `v: grappe`, `i: [grappe, menage]` — per + `CLAUDE.md` §"Gotchas with Teeth". Benin and Togo are both EHCVM 2018-19. +- **`plot_inputs` grain**: `(t, i, input, crop, u)` per each country's + `_/data_scheme.yml`. EHCVM records inputs at **household × input**, not + plot × input (no parcel-level input question), so `plot` is deliberately not a + level. `plot_inputs` is *not* registered in `data_info.yml`'s `index_info`. +- **Automatic categorical mappings** apply on a name match (`CLAUDE.md`). There + is **no** table named `crop`, `input` or `u` in either country's + `categorical_mapping.org`, so those index levels are **not** remapped at API + time; the wave script's values are final. (The library-level + `lsms_library/categorical_mapping/u.org` *does* canonicalize `u` at API time — + `Kilogramme`→`Kg`. It is injective over the units in use, so it neither causes + nor hides this defect.) +- Everything else: `STANDING.md` §3. + +## §4 Invariants & assumptions + +- **`harmonize_seed_crop` must stay INJECTIVE over the labels actually present.** + This is the invariant the defect violated, and it is now *enforced*, not merely + documented: a uniqueness assertion on the declared index at the end of each + wave script fails the build loudly. Verified by re-widening the bucket and + watching both builds die (Benin: 47 dup tuples; Togo: 59). +- **`Purchased` is NOT an index level and must not become one.** Togo's colliding + rows differ in `Purchased` (grappe 101 / menage 9: 16 Charrette purchased vs. + 3 Charrette own-production), which makes adding it to the index look tempting. + It is wrong: `Purchased` is a measured *attribute* of a line-item, not part of + its identity — it would not separate two distinct seeds that were both + purchased, and it would corrupt the declared grain. Evidence that the + identifier (not a missing level) is the fault: **0** of the 226 colliding + groups across both countries repeat the *same* raw `s16bq01` label — every + single collision is between two **distinct** reported items pooled by the lossy + crop map. A counterfactually fully-injective crop map drives destroyed rows to + 0 in both countries. See §6 for the residual. +- **Hollow rows are dropped by design.** `country.py:2217` `dropna(how='all')` + removes rows where every non-index column is NaN. Three Togo rows qualify + (`529004`/Fungicide, `169011`×2/Seed — the household named the input and + reported nothing about it). This is table-agnostic framework behaviour, it + destroys no reported value, and it is **not** this fix's business. +- **`lsms_library/*.py` is off-limits here** — the core `_normalize_dataframe_index` + fix is owned by PR #614 and a separate Site-2 PR. This change is config/script + only. +- Repo-wide landmines: `STANDING.md` §4. + +## §5 Reuse decision + +| quantity | decision | reason | +|----------|----------|--------| +| the collapse itself (`_normalize_dataframe_index`) | **reuse, untouched** | owned centrally (PR #614); a per-country patch is exactly the failure mode this task was scoped to avoid | +| seed → crop resolution | **extend `harmonize_seed_crop`** | the table already exists and is the right place; it was merely lossy | +| making the map injective | **reuse the CIV pattern** (`Autre céréale` / `Autre tubercule`) | identical defect on a sibling EHCVM country; same source labels; keeps the EHCVM family label-consistent | +| the build-time guard | **reuse the CIV pattern** (index-uniqueness assert at the tail of the wave script) | prose does not enforce; this fails the build loudly on any future re-widening | +| a reducer / `groupby().agg()` on plot_inputs | **rejected** | violates the #323 doctrine (§3) — fix the identifier, never declare a reducer | +| adding `Purchased` to the index | **rejected** | see §4 — attribute, not identifier; 0 same-label collisions prove the crop map is the whole fault | + +## §6 Open questions for the human + +- **Residual (guarded) non-injectivity, inherited from CIV.** Two label pairs + still share a Preferred Label, and both have **0 rows today** in both + countries, so neither can collide: + - the bare code `20` → `Autre crop` (an unlabelled category that leaked into + the label column), alongside `Autres semences`; + - `Semences de sésame` → `Sésame`, alongside the source's typo + `Semences de césame` (13 rows Benin / 103 Togo — the *only* variant that + occurs). These two ARE the same crop, so pooling them is correct, not lossy. + + CIV made the same call. If a future EHCVM wave ships rows under `20` + co-occurring with `Autres semences`, the build assertion fires — loudly, by + design — rather than silently eating a row. Flagging it so the choice is + visible rather than implicit. +- **`Autre céréale` / `Autre tubercule` have no `harmonize_food` counterpart.** + `Autre crop` does (`Autre (à préciser)` → `Autre crop`). The other two are new + labels living only in `harmonize_seed_crop`. There is no runtime coupling (the + `crop` level is not auto-mapped, and `plot_inputs` is not in `index_info`), and + CIV did the same. If `plot_inputs` is ever registered in `index_info` and its + `crop` level harmonized against `harmonize_food` cross-country, these two + labels will need homes there. + +--- +### Phase 3 — verification + +- `harmonize_seed_crop` (Benin, Togo `_/categorical_mapping.org`) — **OK (anchored on §2, §4, §5)**: extends the existing table rather than adding new machinery; the split labels are CIV's, so no divergence inside the EHCVM family. +- uniqueness assertion in `Benin/2018-19/_/plot_inputs.py`, `Togo/2018/_/plot_inputs.py` — **OK (anchored on §4, §5)**: reuses the CIV guard pattern verbatim; negative-tested (re-widening the bucket fails both builds). +- **no reducer declared, no index level added** — **OK (anchored on §3, §4)**: the #323 doctrine is honoured; the identifier was lossy and the identifier is what changed. +- **no `lsms_library/*.py` touched** — **OK (anchored on §4)**: `git diff --stat` confirms the change is confined to `lsms_library/countries/{Benin,Togo}/**` plus a new test. +- **REINVENTION check** — none. The one thing this task could have reinvented is the CIV fix; it was found first (`git diff origin/development...fix/323-cotedivoire`) and followed rather than re-derived. + +**Result (2026-07-13, `LSMS_NO_CACHE=1`):** + +| country | wave rows | API rows before | API rows after | destroyed before → after | +|---------|-----------|-----------------|----------------|--------------------------| +| Benin | 10,605 | 10,534 | **10,605** | 71 → **0** | +| Togo | 13,733 | 13,565 | **13,730** | 165 → **0** (+3 hollow rows dropped by design, §4) | + +Duplicate tuples on the declared index: **0** in both. Rows were **recovered** +(+71 Benin, +165 Togo), never lost. Both worked examples now carry distinct crop +keys and both line-items survive. diff --git a/.coder/ledger/323-benin.md b/.coder/ledger/323-benin.md new file mode 100644 index 000000000..a4aacbd7e --- /dev/null +++ b/.coder/ledger/323-benin.md @@ -0,0 +1,186 @@ +# Prior-Art Ledger — GH #323 (Benin): the residue after PR #615 + +**Search tier used:** ripgrep + git floor (gitnexus not consulted; the change is +config-scoped to one country/one wave and the framework symbols were read +directly). The decisive prior art was found with +`git log --all --grep=323 -- lsms_library/countries/Benin` and +`git show fc3be203` (CotedIvoire). + +> **Companion ledger, not a replacement.** `.coder/ledger/323-benin-togo.md` +> owns Benin's `plot_inputs` defect and its fix, which **merged on 2026-07-19** +> (PR #615, commits `36170a57` + `ef45231e`). This ledger owns what was left +> over: the `cluster_features` extraction grain, the `food_acquired` +> non-defect, and the dead `aggregation:` key. Read that one first for the +> `harmonize_seed_crop` story. + +## §1 Task, restated + +The task as briefed was "fix GH #323 for Benin", listing two deliverables: the +`cluster_features` extraction hook salvaged from +`rescue/2026-07-21/323-benin`, and an injective `harmonize_seed_crop` for +`plot_inputs`. + +**The second was already done.** `origin/development` at `53ef3a5d` already +carries the split (`Semences d'autres céréales → Autre céréale`, +`Plants/boutures de tubercules → Autre tubercule`), the uniqueness assertion in +`Benin/2018-19/_/plot_inputs.py`, the ledger, and `tests/test_gh323_benin_togo.py` +— all merged via PR #615. Re-deriving it would have produced a no-op diff and, +worse, an invented second account of the same numbers. **Verified rather than +assumed**: a cold rebuild returns 10,605 rows with 0 duplicate index tuples +(§Phase 3). + +So this task reduces to three things, and they are three *different* things — +conflating them is exactly how #323 survived elsewhere: + +| table | dup rows on the declared index | what it is | action | +|---|---|---|---| +| `plot_inputs` | 71 | broken identifier — **real loss** | none — merged in PR #615 | +| `cluster_features` | 7,342 | **wrong extraction grain** — no loss | fix the extraction + enforce the invariant | +| `food_acquired` | 1,382 | **intended bucketing** — no loss | none — core already SUMs; document it | + +`cluster_features` declares `(t, v)`, but `df_main` in +`Benin/2018-19/_/data_info.yml` reads `Region`/`Rural` from the **household**-level +cover page `s00_me_ben2018.dta` while declaring only `v: grappe`. Each grappe's +attributes are broadcast across its 8,012 households and handed to +`_normalize_dataframe_index` as 8,012 rows on a 670-cluster grain. + +## §2 Existing machinery (this task's area) + +| symbol | path:line | what it does | tested? | reuse / extend / new | +|--------|-----------|--------------|---------|----------------------| +| `_normalize_dataframe_index` | `lsms_library/country.py` (~L4550) | reorders to the declared index; `groupby().first()` when non-unique, `sum` for `_ADDITIVE_MEASURE_COLUMNS`; audits first (#614) | yes | **untouched** (D1: core is not this PR's business) | +| `_audit_index_collapse` | `lsms_library/country.py:4214` | pre-collapse audit; `GrainCollapseWarning` on destruction, **silent when lossless**, stamped into the parquet and replayed warm | yes | reuse as the oracle — its silence on Benin is a *result*, not an absence | +| `_collapse_to_cluster_grain` (Site 2) | `lsms_library/country.py:~4453` | the *other* household→cluster collapse, for countries declaring `i:` in `cluster_features` idxvars | yes | **N/A for Benin** — Benin declares only `v`, so Site 2 never fires here; the collapse lands on Site 1 | +| `_ADDITIVE_MEASURE_COLUMNS` | `lsms_library/country.py` / `feature.py` | the one surviving reduction: `food_acquired` measures are SUMMED | yes | reuse — it is why `food_acquired`'s 1,382 collisions lose nothing | +| df_edit hook dispatch | `lsms_library/country.py:1054` (`dfs:` branch) | a `mapping.py` function named after a declared table runs on the merged, **already-indexed** frame | — | **reuse** — the hook sees `(t, v)`, which is what the salvaged code assumes | +| `Wave.formatting_functions` | `lsms_library/country.py:692` | loads `{wave_folder}.py` + `mapping.py` into the hook namespace | — | reuse (also the test's handle on the hook) | +| CotedIvoire case A | `fix/323-cotedivoire` `fc3be203`, `CotedIvoire/_/cotedivoire.py` | same defect: household-grain source, `v`-only declaration; projects in the extraction | `tests/test_gh323_cotedivoire.py` | **the pattern followed here** | +| `.coder/ledger/323-benin-togo.md` | — | Benin/Togo `plot_inputs` | `tests/test_gh323_benin_togo.py` | **cite, do not re-derive** | + +## §3 Definitions & conventions in force + +- **D1, "core does not aggregate"** — `slurm_logs/DESIGN_grain_collapse_sites_2026-07-13.org`, + §"Decisions (Ethan, 2026-07-13)"; upheld in `SkunkWorks/grain_aggregation_policy.org` + §3a and now summarised in `CLAUDE.md` §"Grain Collapse". Consequence for this + PR: **no file under `lsms_library/*.py` is touched.** +- **`aggregation:` is dead config** — same doc, same section: the key survives + only in core's "skip these meta-keys" sets, a test pins that the collapse does + not honour it, and it now contradicts the policy it was written to serve. + Cited in `CLAUDE.md` §"Grain Collapse". +- **Duplicates on a declared index mean the identifier is broken or a level is + missing** — `CLAUDE.md` §"Grain Collapse"; the Mali `pid` case is the proof + that no reducer can be correct in general. +- **EHCVM key convention**: each `grappe` is visited in exactly one `vague`, so + `v: grappe` and `i: [grappe, menage]` — `CLAUDE.md` §"Gotchas with Teeth", + `Benin/_/CONTENTS.org` §"Sampling Design". +- **`cluster_features` owns `v`** — `CLAUDE.md` §"`sample()` and Cluster + Identity"; `Benin/_/data_scheme.yml` declares `index: (t, v)`. + +## §4 Invariants & assumptions + +- **The collapse hides behind the cache it poisoned.** The L2-country parquet is + written *post*-collapse, so a warm read shows a clean unique index. Every + number in this ledger is a cold build: `LSMS_NO_CACHE=1` **plus** an isolated + `LSMS_DATA_DIR` (with `dvc-cache` symlinked to the shared L1 so no blob is + re-fetched). `LSMS_NO_CACHE` alone is **not** sufficient — it is *soft* for + script-path L2-wave parquets, and a stale pre-PR-#615 `plot_inputs.parquet` + in the shared cache did in fact report 71 duplicates on the first attempt. +- **Config-tree identity.** `LSMS_COUNTRIES_ROOT=/lsms_library/countries`, + plus `PYTHONPATH=` and an `assert 'worktrees' in lsms_library.__file__` + in every measurement script (`CLAUDE.md` scrum-master addendum 3). +- **`groupby().first()` skips NA per column**, so a conflicting group collapses + to a *composite* row assembled from different source rows — a cluster that + exists nowhere in the survey. This is why "one household is missing Region" + must count as a straddle, not as a tolerable gap; a test pins it. +- **`nunique(dropna=False)` counts NaN as a value**, so a cluster whose + attribute is *uniformly* missing is one distinct value, not two. Also pinned. +- **Benin's `cluster_features` collapse is value-lossless today** (0 of 670 + grappes straddle; GPS is already grappe-level at 670/670) — so the #614 audit + is correctly silent and **this fix recovers zero rows**. Saying otherwise + would be a false claim of recovered data. +- **No `dvc` CLI.** All source reads go through `get_dataframe()` / + `_ensure_dvc_pulled()` (lock-free direct-S3); this PR needs no write path. + +## §5 Reuse decision + +| quantity | decision | reason | +|----------|----------|--------| +| `plot_inputs` injectivity | **reuse (already merged)** | PR #615 landed it on `development`; re-deriving would fork the account of the same numbers | +| `cluster_features` grain | **reuse the CotedIvoire pattern** (project in the extraction) | same defect on a sibling EHCVM country; keeps the family consistent | +| the straddle policy | **new — RAISE**, not CIV's strict-majority-with-warning | CIV *had* a conflict (grappe 648: 11 Rural vs 1 Urbain) and so needed a resolution rule. Benin has **zero**, so a resolution rule would be dead code that quietly licenses a future guess. Raising is the strictly safer no-op. Noted as a deliberate divergence in §6. | +| duplicate-collapse policy | **reuse core as-is** | D1 — core is PR #614's; a country may not patch it | +| `food_acquired` collisions | **reuse `_ADDITIVE_MEASURE_COLUMNS`** | already lossless (totals verified identical); the only deliverable is prose so the next reader does not mistake a sum for a drop | +| `aggregation: {visit: first}` on `interview_date` | **delete** | dead config that contradicts D1, and nonsense on its own terms (`visit` is an index *level*, not a measure) | + +## §6 Decisions worth flagging + +1. **The hook RAISES; CotedIvoire's warns and takes a strict majority.** A + deliberate divergence, argued in §5. If Benin ever acquires a straddling + grappe the build stops with the offending clusters named — which is the + outcome we want, because we would then have to decide *what the right answer + is*, and the framework must not decide it for us in the meantime. +2. **`food_acquired` was left alone.** It is tempting to "fix" 1,382 colliding + rows by splitting the residual food buckets the way `plot_inputs`' seed + bucket was split. That would be wrong: `Autres poissons fumés` is the + harmonized taxonomy's *own* residual category, the pooling is intended, and + `u` and `s` are both in the index so the sum is commensurable. The two cases + look identical at the level of "duplicates on a declared index" and are + opposite at the level of what the data means. Hence the table in §1. +3. **A pre-existing `aggregation:` key was deleted, slightly widening the diff.** + It was in the file this PR edits, it is dead, and leaving it would have left + a Benin-shaped counterexample to the very policy the rest of the diff + documents. + +## §7 Open questions for the human + +- None blocking. One observation for whoever owns Site 4: Benin's + `cluster_features` uses a `dfs:` **outer** merge (`df_main` ⋈ `df_geo` on + `v`) with no cardinality guard — the Site-4 pattern. Here it is benign + (`df_geo` is 670/670 unique on `v`, so the merge cannot fan out) and the + hook now runs downstream of it regardless, but it is the same construct. + +--- +### Phase 3 — verification + +Cold builds, `LSMS_NO_CACHE=1` + isolated `LSMS_DATA_DIR` (L1 shared via a +`dvc-cache` symlink), `LSMS_COUNTRIES_ROOT` + `PYTHONPATH` pinned to the +worktree and asserted in-process. + +| measure | before | after | +|---|---|---| +| `cluster_features` wave-frame rows | 8,012 | **670** | +| `cluster_features` duplicate `(t, v)` tuples | 7,342 | **0** | +| `cluster_features` API rows | 670 | 670 (frames byte-identical) | +| `food_acquired` wave rows → API rows | 190,618 → 189,236 | unchanged; `Quantity` 576,293.81 and `Expenditure` 87,287,926.96 identical on both tiers | +| `plot_inputs` wave rows / dups / API rows | 10,605 / 0 / 10,605 | unchanged (PR #615 already landed) | + +**Regression re-verification of PR #615** (cold, counterfactually re-widening +the split buckets back to one `Autre crop`): the landed fix holds — 10,605 API +rows, 0 duplicate index tuples. The counterfactual destroys **71** rows across +**64** conflicting groups, carrying **11,445.25** units of reported `Quantity` +and flipping **4** households' `Purchased` True→False. The issue text's **68** +is the same defect counted conservatively: it drops the 3 pairs whose *measured* +payload is coincidentally equal, where `first()` loses no measurement — but +those two rows still name two *different* crops, so 71 is the honest figure. +Both accountings go to 0. +| `GrainCollapseWarning`s across all 21 Benin tables | 0 | **0** | + +- `Benin/2018-19/_/mapping.py::cluster_features` — **OK (anchored on §2, §5)**: + it is the CotedIvoire case-A pattern (project in the extraction), running in + the documented df_edit slot, with the straddle policy divergence recorded in + §6.1. +- `Benin/_/data_scheme.yml` + `Benin/_/CONTENTS.org` prose — **OK (anchored on + §3)**: documents the three grain cases and cites the policy; declares no + `aggregation:` key and says why. +- Deletion of `interview_date`'s `aggregation:` — **OK (anchored on §3, §5)**: + the key is dead by the cited authority; `interview_date` returns 24,035 rows + with zero grain reports either way. +- No `lsms_library/*.py` in the diff — **OK (anchored on §3, D1)**. +- `tests/test_gh323_benin_cluster_features.py` — **OK (anchored on §4)**: 6 of + its 8 tests fail/error on the pre-fix tree; the 2 that pass are the ones that + *should* (the invariant holds in the raw data, and the API row count was + already right) — which is the honest instrument, per the CotedIvoire note + that a post-collapse-uniqueness assertion passes with the bug fully present. +- **No REINVENTION found** against `.coder/ledger/323-benin-togo.md`: this PR + contains no `harmonize_seed_crop` change. That ledger's account of + `plot_inputs` stands unmodified and is cited, not restated. diff --git a/.coder/ledger/323-ethiopia-config.md b/.coder/ledger/323-ethiopia-config.md new file mode 100644 index 000000000..aa5faa172 --- /dev/null +++ b/.coder/ledger/323-ethiopia-config.md @@ -0,0 +1,232 @@ +# Prior-Art Ledger — GH #323 (Ethiopia config) / unblocking PR #627 + +**Search tier used:** ripgrep + git floor (gitnexus not consulted). The task is +config-tree only; the blast radius was established directly, by rebuilding every +Ethiopia table before and after against PR #627's core. + +## §1 Task, restated + +PR **#627** closes site 4 of GH #323 — the `dfs:` outer merge in +`Wave.grab_data` that *manufactures* the duplicate rows every other site then +collapses. It also converts the GH #515 swallowed `KeyError` into a hard +`RuntimeError` when a dropped sub-frame costs a **required** declared column. +#627's body lists **Ethiopia — 3 cells** as an unfixed blocker. + +Two questions, therefore: + +1. Does `Country('Ethiopia')` build under #627's core against the config now on + `development`? (The blocker table in #627's body was written 2026-07-13 and + has not been refreshed since.) +2. Are #627's two Ethiopian **cartesian** cells — 2013-14 (60,221 phantom rows) + and 2015-16 (52,832) — still present? + +**Answer to both: Ethiopia is clear.** Commit `3488b791` (PR #628) landed the +`cluster_features` fix for all five waves and is an ancestor of +`origin/development`. It closes the 3 raising cells *and* both cartesian cells — +the same re-key does both, which is why #627 could not see it coming from its +own census. **No config change was needed for #627.** This branch therefore +carries the verification (tests + this ledger) rather than a re-implementation. + +Separately, the verification surfaced **one genuinely unfixed defect** in the +same country — `individual_education` in 2013-14 / 2015-16 — which is *not* a +#627 blocker but is a #323 site-1 true positive of the same shape (a broken +identifier). It is fixed here. + +## §2 Existing machinery (this task's area) + +| symbol | path:line | what it does | tested? | reuse / extend / new | +|--------|-----------|--------------|---------|----------------------| +| `Wave._cartesian_keys` | `country.py:909` (#627 only) | exact many-to-many detection: key values duplicated in **both** sub-frames | yes (#627, 16 tests) | **reuse** — the oracle for the before/after census | +| `Wave._merge_subframes` | `country.py:945` (#627 only) | warns on a cartesian with an exact phantom count; fatal under `LSMS_GRAIN_STRICT=1` | yes | **reuse** | +| `_required_scheme_columns` | `country.py:478` (#627 only) | required vs `optional:` scheme columns; drives the #515 raise | yes | **reuse** | +| `Wave.cluster_features` | `country.py:1373` | GH #161 projection of a household-grain frame onto `(t, v)`; fires **only when `i` is an index level** | yes | **reuse** — this is why `3488b791` puts `i` in `final_index` rather than `drop:`-ing it | +| `_normalize_dataframe_index` | `country.py` | collapses a non-unique **declared** index with `groupby().first()`; audits first (`_audit_index_collapse`) | yes | **reuse as the oracle** — its own warning text quantified the education loss | +| `_join_v_from_sample` | `country.py` | joins `sample.v` at API time | yes | untouched | +| `id_walk` / `Country.panel_ids` | `local_tools.py` / `country.py:3359` | walks wave-native ids back to the panel baseline | yes | untouched — but see §4, it is order-dependent | +| `3488b791` (PR #628) | `countries/Ethiopia/{wave}/_/data_info.yml` | the landed `cluster_features` fix | **not until now** | **verified here, not re-implemented** | + +## §3 Definitions & conventions in force + +Cited, not paraphrased: + +- **D1 — the core never aggregates.** `SkunkWorks/grain_aggregation_policy.org` + §3a; restated in `CLAUDE.md` §"Grain Collapse". The `aggregation:` key in + `data_scheme.yml` is **dead config** and is deliberately not honoured. A test + in this branch pins that Ethiopia declares none. +- **Duplicates on a declared index mean the IDENTIFIER IS BROKEN or a LEVEL IS + MISSING — fix the index, do not declare a reducer.** `CLAUDE.md` §"Grain + Collapse". This is the whole argument for the `individual_education` fix. +- `cluster_features` canonical index is `(t, v)`; + `individual_education` is `(t, i, pid)` — `countries/Ethiopia/_/data_scheme.yml`. +- **Ethiopia's wave-keyed ID scheme**: `household_id` / `ea_id` in W1/W4/W5; + `household_id2` / `ea_id2` in W2/W3 — `countries/Ethiopia/_/CONTENTS.org`. +- Ethiopia is a PP/PH country (19 script-path tables), but **not at these + sites**: `cluster_features` and `individual_education` both read the + *household* cover/section files (`sect_cover_hh_w*`, `sect2_hh_w*`), not a + `_pp_`/`_ph_` file, and Ethiopia declares no `wave_folder_map`. Verified + per-`t`: one round per wave here. + +## §4 Invariants & assumptions + +- **The re-key must be `household_id2` in W2/W3, never `household_id`.** + `household_id` is the W1 baseline id and is **blank** for households with no + W1 antecedent, so it is non-unique on the empty value — re-keying to it trades + an EA cartesian for a **null-key** cartesian, because `pd.merge` matches null + keys. Measured: `household_id2` is unique and blank-free in both + `cluster_features` sub-frames (5,262 / 5,287 rows, 0 keys duplicated in both). +- **`i` must reach `final_index`, not be `drop:`-ed.** `Wave.cluster_features`' + GH #161 collapse to the cluster grain only fires when `i` is an index *level*. + `3488b791` gets this right; the abandoned `fix/323-ethiopia` branch did not — + it used `final_index: [t, v]` + `drop: [i]` and leaned on an `aggregation:` + key that nothing reads, leaving the frame household-grain on a non-unique + `(t, v)` for site 1 to `first()`. +- **Wave-level assertions only, for row counts.** `_normalize_dataframe_index` + makes the API index unique **by construction**, so a post-collapse uniqueness + assertion passes with the bug fully present. (Inherited instrument note from + `tests/test_gh323_benin_togo.py` / the CotedIvoire tests.) +- **`individual_education`'s country-level row count is ORDER-DEPENDENT and must + not be asserted on.** `id_walk` is applied only once `panel_ids` has resolved, + and whether it has depends on cache state and on what else the process built + first. Reproduced against the *unchanged* `development` config: + + | invocation (same process, same cache) | rows | + |---|---| + | `Country('Ethiopia').individual_education()` | 63,139 | + | `c.panel_ids` first, then the same call | 62,939 | + + The 200-row delta lands on **2011-12 / 2015-16 / 2018-19 / 2021-22** — waves + this branch does not touch — and reproduces identically in the pre-fix config + tree. It is **pre-existing and orthogonal**; recorded here so the next person + does not mistake it for this change. See §6. +- A **stale L2-wave parquet shadows the #627 raise.** Observed while running the + negative control: three Ethiopian waves that raise on a genuinely cold build + returned a cached `{wave}/_/cluster_features.parquet` and passed. `#627`'s own + warning ("the suite is green and that green is a lie") applies to any + data-backed test here; the config-level assertions in + `tests/test_gh323_ethiopia_config.py` are the ones that cannot be shadowed. + +## §5 Reuse decision + +| quantity | decision | reason | +|----------|----------|--------| +| Ethiopia `cluster_features` config | **reuse `3488b791` unchanged** | Already on `development`, already correct, and verified here end to end. Re-implementing would layer a second differently-shaped fix on a solved problem. | +| the cartesian census | **reuse #627's `_merge_subframes` warning** | It emits an exact phantom count; no separate oracle needed. | +| the education loss measurement | **reuse `_audit_index_collapse`'s warning** | The framework already reports destroyed and NaN-key-deleted rows by wave. Writing a bespoke counter would have been a second, unvalidated implementation of the same arithmetic. | +| `individual_education` W2/W3 index | **new (config)** — `household_id2` / `individual_id2` | Per §3: fix the identifier, never declare a reducer. No existing mechanism repairs a broken id. | +| `interview_date` W4/W5 datetime coercion | **rejected** | `fix/323-ethiopia` added `mapping.py` `interview_date` hooks. Measured on `development`: the table already returns `datetime64[us]`, 8,236 rows, 0 nulls — `_enforce_canonical_dtypes` honours the declared `Int_t: datetime` at API time. The hooks are now redundant; adding them would be inert code that reads as a fix. | +| the `2013-14` wave-level `panel_ids:` block | **deferred, not touched** | `fix/323-ethiopia` deleted it as a wrong-source (livestock-cover, holder-grain) duplicate. On `development` it now reads `i: household_id2` and the country-level `_/panel_ids.py` (`materialize: make`) is authoritative anyway. Out of #627's path; see §6. | + +## §6 Open questions for the human + +- **`individual_education`'s country-level row count is order-dependent** (§4). + The proximate cause is the lazy `self.updated_ids` probe in + `_aggregate_wave_data` (`country.py:2668`), whose `except (FileNotFoundError, + KeyError, ValueError): pass` means a first-attempt failure is recorded as + "attempted" and `id_walk` is then skipped for the rest of the process. It + affects every Ethiopian wave, in both the old and the new config, so it is not + this branch's to fix — but it means **the same call can return two different + row counts depending on what the process touched first**, which is a + correctness problem, not a performance one. Worth its own issue. +- **Ethiopia's 3 raising cells were, in #627's words, "true positives".** They + were — but they had already been fixed when #627's table was written. The + general lesson is #627's own: *a stale blocker table is a stale cache*. Nothing + in the repo re-derives that table; it is prose. +- **The 2013-14 wave-level `panel_ids:` block still points at + `sect_cover_ls_w2.dta`**, the livestock-section cover, whose grain is the + agricultural **holder** (3,812 rows / 3,670 households) and which covers only + 3,670 of the wave's 5,262 households. `data_scheme.yml` declares `panel_ids` + as `materialize: make`, so the country-level script wins and the block is + vestigial — but it is a wrong-source claim sitting in config, and it should + either be deleted or shown to be load-bearing. Not touched here: it is outside + #627's path and deleting config on a "probably unused" basis is exactly the + unevidenced move `CLAUDE.md` forbids. + +--- +### Phase 3 — verification + +- `2013-14/_/data_info.yml` + `2015-16/_/data_info.yml` (`individual_education` + idxvars) — **OK (anchored on §3/§4)**: fixes the *identifier*, per the standing + rule, rather than declaring a reducer; uses the same wave-native ids + `household_roster` and `shocks` already use for those waves, so panel linkage + via `id_walk` + `panel_ids` is unchanged. +- `tests/test_gh323_ethiopia_config.py` — **OK (anchored on §4)**: asserts at the + **wave** level, upstream of `_normalize_dataframe_index`, and deliberately does + **not** assert on post-collapse index uniqueness or on the order-dependent + country-level row count. +- `test_no_dead_aggregation_key_in_ethiopia_config` — **OK (anchored on §3)**: + pins D1 for this country, so the abandoned branch's `aggregation:` keys cannot + be revived by a future salvage attempt. +- Ethiopia `cluster_features` config — **REINVENTION AVOIDED (§5)**: the fix + exists on `development` as `3488b791`; this branch verifies it and changes + nothing. +- `interview_date` `mapping.py` hooks — **REINVENTION AVOIDED (§5)**: measured + redundant against `_enforce_canonical_dtypes`, so not salvaged. + +--- +### Measurements + +All against **PR #627's core** (`origin/development` merged with +`origin/fix/323-site4-dfs-merge`, `git merge-tree` → `59b34cc2`), asserted at +runtime via `lsms_library.__file__`, with an **isolated `LSMS_DATA_DIR`** whose +only pre-populated tier is a symlinked `dvc-cache`. + +**Negative control** — pre-`3488b791` Ethiopia config, same core: + +| wave | result | cartesian cells | phantom rows | +|---|---|---|---| +| 2011-12 | **RuntimeError** — `df_geo` lacks `lat_dd_mod` (file has `LAT_DD_MOD`) | 0 | 0 | +| 2013-14 | 65,508 rows | 1 | **60,221** | +| 2015-16 | 57,786 rows | 1 | **52,832** | +| 2018-19 | **RuntimeError** — `df_geo` lacks `lat_dd_mod` (file has `lat_mod`) | 0 | 0 | +| 2021-22 | **RuntimeError** — geo file has no `ea_id` at all | 0 | 0 | + +Reproduces #627's census exactly: 3 raising cells, 2 cartesian cells, +**113,053** phantom rows. + +**Current `development` config, same core:** + +| wave | result | cartesian cells | phantom rows | +|---|---|---|---| +| 2011-12 | 3,969 rows | 0 | 0 | +| 2013-14 | 5,262 rows | 0 | 0 | +| 2015-16 | 4,954 rows | 0 | 0 | +| 2018-19 | 6,770 rows | 0 | 0 | +| 2021-22 | 4,959 rows | 0 | 0 | + +`Country('Ethiopia').cluster_features()` → **2,168 rows on a unique `(t, v)`**, +2,146 of them carrying Latitude. Per wave: 333 / 433 / 432 / 535 / 435 clusters. + +**Full-country sweep, all 25 declared tables, same core:** `raised: 0`, +`cartesian: 0`, `dropped sub-dfs: 0`. + +**#627's own test file** (`tests/test_gh323_site4_dfs_merge.py`, 16 tests) passes +against this config tree. + +**`individual_education`**, identical conditions (`LSMS_NO_CACHE=1`, fresh +process, same script) before → after: + +| | before | after | +|---|---|---| +| country rows | 59,092 | **63,181** (+4,089) | +| 2013-14 wave frame, unique `(t, i, pid)`? | **No** | Yes (23,785 / 23,785) | +| 2015-16 wave frame, unique `(t, i, pid)`? | **No** | Yes (23,393 / 23,393) | +| `GrainCollapseWarning`s | 2 | **0** | +| W2 `(i, pid)` pairs joinable to `household_roster` | **0 / 8,505** | 12,583 / 12,583 | +| W3 `(i, pid)` pairs joinable to `household_roster` | **0 / 12,597** | 12,599 / 12,599 | + +The framework's own audit quantified the loss before the fix: + +> `Ethiopia/individual_education/2013-14: declared index (t, i, pid) is NOT +> UNIQUE. Collapsing it with groupby().first() DESTROYED 5,247 of 23,785 rows +> whose values DISAGREE (1 conflicting index tuples). ... Additionally 5,248 +> row(s) carry NaN in a declared index level and are DELETED OUTRIGHT` + +> `Ethiopia/individual_education/2015-16: ... DESTROYED 5 of 23,393 rows whose +> values DISAGREE (5 conflicting index tuples).` + +**Test negative control:** `tests/test_gh323_ethiopia_config.py` — **21/21 pass** +against the current config; **17/21 fail** against the pre-`3488b791` config tree +on the same core. (Of the 4 that still passed, 3 were the data-backed +`cluster_features` cases in waves whose raise was shadowed by a stale L2-wave +parquet — see §4; the config-level assertions failed 5/5 in every wave and cannot +be shadowed.) diff --git a/.coder/ledger/323-ethiopiarhs.md b/.coder/ledger/323-ethiopiarhs.md new file mode 100644 index 000000000..2663f2722 --- /dev/null +++ b/.coder/ledger/323-ethiopiarhs.md @@ -0,0 +1,194 @@ +# GH #323 — EthiopiaRHS: silent collapse of a non-unique declared index + +Branch `fix/323-ethiopiarhs`, base `development` @ `d572d8a9`. + +## §1 What the brief said, and where it was stale + +The dispatched diagnosis said `_normalize_dataframe_index` applies +`groupby().first()` to EthiopiaRHS `food_acquired`, destroying 131 rows of real +Quantity/Expenditure, and prescribed (1) a source-level dedup and (2) declaring +`aggregation: sum`. + +**Half of that was already fixed and the sign of the other half was inverted.** +`food_acquired` has been in `feature.py::_ADDITIVE_MEASURE_COLUMNS` since GH +`#501`/`#514` (`899b89c6`, `d8ff08d5`), so the collapse already **SUMs** rather +than `first()`s. Measured on the L2-wave parquets vs. the API: + +| wave | L2-wave Qty | API Qty | Δ | +|-------|-------------|---------|---| +| 1989 | 14020.6 | 14020.6 | 0 | +| 1994a | 116829.4 | 116829.4 | 0 | +| 1994b | 70860.2 | 70860.2 | 0 | +| 1997 | 86044.1 | 86044.1 | 0 | + +Mass **preserved** ⇒ the sum path is live ⇒ the 131 "destroyed" rows were +already being folded in correctly. What the brief missed is the consequence: +**`sum` DOUBLE-COUNTS a row that was punched twice.** + +## §2 What was actually broken + +Instrument validated first on the mandated positives (Mali/2014-15 +`household_roster` = 32,026 dup rows; Guyana/1992 `housing` = 311) before any +EthiopiaRHS number was trusted. + +**Channel A — double-count (class-1, silently WRONG).** 90 rows across the five +roster waves are byte-identical in the canonical grain *and* in both measures. +The framework sums them, so they are counted twice: **514.9 Quantity and 343.1 +Birr of pure overstatement.** + +Smoking gun, end to end: `food89.dta` rows 865/866 are identical in *every* +source column (hh 20120, foodcode 34, qty 2.0, unit 9, value 0.5). The API +reported **Quantity 4.0 / Expenditure 1.0**. The source records one line. + +The other 127 colliding rows genuinely DIFFER (1997 hh 10_93 produced Berbere +1.0 kg *and* 30.0 kg). Those are real repeat measurements and `sum` is right for +them. So dedup and sum are a **matched pair** — neither is correct alone. + +**Channel B — NaN index keys silently DELETED (new; not in the brief).** +`groupby(level=..., observed=True)` defaults to `dropna=True`, so any row with a +NaN in an index level **vanishes** during the collapse. It only bites when the +index is non-unique (otherwise no groupby runs), which is why it hid inside +Channel A. EthiopiaRHS 1995 lost 11 rows (20.2 Qty, 23.7 Birr) this way — the +wide melt path filtered missing `i` and `u` but forgot `j`. + +This model is **exactly predictive**: `rows − dup − nan_key` reproduces the +observed API row count for all five waves (2068 / 12971 / 13333 / 11956 / 14365). + +## §3 The fix + +**Class level** (`country.py`) — the part that matters for #323. +`SkunkWorks/grain_aggregation_policy.org` introduced an `aggregation:` block in +`data_scheme.yml`; ten countries declare one; **nothing read it.** It was +documentary prose while the code applied `.first()` regardless — the repo's own +"prose is not enforcement" failure mode. `_normalize_dataframe_index` now +**honours** it (`_declared_aggregation`), with precedence +declared → hardcoded additive map → `first`. An unknown reducer raises rather +than degrading to `first()`. Undeclared collapses stay loud, and +`LSMS_STRICT_INDEX=1` turns them into an error — the enforcement lever. + +Proof it was inert on base: with `aggregation: {Quantity: sum}` declared and +rows 1.0 + 30.0, base returns **1.0**. + +**Country level** (`EthiopiaRHS/_/ethiopiarhs.py`, `_/data_scheme.yml`). +`_drop_double_punched()` drops rows indistinguishable in the canonical grain, +*before* the framework sums; the wide path now also filters NaN `j`, converting +an accidental pandas deletion into an intentional, symmetric drop. +`aggregation: {Quantity: sum, Expenditure: sum}` is declared explicitly rather +than inherited from the hardcoded map. + +## §4 Numbers + +Cold rebuild (`cache clear --country EthiopiaRHS`; the L2-country parquet is +written POST-collapse, so a warm run shows the poisoned cache and appears to +pass). + +| wave | API rows before → after | Qty before → after | Exp before → after | +|-------|-------------------------|--------------------|--------------------| +| 1989 | 2068 → **2068** | 14020.6 → 14018.6 (−2.0) | 10094.8 → 10094.3 (−0.5) | +| 1994a | 12971 → **12971** | 116829.4 → 116745.5 (−83.9) | 50267.6 → 50243.5 (−24.1) | +| 1994b | 13333 → **13333** | 70860.2 → 70689.5 (−170.7) | 58759.7 → 58706.1 (−53.6) | +| 1995 | 11956 → **11956** | 56950.9 → 56791.6 (−159.3) | 44405.8 → 44300.8 (−105.0) | +| 1997 | 14365 → **14365** | 86044.1 → 85945.1 (−99.0) | 72176.8 → 72016.8 (−160.0) | + +Row counts unchanged (the grain did not move); the mass falls by *exactly* the +double-counted amount. Byte-identical dupes 90 → **0**; the 127 real repeat +measurements are retained and summed; NaN-`j` 11 → **0**. 1989 Fenugreek now +reports **2.0 / 0.5**, matching source. + +**Regression.** `_normalize_dataframe_index` was run over every affected cell +under old and new code, with the config tree *and* a frozen parquet snapshot +pinned (the shared cache is being mutated by other agents — Panama/Niger cells +appeared and vanished mid-run, which is what made a first, unpinned comparison +untrustworthy): **719 cells / 31 countries / 27,085,429 rows → 0 output +changes.** Includes the nine countries whose `interview_date: {visit: first}` +policy is now live; `first`-for-every-column is byte-identical to the old +`.first()` (NaN-skipping included, verified). + +## §5 Judgment call, stated plainly + +Treating the 90 byte-identical rows as double-punches rather than as genuine +repeat acquisitions of identical amount+price+unit+source is **not settleable +from the data** — the instrument records no axis on which the two rows differ. +I took the conservative reading and drop. + +If that call is wrong, it **understates** by 90 rows (class-2, silently +missing). Summing them **overstates** (class-1, silently wrong). Class-2 is the +safer error, and for rows identical on every recorded axis, asserting "two +distinct events" is the stronger and less defensible claim. The 1989 case is the +tell: rows 865/866 are identical in *unrelated household-level fields too*, +which is a data-entry signature, not an economic one. + +The load-bearing negative: **there is no missing level to name.** All 27 columns +of the q36 module were enumerated — no line / transaction / occasion / visit id. +`q36_1a` is a section flag (1.0/NaN/2.0), not a line number; adding it to the key +moves 1994a's dup count only 213 → 212. Unlike Guyana (where `SN` was the missing +level), `(t,i,j,u,s)` **is** the maximal grain the instrument supports, so a +synthetic row-order id would be a meaningless axis and would break cross-country +comparability. + +## §6 What I did NOT fix, and why + +**The class is only PARTLY closed. Do not close #323 on this branch.** + +1. **Channel B is repo-wide and unfixed: 710,845 rows** with a NaN index key are + silently deleted by `groupby(dropna=True)` across ~28 countries. The one-line + fix (`dropna=False`) would *restore* those rows and thereby change many + countries' outputs — it cannot be validated country-by-country from an + EthiopiaRHS remit, and shipping it here would violate "prove you broke nothing + else." **It needs its own issue.** I fixed only EthiopiaRHS's 11, at source. + +2. **The `first()` default still stands for undeclared tables.** A repo-wide scan + of L2-wave parquets on the full declared index finds **99 cells / 24 countries + where colliding rows genuinely DIFFER (477,499 rows silently wrong)**. Making + an undeclared collapse `raise` by default — which is the correct end state — + breaks all 24 at cold build. This branch ships the *mechanism* (declare a + policy) and the *lever* (`LSMS_STRICT_INDEX=1`); flipping the default requires + declaring a policy for each of those countries first. Per-country damage: + Burkina_Faso 184,308 · Mali 68,666 · Malawi 67,137 · Nigeria 58,782 · Uganda + 20,317 · Tanzania 20,102 · Tajikistan 11,540 · Albania 9,514 · South Africa + 8,454 · Kosovo 6,702 · India 6,194 · Guyana 4,545 · Liberia 2,954 · Kazakhstan + 1,861 · Niger 1,815 · Benin 1,302 · Cambodia 1,260 · China 758 · Togo 579 · + Senegal 208 · Ethiopia 207 · Serbia 163 · EthiopiaRHS 127 · CotedIvoire 4. + (That scan does not separate *intended* level-drop aggregation from the bug; + it is an upper bound on the cells needing a declared policy, not on the bug.) + +## §7 Prior art consulted + +`SkunkWorks/grain_aggregation_policy.org` (the `aggregation:` contract and its +"NO AGGREGATION IN CORE" direction — this change does not contradict it; it makes +the *interim* collapse honest), `feature.py::_ADDITIVE_MEASURE_COLUMNS` + +`_collapse_duplicate_index` (GH #501/#514 — reused as the fallback rather than +duplicated), CLAUDE.md cache tiers (why verification must be cold). + +--- + +## Stripped to config-only (2026-07-13, GH #323 consolidation) + +This branch (`fix/323-ethiopiarhs-config`, off `origin/development`) carries the +country work from `fix/323-ethiopiarhs` and **nothing else**. Per +`slurm_logs/DESIGN_grain_collapse_sites_2026-07-13.org`: + +* **Stripped: 129 lines of `lsms_library/country.py`** (the Design-A + `_declared_aggregation` reducer machinery) and `tests/test_index_collapse_policy.py` + (which exercised it). **D1: core does not aggregate** — Design B (PR #614) won. +* **Stripped: the `aggregation: {Quantity: sum, Expenditure: sum}` block** in + `_/data_scheme.yml`. It was always a **NO-OP**: `food_acquired` is in + `feature.py`'s `_ADDITIVE_MEASURE_COLUMNS`, so core *already* sums those two + columns when the canonical index is non-unique. Prose in `CONTENTS.org`, + `data_scheme.yml` and the `_drop_double_punched` docstring that described the + sum as *declared* has been corrected to say it is *core's, and unconditional*. +* **Kept, and it stands alone:** `_drop_double_punched()` and the `j.notna()` + filter in `_/ethiopiarhs.py`. The dedup is what makes core's (pre-existing) sum + *correct*; it does not depend on the stripped machinery in any way. + +Verified post-strip, cold build (`LSMS_NO_CACHE=1`), branch vs. `origin/development`: + +| | Quantity | Expenditure | +|---|---|---| +| development | 344,705.2 | 235,704.6 | +| this branch | 344,190.4 | 235,361.5 | +| **removed** | **514.8** | **343.1** | + +Exactly the claimed double-punch overstatement (90 rows). Row count is unchanged +(54,693) because core was *summing* the punch-duplicates into the same index +tuple, not adding rows — the defect inflated values, not the shape. diff --git a/.coder/ledger/323-framework.md b/.coder/ledger/323-framework.md new file mode 100644 index 000000000..c77dbd8e7 --- /dev/null +++ b/.coder/ledger/323-framework.md @@ -0,0 +1,155 @@ +# Prior-Art Ledger — GH #323 (framework / class fix) + +**Search tier used:** ripgrep + git + direct measurement against the L2-wave parquet tier. + +## §1 Task, restated + +`country._normalize_dataframe_index` collapses a non-unique **declared** index with +`groupby(level=...).first()`. Where the duplicate rows carry *different* values, the +dropped rows are real data and vanish with no signal. This is the ROOT of #323: the +per-country index fixes close *instances*; this closes the *class*. + +The class must become **impossible to hit silently** — the collapse is either +lossless (proved), or it is LOUD, and it survives the cache that previously hid it. + +## §2 Existing machinery (do not reinvent) + +| symbol | path:line | what it does | reuse / extend / new | +|--------|-----------|--------------|----------------------| +| `_normalize_dataframe_index` | `country.py:4100` | reorder/drop levels; `groupby().first()` collapse at 4197-4210 | **extend** | +| `_ADDITIVE_MEASURE_COLUMNS` | `feature.py:101` | `{"food_acquired": ("Quantity","Expenditure")}` — the ONE table with a real reduction policy | **reuse as-is** | +| `_collapse_duplicate_index` | `feature.py:106` | the second `.first()` site (Feature assembly) | **extend (same audit)** | +| `melt_visit_intervals` | `local_tools.py:2150` | landed recipe for `interview_date` multi-visit (Malawi, `3d7a7c61`) | reuse — NOT my job | +| `cache_freshness` / `stamp_parquet_hash` / `to_parquet(cache_hash=)` | `local_tools.py:1486/1507/1541` | v0.8.0 content-hash L2 staleness; embeds `lsms_cache_hash` in parquet schema metadata | **extend** — same mechanism carries the grain audit | +| `LSMS_CACHE_SCHEMA` | `local_tools.py:1354` | manual library-version cache-invalidation lever | **bump 1 -> 2** | + +## §3 Definitions & conventions in force + +- **NO AGGREGATION IN CORE** — `SkunkWorks/grain_aggregation_policy.org` §"The contract". + The access path (`country.py`, `feature.py`, `local_tools`) never reduces grain; all + aggregation is analyst-invoked in `transformations.py`. Step 1 of 5 landed (PR #471, + `u`-in-index). Steps 2-5 pending. **This ledger's fix must not contradict it.** +- Precedent from the only landed step: **PR #471 fixed `crop_production` by ADDING `u` + to the index — not by aggregating over it.** +- `aggregation:` in `data_scheme.yml` — **DEAD CONFIG, zero consumers** (grep: + `diagnostics.py:174` and `country.py:2387` both list it in a `_skip` set). All 9 + declarations are `interview_date: {visit: first}`, and all 9 tables **already declare + `visit` in the index** — so they have no duplicates and the collapse never fires for + them. It is a legacy-reproduction shim (`senegal.py:262`), NOT a grain policy. + +## §4 Invariants & assumptions (landmines) + +- **The L2-COUNTRY parquet (`var/`) is written POST-collapse.** The L2-WAVE parquet + (`{wave}/_/`) holds the truth. Any scanner run against `var/` returns a false zero. +- **The #323 warning is structurally unable to fire warm.** It is gated on + `not df.index.is_unique`; the cached frame is already collapsed, so the gate is + never true. *The bug hides behind the cache that the bug poisoned.* Every existing + instrument sits downstream of the destruction (`diagnostics._check_duplicate_index` + reports "pass"). +- `.pth` trap: `.venv/.../lsms_library.pth` pins imports to the MAIN checkout; + `PYTHONPATH` does **not** override it (verified). Only cwd-as-`sys.path[0]` + (`cd $WT` + `python -c`/`-m`) or an explicit `sys.path.insert` wins. Assert it. +- `yaml.safe_load` **throws on the `!make` tag** in `data_scheme.yml`. Use + `lsms_library.yaml_utils.load_yaml`. A blanket `except: continue` around it silently + turns every country into a zero — this cost me one broken scanner (§6). + +## §5 Reuse decision + +- Duplicate detection + reporting: **new** (`_audit_index_collapse`), because no + existing check runs *upstream* of the destruction. +- Persistence of the signal: **reuse** the v0.8.0 parquet-schema-metadata mechanism + (`lsms_cache_hash`) — add a sibling key `lsms_grain_audit`. Do not build a sidecar. +- Additive policy: **reuse** `_ADDITIVE_MEASURE_COLUMNS` unchanged. +- `transformations.collapse()`: **NOT built here** (design step 4). Out of scope. + +## §6 Measurement (this is the load-bearing part) + +Instrument **validated on the known positives before any zero was trusted**: +Mali/2014-15/household_roster -> 32,026; Guyana/1992/housing -> 311. First scanner +returned 0/0 (broken: `safe_load` + `Data Scheme` key) and was rebuilt. + +Census over the L2-**wave** tier, simulating the core exactly (reorder -> drop +undeclared levels -> count dups): **142 cells / 31 countries / 7,501,053 dropped rows.** + +The row count alone is a **misleading instrument**. Splitting by whether the duplicate +rows actually *disagree*: + +| verdict | cells | dropped rows | rows destroyed | +|---|---|---|---| +| **DESTRUCTIVE** (duplicate rows conflict) | 89 | 1,040,342 | **542,114** | +| redundant (duplicate rows identical -> `first()` is a lossless dedup) | 53 | 6,460,711 | 0 | + +6.46M of the 7.5M "dropped" rows are a **lossless de-dup** (e.g. a cluster attribute +repeated once per household in the cluster). Warning on raw row counts would bury the +real 542k under 6.5M false alarms — *that is precisely how a warning becomes noise +nobody reads*, which is how #323 died the first time. + +**Third silent-loss path, previously unreported:** `groupby(level=...)` defaults to +`dropna=True`, so any row with **NaN in a declared index level is DELETED OUTRIGHT** by +the collapse — over and above it. 14 cells, **485,231 rows**. Worst: +`Burkina_Faso/food_acquired/2014` loses 460,438 of 557,822 rows (82.5%). This fires on +the additive branch too. Reported loudly here; **behaviour deliberately unchanged** +(see §7). + +Declared-but-**absent** index level (the silent-narrowing chain at `country.py:4143-4152`): +**0 real cells.** The 20 apparent hits were false positives of my probe — `map_index()` +renames the legacy `j` index to `i`. So the guard is free to add. + +## §7 Decisions, and what I deliberately did NOT do + +1. **Did NOT wire `aggregation:` into the core collapse.** It contradicts the + NO-AGGREGATION-IN-CORE contract, and it does not fix the headline case: Mali's `pid` + is a *household* id stamped on every member (5,149 distinct values over 37,175 rows, + 3,335 households with `pid.nunique()==1`), so **no reducer is correct** — `first()` + keeps one person per household; `sum` is meaningless on `Sex`. A declared + `aggregation` there would only *put a signature on the corpse*, converting a + silently-wrong bug into a silently-wrong bug **with paperwork**. Duplicates on a + declared index mean the **identifier is broken** or a **level is missing** — the core + now says exactly that, and refuses to be declared away. +2. **Did NOT change `dropna` behaviour** for the NaN-key deletion. Restoring 485k rows + (incl. +460k to a Burkina food_acquired total) is a *data* change needing per-country + validation; doing it in the same diff that is supposed to make data changes *visible* + would make the diff unreviewable. It is now LOUD and strict-mode-fatal instead. + Follow-up issue. +3. **Signal survives the cache** — the one line that matters. Reports are embedded in + the L2-country parquet schema metadata (`lsms_grain_audit`) at write time and + **re-emitted on every warm read**. `LSMS_CACHE_SCHEMA` 1 -> 2 forces the existing + poisoned caches to rebuild once, so the fix actually fires on machines where the bug + is already baked in. Without that bump the fix would be invisible exactly where the + bug lives. +4. **The cache bump surfaces a pre-existing CotedIvoire bug (NOT caused by this diff).** + `LSMS_CACHE_SCHEMA` 1 -> 2 forces one rebuild of every L2-country parquet, which + acts as a repo-wide `--rebuild-caches`. That makes two tests fail: + `test_table_structure.py::{test_declared_columns_present,test_feature_is_sane}[CotedIvoire/cluster_features]` + — *"declared column 'Latitude' not found in ['Region','Rural','t','v']"*. **Both + reproduce identically on pristine `development` under `LSMS_NO_CACHE=1`**, so the + defect is pre-existing: CotedIvoire's `cluster_features` config no longer produces + the `Latitude`/`Longitude` it declares, and a **stale L2-country parquet was still + serving the old columns and keeping the test green.** Same pattern as Uganda #245 + (CLAUDE.md: "the bug stayed hidden behind a stale country-level cache"). Needs a + per-country fix + its own issue; deliberately NOT fixed here (out of scope for the + framework agent, and the per-country agents are editing those trees concurrently). + +### Test status (honest) + +- `tests/test_grain_collapse.py` — **14 new tests, all pass**; all fail on pre-fix code. +- A **2,292-test subset** (70% of the 3,272 collected), incl. `test_table_structure` + across every country, `test_feature`, `test_sample`, `test_conversion`, + `test_join_v_silent_skip_warn`, `test_uganda_v_grain_invariants`, + `test_add_market_index_dedup`: **2,290 passed, 2 failed** — both the pre-existing + CotedIvoire failures in §7.4, proven on pristine `development`. +- `test_cache_hash_invalidation` + `test_build_transform_hash` + + `test_canonical_shape_via_cache_miss`: **50 passed, 1 xpassed.** +- **The full 3,272-test run did NOT complete** before this was written: the box was + carrying ~20 concurrent pytest processes from the parallel per-country agents, and + the schema bump makes the *first* run after this change pay a one-time cache + rebuild per country. **Not claimed as green.** It must be run to completion on a + quiet machine before merge. +- `tests/test_currency.py::test_feature_ghana_per_wave` (**GH #589**) fails on + pristine `development` — confirmed identically; not touched. + +5. **Warn by default, raise under `LSMS_GRAIN_STRICT=1`.** Raising by default detonates + 31 countries and gets reverted; warning alone is how we got here. The warning is + high-precision (89 cells, not 142) and cache-surviving; strict mode lets CI/tests + ratchet. **No allowlist of known-bad cells** — a known-bad cell stays loud until it + is fixed. diff --git a/.coder/ledger/323-guatemala.md b/.coder/ledger/323-guatemala.md new file mode 100644 index 000000000..9720e6748 --- /dev/null +++ b/.coder/ledger/323-guatemala.md @@ -0,0 +1,152 @@ +# Prior-Art Ledger — GH #323 (Guatemala) + +**Search tier used:** ripgrep + git floor (gitnexus not consulted; the work is +config-tree + one country module, and the blast radius was established directly +by rebuilding every Guatemala table before/after). + +## §1 Task, restated + +`_normalize_dataframe_index` (`lsms_library/country.py:4175`) collapses a +non-unique **declared** index with `groupby().first()`. For Guatemala the +affected cell is `2000 / cluster_features`: the wave frame carried 7,276 rows +(one per household) on a declared `(t, v)` index, and 7,268 of them were +discarded to reach 8 rows. + +The declared cluster key was `v: region` — 8 Guatemalan regions. That is +**coarser than the geography that determines the declared `Rural` column**: all +8 regions contain both urban and rural households. So `Rural` was not a function +of `v`, and the `.first()` collapse was not a dedup but an **arbitrary pick by +row order** — it stamped 7 regions "Urban" and `suroccidente` "Rural", leaving +**3,591 of 7,276 households (49.4%)** in a cluster whose `Rural` flag +contradicted their own. This is **class-1 silently WRONG**, not class-2 missing. + +A co-defect: `cluster_features`' idxvars also carried `i: hogar`, which is not +part of the declared `(t, v)` index — the proximate reason the frame was +household-level for a cluster-level table. + +## §2 Existing machinery (this task's area) + +| symbol | path:line | what it does | tested? | reuse / extend / new | +|--------|-----------|--------------|---------|----------------------| +| `_normalize_dataframe_index` | `lsms_library/country.py:4100` | reorders/drops index levels; collapses duplicates via `groupby().first()` (or `sum` for `_ADDITIVE_MEASURE_COLUMNS`), warning "GH #323" | yes (indirectly) | **untouched** — see §6 | +| `_join_v_from_sample` | `lsms_library/country.py:2134` | joins `sample.v` into household-level tables at API time | yes | reuse (constrains the fix: `sample.v` and `cluster_features.v` must move together) | +| `df_data_grabber` | `lsms_library/local_tools.py:1018` | "Trickier" form `{newvar: (list_of_cols, fn)}` builds a composite key from several columns | yes | **reuse** — this is how the composite `v` is built | +| `benin.i()` | `countries/Benin/_/benin.py:6` | precedent: composite household id from a list value in `data_info.yml` | yes | **reuse the pattern** for `guatemala.v()` | +| `guatemala.individual_education(df)` | `countries/Guatemala/_/guatemala.py` | precedent: a `df_edit` hook named after a declared table | yes | **reuse the pattern** for `cluster_features(df)` | +| `Country.column_mapping` | `lsms_library/country.py:704` | binds a country-module function to a var when the names match and the var isn't a declared table | — | reuse (makes `v: [depto, mupio, sector, segmento]` work in both `sample.myvars` and `cluster_features.idxvars`) | + +## §3 Definitions & conventions in force + +- `sample` is the single source of truth for household→cluster mapping; + `v` belongs in `cluster_features`' index and nowhere else — per `CLAUDE.md` + §"`sample()` and Cluster Identity". +- `cluster_features` canonical index is `(t, v)` — `countries/Guatemala/_/data_scheme.yml`. +- Categorical columns from `.dta` come back as pandas categoricals; YAML mapping + keys must be *string* keys when the raw labels are strings (`'urbana': Urban`) + — `CLAUDE.md` §"Gotchas with Teeth". (`area` is exactly such a column.) +- `format_id` is auto-applied to `idxvars` but **not** to `myvars` — but the + named-function branch in `column_mapping` takes precedence for a *list* value, + so `guatemala.v()` runs in both positions. + +## §4 Invariants & assumptions + +- **`sample.v` and `cluster_features.v` must change together.** + `_join_v_from_sample()` propagates `sample`'s `v` into every household-level + table; a one-sided edit silently breaks the v-join. (`country.py:2134`) +- **A cluster-level column must be a function of `v`.** If it is not, collapsing + to one row per `v` cannot be lossless — which is the whole #323 defect. Now + *enforced*, not assumed: `guatemala.cluster_features()` raises if any payload + column varies within `(t, v)`. +- **`upm` in `CONSUMO5.DTA` is float32-corrupted and must NOT be used** (§5). + +## §5 Reuse decision + +| quantity | decision | reason | +|----------|----------|--------| +| cluster key `v` | **new** (`guatemala.v()`), built on the existing Benin composite-key pattern | ENCOVI 2000 *does* have a PSU — see below. No existing helper composes it. | +| `cluster_features` dedup | **new** df_edit hook, guarded | The framework's generic `groupby().first()` is exactly what must not run silently here. | +| `strata` | **unchanged** (`region`) | Could not be determined; see §6. Not guessed. | + +### The PSU: found, and the trap inside it + +`CLAUDE.md` asserted *"Guatemala | No PSU/cluster variable in data"*, and the +independent diagnosis for this task concluded *"NO FINER PSU EXISTS"*, proposing +a `region × area` (16-cell) pseudo-cluster. **Both are wrong.** They checked +`HOGARES` / `ECV02H01` / `ECV03H01` / `ECV04H02` / `PERSONAS` / `ECV40P18`, which +carry only `region` + `area`. + +`CONSUMO5.DTA` is household-level (7,276 rows; its `hogar` set is *identical* to +`ECV01H01`, so it joins 1:1) and carries `upm` — *Unidad Primaria de Muestreo* — +plus the full hierarchy `depto` / `mupio` / `sector` / `segmento` and `estrato`. + +**But the raw `upm` column must not be used.** Stata stored it as a `float` +(IEEE single precision) and every value lies in ~1.0e8–2.2e9, far above float32's +exact-integer limit of 2^24 = 16,777,216. The float32 ULP there is 8–256, which +rounds away the low-order digits encoding `segmento`: + +- 1,065 real geographic cells → only **847** distinct stored `upm` values; +- **201** `upm` values conflate ≥2 genuinely different PSUs (**2,128 households**); +- e.g. `segmento` 16 and 32 in sacatepequez/mupio 1/sector 9 both store as `301100928`. + +Reading `upm` back would therefore silently *merge* clusters — a smaller instance +of the very bug being fixed. The faithful reconstruction is the composite +`depto-mupio-sector-segmento` (components are small ints that float32 holds +exactly). Verified on the 7,276-household frame: + +| property | composite | raw `upm` | +|---|---|---| +| clusters | **1,065** | 847 (218 identities destroyed) | +| cells spanning >1 region | **0** | 0 | +| cells containing both urban+rural | **0** | 0 | +| cells with >1 distinct design weight | **0** | **17** | + +The last row is the decisive independent check: in a two-stage design the design +weight is constant within a PSU. It is **exactly constant within all 1,065** +composite cells, and is *not* for the corrupt `upm`. The composite also strictly +refines `upm` (no composite cell spans two `upm` values). Median 6, max 16 +households per cluster — the shape of a real enumeration area. + +Re-pointing `sample` / `cluster_features` from `ECV01H01.DTA` to `CONSUMO5.DTA` +changes **no existing value**: the two files agree exactly on `hogar`, `factor`, +`region`, `area` (max abs diff 0.0). It only *adds* the geography. + +## §6 Open questions for the human + +- **`strata` is still `region`, and that is very likely incomplete.** + `CONSUMO5.DTA` carries `estrato` (6 levels, constant within every PSU). + Within-stratum design-weight dispersion (as a fraction of total weight + variance): `region` 0.725 · `estrato` 0.946 · `region × area` 0.623 · + `region × estrato` **0.399** · the PSU itself 0.000. So `region × estrato` is + clearly *closer* to the design stratum than `region` — but **no** candidate + makes the weight constant within-stratum, so the true stratum cannot be + identified from the data alone. Left as `region` and flagged in the YAML rather + than replaced with an unverifiable guess (non-negotiable #4: do not guess + quietly). Resolving it needs the ENCOVI 2000 sample-design documentation. + `strata` is not part of the declared index and did not cause the #323 collapse. + +- **The CLASS fix is deliberately NOT in this branch.** `_normalize_dataframe_index` + should refuse to collapse a non-unique *declared* index with `.first()` when any + non-index column is non-constant within a group (Guatemala is the textbook case: + `Rural` varied within `v` in 8/8 groups). But that is shared framework code, and + ~14 countries still carry the bug: making it raise today converts their silent + wrongness into hard failures all at once. That must land as one coordinated + change *after* the per-country cells are fixed, not unilaterally from a + Guatemala worktree. Flagged here so it is not lost — closing the instance while + leaving the class is how #323 got closed the first time. + +--- +### Phase 3 — verification + +- `guatemala.v()` — **OK (anchored on §2/§5)**: reuses the Benin composite-key + pattern via `df_data_grabber`'s "Trickier" form; no existing helper composed a + PSU from geographic components. +- `guatemala.cluster_features()` — **OK (anchored on §4)**: does not reinvent the + framework collapse — it *forbids* the unguarded one, raising if any payload + column is non-constant within `(t, v)` instead of picking arbitrarily. +- `2000/_/data_info.yml` (`sample`, `cluster_features`) — **OK (anchored on §4)**: + both `v`s changed together, as `_join_v_from_sample` requires; `i: hogar` + removed from `cluster_features` idxvars. +- `_/data_scheme.yml` — **OK**: `Region` is no longer `optional` (it is now a real, + well-defined column); stale "region is used as the v-index itself" note removed. +- `CLAUDE.md` — **CONTRADICTION corrected (§5)**: the "No PSU/cluster variable in + data" row was false and is precisely why nobody looked in `CONSUMO5.DTA`. diff --git a/.coder/ledger/323-guinea-bissau.md b/.coder/ledger/323-guinea-bissau.md new file mode 100644 index 000000000..53a03171c --- /dev/null +++ b/.coder/ledger/323-guinea-bissau.md @@ -0,0 +1,208 @@ +# GH #323 — Guinea-Bissau (the CONTROL) + the class-level fix + +Task: `fix/323-guinea-bissau`. Branch off `development`. +Cell assigned: `2018-19 / cluster_features`, declared index `(t, v)`, 5,410 rows -> 450. + +## §1 What was asked + +Close the Guinea-Bissau instance of #323 (`_normalize_dataframe_index` silently +collapsing a non-unique declared index with `groupby().first()`), **without** +leaving the class alive — the failure mode that got #323 closed the first time. + +## §2 Instrument validation (done FIRST, per the brief) + +A scan of the L2-**country** parquets (`{C}/var/`) is worthless: those are written +POST-collapse, so the evidence is already destroyed. All scanning ran against the +L2-**wave** parquets. + +My first scanner returned a false negative on 16 countries — including Mali — +because `yaml.safe_load` raises `ConstructorError` on the `!make` tag and a bare +`except: continue` swallowed it. Fixed with a multi-constructor loader; then +calibrated against the two known positives before trusting any zero: + +| calibration cell | required | measured | +|-------------------------------------|----------|----------| +| Mali/2014-15/household_roster | 32,026 | **32,026** | +| Guyana/1992/housing | 311 | **311** | + +`Country('Mali').household_roster()` legacy per-wave count for 2014-15: **5,149** +— reproducing the brief's own API-level number exactly. + +## §3 Diagnosis — Guinea-Bissau is BENIGN (reproduced from the raw .dta) + +The 4,960 duplicate rows decompose exactly into two terms, both **byte-identical +repeats**, neither a distinct entity: + +- **TERM 1 (4,901)** `cluster_features` builds `df_main` from `s00_me_gnb2018.dta` + — the household COVER PAGE: 5,351 households, 0 duplicate households, over 450 + grappes. But it declares only `idxvars: v: grappe`, so it projects a + household-grained file onto a cluster-grained index and each household emits a + row carrying ITS CLUSTER's attributes (`Region` s00q01, `Rural` s00q04). Both + are **perfectly constant within grappe at source** (verified: 0 grappes with >1 + distinct value on either). 5,351 − 450 = 4,901. +- **TERM 2 (59)** `grappe_gps_gnb2018.dta` has 450 rows but only 445 unique + grappes: 5 records are byte-identical duplicates (same grappe, vague, Lat, Lon, + Accuracy, Altitude **and Timestamp** — an export artifact in the WB-released + file, not two readings). `merge_on: v` fans those 5 grappes out ×2; they + contain exactly 59 households. + +5,351 + 59 = **5,410** (observed, to the row). 5,410 − 450 = **4,960** (the +reported duplicate count, to the row). + +**The collapse is provably lossless.** `drop_duplicates(FULL ROW)` → 450; +`drop_duplicates(KEY t,v)` → 450. Equal. 0 clusters carry >1 distinct +`Region` / `Rural` / `Latitude` / `Longitude`. `groupby().first()` is here the +IDENTITY function. **rows_recovered = 0 — 450 is already the correct answer.** + +Guinea-Bissau's value to #323 is as a **CONTROL**: it is the case that proves the +fix must *dedup-then-verify*, not *dedup-then-guess*. + +RULED OUT: not INDEX_INCOMPLETE — adding `i` would turn `cluster_features` (the +table that OWNS `v` per CLAUDE.md) into a household table and break +`_add_market_index` + the v-join for the whole country. Not PHANTOM_NAN_ROWS +(0 NaN `v`; all 450 clusters present). Not INTENDED_AGGREGATION (nothing is +aggregated — the values are constant, so there is no reducer to choose). + +## §4 The class, measured + +Across every cached L2-wave parquet (119 collapsing cells): + +| | rows | +|---|---| +| destroyed today by `groupby().first()` | **7,244,929** | +| …byte-identical repeats (collapse LOSSLESS) | 6,783,753 (93.6%) | +| …**CONFLICTING payloads (REAL silent loss)** | **461,176** (6.4%) | + +So 93.6% of the collapse is harmless and the existing #323 warning cries wolf over +it — which is part of why the real 6.4% stayed invisible. + +## §5 The fix + +`country.py::_resolve_duplicate_index` (new), called from +`_normalize_dataframe_index`. Three paths: + +1. **ADDITIVE** (`food_acquired`) — unchanged declared SUM reducer. + **Deliberately NOT de-duplicated.** *The trap*: two byte-identical + `food_acquired` rows are two REAL transactions; dropping one would silently + HALVE the household's quantity/expenditure. Benin has 145 such rows. A naive + "dedup first" would have turned this fix into a new class-1 (silently wrong) + bug. Verified: 14/14 additive cells byte-identical. +2. **DEDUP** (lossless) — drop rows byte-identical on key AND payload. Provably + output-identical to `.first()`, which ignores row multiplicity (verified + empirically). Guinea-Bissau is entirely this case: 5,410 → 450, silently. +3. **VERIFY** (loud) — anything still duplicated carries CONFLICTING payloads, so + any collapse destroys information → **raise `DuplicateIndexError`**, naming the + table, wave, key, disagreeing columns, row count and example keys. + `LSMS_INDEX_COLLAPSE=warn` restores the legacy lossy behaviour for triage. + +Also preserved: `groupby(dropna=True)`'s NaN-key drop (step 1b). Without it the +dedup path *resurrected* a NaN-keyed row in 2 cells (GhanaLSS 2016-17 +`food_security` +1, CotedIvoire 1988-89 `individual_education` +1), changing two +countries I do not own and injecting NaN into the index. Caught by the regression +harness; see §7. + +### Why RAISE, and why that is not overreach +`SkunkWorks/grain_aggregation_policy.org` — the repo's own contract — states: +> "The composition/access path — `country.py` (`_finalize_result` / +> `_normalize_dataframe_index`) … **NEVER reduces grain**." +and names this exact site: *"Both core `.first()` collapses are the GH #323 / #325 +silent-data-loss footguns."* A silent grain reduction here is a contract +violation, and "loudly BROKEN beats silently WRONG." + +### Cache poisoning — why the fix actually reaches users +The #323 warning only ever fired on a COLD build; the collapse was then baked into +the L2-country parquet, so the bug hid behind the cache it had poisoned. +`_normalize_dataframe_index` is decorated `@build_transform()`, and +`_build_registry._closure_parts` folds the **normalised AST of the function *and +every global it calls*** into every table's cache fingerprint. So the new helper +is automatically in the hash: changing the logic invalidates every poisoned +parquet. **No `LSMS_CACHE_SCHEMA` bump needed** — verified empirically (Mali +raised straight through a warm, poisoned cache). + +## §6 `aggregation:` is inert prose (reported, not used) +`aggregation:` is declared in 9+ countries' `data_scheme.yml` and read by +**nothing** (`grep` for consumers: zero). It is parsed only as a skip-key. Per +the design doc it belongs to an explicit `transformations.collapse()` (step 4, +unimplemented) — NOT to normalize-time reduction. So a sibling agent told to +"declare it in `aggregation:`" would ship a no-op. Deliberately NOT wired in here. + +## §7 Regression evidence +Behaviour changes ONLY inside the `not df.index.is_unique` branch, so the blast +radius is exactly the 119 collapsing cells. Legacy vs new, on the real wave +parquets, all 119: + +| | cells | +|---|---| +| IDENTICAL (14 additive + 34 lossless-only) | **48** | +| RAISE (the genuine #323 instances) | 85, across 23 countries | +| **REGRESSIONS (output changed)** | **0** | + +## §7b Bonus defect found IN the assigned cell: the `Urbano` leak + +Caught by my own invariant test (`test_rural_is_categorical_not_a_raw_code`), +which failed on the first run — a test that only passes is a test that taught +you nothing. + +`cluster_features.Rural` was emitting **`Urbano`** for 169 of the 450 clusters. +Guinea-Bissau is LUSOPHONE — raw `s00q04` ∈ {`Rural`, `Urbano`} — but the +`cluster_features` mapping block carried only the FRENCH keys +(`Urbain` / `urbain` / `URBAIN`), copied from a francophone EHCVM sibling. They +never matched, so the value passed through unmapped, violating the canonical +domain `{Rural, Urban}` declared in `lsms_library/data_info.yml` +(`cluster_features.Rural.spellings`). The `sample` block in the SAME wave file +already mapped `Urbano` correctly — only this block was missed. + +| `cluster_features.Rural` | before | after | +|---|---|---| +| `Rural` | 281 | 281 | +| `Urbano` (OFF-SCHEMA) | **169** | 0 | +| `Urban` (canonical) | 0 | **169** | + +450 rows before and after. Fixed in the wave `data_info.yml`; pinned by +`test_rural_is_categorical_not_a_raw_code`. + +Minor, NOT fixed (reported): `cluster_features.Region` has inconsistent casing +(`BOLAMA_BIJAGOS` vs `bafata`, `biombo`, …). `Region` declares no canonical +spellings, so this is not a schema violation — but it will not group cleanly. + +## §8 Scoped OUT (with reasons) +- **`feature.py::_collapse_duplicate_index`** (feature.py:106) — the *second* + `.first()` collapse, named in the same design-doc row (GH #325). It fires after + `_harmonize_country_frame` DELIBERATELY drops a level, so a collapse there is + intended; the doc's prescribed remedy is the union+sentinel redesign, not a + raise. Changing it blind would be the "validate where it's free, not where it's + needed" error. **Left alone and reported.** +- **Phantom NaN-key rows** — a distinct silent-loss class (GhanaLSS 2016-17 + `food_security`: 110 rows with a NaN household id). Pre-existing; not made + worse; reported. +- **Uganda `cluster_features` NaN GPS / 11 GNB clusters with NaN Lat-Lon** — + separate geo-coverage tickets. + +--- + +## Stripped to config-only (2026-07-13, GH #323 consolidation) + +This branch (`fix/323-guinea-bissau-config`, off `origin/development`) carries the +country work from `fix/323-guinea-bissau` and **nothing else**. Per +`slurm_logs/DESIGN_grain_collapse_sites_2026-07-13.org`: + +* **Stripped: 191 lines of `lsms_library/country.py`** (the Design-A + `DuplicateIndexError` / lossless-dedup machinery) and + `tests/test_duplicate_index_no_silent_loss.py` (which exercised it). + **D1: core does not aggregate.** +* **Stripped: `test_building_it_emits_no_data_loss_warning`** from + `tests/test_guinea_bissau_cluster_features.py`. It asserted that *core* stays + silent on a provably-lossless collapse — which is precisely **PR #614's** promise + (Design B), not something this country's config can deliver. On plain + `development` core *does* warn here, so the test failed once the core patch was + removed. It is core's test, and it belongs to #614. Prose in `CONTENTS.org` + claiming the framework "will raise `DuplicateIndexError`" has been corrected. +* **Kept, and it stands alone:** the `Rural` mapping fix. Guinea-Bissau is + **LUSOPHONE**; the 2018-19 `cluster_features` `Rural` map carried French + `Urbain` keys copied from a francophone EHCVM sibling and matched nothing, so + the raw label `Urbano` leaked through unmapped. + +Verified post-strip, cold build (`LSMS_NO_CACHE=1`): 450 clusters, unique `(t, v)`; +`Rural` = **281 Rural / 169 Urban**, and the off-schema value set is **empty** (was: +169 clusters carrying the raw `Urbano`, outside the `{Rural, Urban}` domain that +`lsms_library/data_info.yml` declares). Test count 4 passed. diff --git a/.coder/ledger/323-india.md b/.coder/ledger/323-india.md new file mode 100644 index 000000000..b2a540677 --- /dev/null +++ b/.coder/ledger/323-india.md @@ -0,0 +1,141 @@ +# Prior-Art Ledger — GH #323 (India / `employment`) + +**Search tier used:** ripgrep + git floor (gitnexus MCP not reachable from this +worktree; used `rg` over `lsms_library/` + `tests/` and read `country.py` +directly at the collapse site). + +## §1 Task, restated + +India's `employment` table is registered in `countries/India/_/data_scheme.yml` +with the canonical index `(t, i, pid)` and built by the **YAML path** from a +single source file, `1997-98/Data/SECT02AD.DTA`, via the `employment:` block in +`1997-98/_/data_info.yml` (`idxvars: i: hhcode, pid: idcode`). + +That source is **activity-level, not person-level**: one row = one *job*. The +declared index therefore under-declares the key, `Country.employment()` returns a +non-unique index, and `_normalize_dataframe_index` (`country.py:4176-4210`) +collapses it with `groupby(...).first()`. The task is to restore the missing key +level so the framework never collapses — *not* to declare an aggregation. + +Classification: **INDEX_INCOMPLETE**, and specifically **class-1 (silently +WRONG)**, not merely class-2 (silently MISSING) — see §4. + +## §2 Existing machinery (this task's area) + +| symbol | path:line | what it does | tested? | reuse / extend / new | +|--------|-----------|--------------|---------|----------------------| +| `_normalize_dataframe_index` | `lsms_library/country.py:4100` | reorders/drops index levels to match the declared schema; collapses a non-unique index (`groupby().first()`, or `sum` for `_ADDITIVE_MEASURE_COLUMNS`) and warns (GH #323) | yes — `tests/test_normalize_index_j_preserved.py` | **reuse unchanged** (no library edit) | +| `_declared_index_levels` | `country.py` (called at 4120) | parses `index: (t, i, pid)` from `data_scheme.yml` | indirectly | reuse — it is what makes a widened index take effect | +| `_ADDITIVE_MEASURE_COLUMNS` | `lsms_library/feature.py` | per-table map of columns that may be SUMmed on collapse (`food_acquired`) | yes | **deliberately NOT extended** — see §5 | +| `df.dropna(how='all')` | `country.py:2217` | universal safety-net: drops rows where every non-index column is NaN | — | **pre-existing**; explains the API row count (§4) | +| `_join_v_from_sample` | `country.py` | joins `v` from `sample()` at API time | yes | reuse — untouched; still resolves `v` on the widened index | +| GH #324 fix (`i: hh` → `i: hhcode`) | `India/1997-98/_/data_info.yml` `employment:` comment | earlier fix to the *same* block | — | context only | +| GH #602 (India has no `Rural`) | `India/1997-98/_/data_info.yml` `sample:` comment | deleted a bogus float-keyed `Rural` mapping | — | context — collides with `employment.Rural`, §6 | + +## §3 Definitions & conventions in force + +- Canonical index levels + required columns: `lsms_library/data_info.yml` + (per `STANDING.md §3`). `employment` is **not** registered there — it is an + India-only table (verified: `grep -rln '^ employment:' countries/*/_/data_scheme.yml` + → India only), so there is no cross-country `index_info` to widen and no + `Feature()` re-collapse one layer up (verified empirically: `Feature('employment')` + returns 6,158 rows, unique). +- YAML build path (`idxvars` / `myvars` → `df_data_grabber`): per `CLAUDE.md` + "Two Build Paths". `employment` is YAML-path; no `materialize: make`. +- `v` is joined from `sample()` at API time, never declared in a feature index: + per `CLAUDE.md` "`sample()` and Cluster Identity". Unchanged here. +- Cache tiers + `LSMS_NO_CACHE`: per `CLAUDE.md` "Cache Behavior". Load-bearing + for the test, see §4. + +## §4 Invariants & assumptions + +- **The L2-country parquet (`var/`) is written POST-collapse; the L2-wave parquet + (`{wave}/_/`) holds the truth.** Any scan of `var/` for this bug returns a false + negative. Instrument validated against the known positives before use: + Mali/2014-15 `household_roster` → 32,026 dup rows ✓; Guyana/1992 `housing` → + 311 ✓. Only then India/1997-98 `employment` → **6,194** dup on `(t, i, pid)`. +- **The GH #323 warning fires only on a COLD build.** In warm operation the + collapse is already baked into the L2-country cache and + `_normalize_dataframe_index` is never re-entered. A regression test that calls + `Country('India').employment()` on a warm cache is therefore **vacuous** — it + passes on the buggy config. Verified: the first draft of + `test_no_gh323_collapse_warning_*` passed pre-fix for exactly this reason. It + now sets `LSMS_NO_CACHE=1` to force the rebuild, and fails pre-fix. +- **`groupby().first()` is SKIPNA** (`country.py:4199`). It fills each column + *independently* from that column's first non-null value, so it does not merely + drop rows — it **manufactures** rows present in no source record. This is what + makes the defect class-1 rather than class-2: + - `hhcode=1011, idcode=1`: source rows are `A=(15.0, NaN)`, `B..E=(0.0,'Rural')`, + `F=(NaN,'Rural')`. The collapse returns **`(15.0, 'Rural')`** — a pair in no + source row. + - 107 persons received `Cash_per_day` and `Rural` from two *different* rows; + 680 persons' surviving wage came from a *non-first* activity. +- **The 3-key is exactly unique**: `(hhcode, idcode, actcode)` → 16,089 distinct + triples over 16,089 rows, zero nulls in any key. So this is neither + GENUINE_DUPLICATES nor phantom-NaN (GH #606), and no rows need to be dropped. +- **API row count ≠ source row count, and that is pre-existing.** `country.py:2217` + drops rows where *every* non-index column is NaN. 9,931 of the 16,089 activity + rows have neither a wage nor a workplace, so the API returns **6,158**, not + 16,089. This safety-net is identical before and after the fix, and no wage row + is lost to it (all 5,627 wage rows survive). *The dispatch brief's stated target + of `len == 16089` is therefore wrong*; 6,158 is the correct post-fix count. +- **`Cash_per_day` (`v02b02`) is a per-day wage RATE**, not a stock — see §5. + +## §5 Reuse decision + +| quantity | decision | reason | +|----------|----------|--------| +| activity key `act` | **new index level** (`act: actcode`) | The only fix that removes the collapse without inventing data. The 3-key is exactly unique, so widening is lossless and total. | +| `aggregation: first` | **rejected** | Provably wrong: destroys 62.0% of wage observations (5,627 → 2,136 non-null), loses 39.8% of reported cash (92,116 → 55,498), and fabricates cross-row pairs (§4). | +| `aggregation: sum` / `_ADDITIVE_MEASURE_COLUMNS` | **rejected** | `v02b02` is a per-day wage **rate**. Rates do not add without days-worked weights (`v02a02a..l` / `v02a03`). Summing would produce a meaningless "total wage rate". `food_acquired` is in the additive map because expenditure/quantity across transactions genuinely *are* additive; a wage rate is not. | +| person-level collapse (primary activity / days-weighted mean) | **deferred to the CONSUMER** | It is a real economic decision, not a framework default. Downstream of a faithful activity-level table it is one line and it is *auditable*: `df.xs('A', level='act')` for the primary activity. A silent `groupby` inside the framework is exactly what caused #323. | +| library code (`country.py`) | **unchanged** | The framework behaved as designed; the config under-declared the key. Zero-line library diff ⇒ zero blast radius for the other 39 countries. | + +## §6 Open questions for the human + +- **`employment.Rural` is misnamed and should be renamed or dropped** (filed + separately; deliberately NOT folded into this commit). `v02a05b` is a + per-*activity* **workplace** location (14,998/16,089 NaN; 728 `Urban`, 363 + `Rural`) — *not* a household residence flag. India's `sample` deliberately + carries **no** `Rural` per GH #602 ("India simply has no Rural indicator"), so + the collapse was laundering a sparse per-job workplace field into a column that + reads as the very household indicator #602 removed. Widening the index de-fangs + it (it is now honestly per-activity rather than a fabricated person-level + value), but the **name still misleads**. Suggested: `Workplace_Rural`. + Blocks: nothing in #323; it is a naming/semantics fix with its own blast radius + (`data_scheme.yml` declares `Rural: str` for `employment`). +- `data_scheme.yml` still declares `Rural: str` under **`sample`** for India even + though GH #602 removed the column from `sample`'s `data_info.yml`. Harmless + today (the column simply isn't produced on `development`… but see below). +- **Heads-up, not a finding of this task:** the main repo working copy is checked + out on branch `fix/602-spellings`, not `development`. On `development` the + pre-#602 float-keyed mapping is still live, so a cold build of India `sample` + there yields `Rural` populated with **raw stratum labels** (`' B-other'`, + `'UP-qual'`, …) for 2,251/2,251 households. That is #602, already fixed on its + own branch; noted here only because it briefly contaminated this task's control + (see Phase 3). + +--- +### Phase 3 — verification + +- `India/1997-98/_/data_info.yml :: employment.idxvars.act` — **OK (anchored on §4, §5)**: + adds the exactly-unique third key; no aggregation declared, no rows dropped. +- `India/_/data_scheme.yml :: employment.index` — **OK (anchored on §5)**: widened to + `(t, i, pid, act)`. The widened index *is* the enforcement; the comments beside it + are rationale only (`CLAUDE.md`: "prose is not enforcement"). +- `tests/test_india_employment_activity_index.py` — **OK (anchored on §4)**: 6 of its + 7 tests fail against the pre-fix config; the 7th is a schema-free unit pin on the + skipna-`first` fabrication mechanism itself. The cold-build warning test was + rewritten after it was caught passing vacuously on a warm cache (§4). +- **Control contamination (caught and corrected).** The first regression run used + the *main repo* as the "base" config and reported `sample` as CHANGED. The main + repo is on `fix/602-spellings`, not `development` — the control was wrong, not + the fix. Re-run against a detached worktree of the true base commit + (`2d3d5f71`, my branch's parent): **exactly one table changed — `employment`**; + the other 9 India tables (incl. `sample`) are byte-identical by sha256 of a + full sorted CSV dump. Lesson, same shape as the brief's instrument trap: *a + result from a control you have not validated is a result about the control.* +- **Reinvention check:** none. No new helper, transform, or estimator was written; + the fix is 3 semantic lines of YAML (one `idxvars` entry, one widened `index`). + `_normalize_dataframe_index` and `_ADDITIVE_MEASURE_COLUMNS` were considered and + deliberately left untouched (§5). diff --git a/.coder/ledger/323-malawi-gb-cartesian.md b/.coder/ledger/323-malawi-gb-cartesian.md new file mode 100644 index 000000000..74a2caeb2 --- /dev/null +++ b/.coder/ledger/323-malawi-gb-cartesian.md @@ -0,0 +1,153 @@ +# Prior-Art Ledger — GH #323 site 4 / #627 (Malawi + Guinea-Bissau cartesians) and #637 (Malawi `.first()` sites) + +> Per-task ledger. Inherits the repo §0 baseline in `STANDING.md`; cites +> `CLAUDE.md` and `lsms_library/data_info.yml` rather than re-copying them. + +**Search tier used:** ripgrep + git floor. gitnexus not consulted — the config +half touches no symbol, and the `malawi.py` half is a comment + one `dropna` +inside a function whose only callers are the four wave scripts that import it +by name (enumerated by grep). + +## §1 Task, restated + +Three of the eight cartesian `dfs:` cells from PR #627's 40-country census were +unowned after Mali's went in PR #641: + +| cell | L x R | merged | phantom | +|---|---|---|---| +| Malawi 2010-11 | 12,271 x 12,271 | 196,083 | 183,812 | +| Malawi 2019-20 | 14,612 x 11,434 | 185,842 | 171,230 | +| Guinea-Bissau 2018-19 | 5,351 x 450 | 5,410 | 59 | + +Plus: review the three `groupby().first()` sites in `Malawi/_/malawi.py` for +KEY SOUNDNESS (#637). Config/script only, no `aggregation:` key, fix the merge +never the aftermath. + +## §2 Existing machinery (this task's area) + +| symbol | path | what it does | tested? | reuse / extend / new | +|---|---|---|---|---| +| `Wave._cartesian_keys` / `_merge_subframes` | `origin/fix/323-site4-dfs-merge` `country.py:909/945` | exact many-to-many detector + phantom count | 16 tests on that branch | **measuring instrument only**; core untouched | +| `Wave.grab_data` `dfs:` block | `country.py:~1112-1215` | outer-merges sub-frames on `merge_on ∪ {t}` | yes | untouched | +| sub-frame `df_edit` dispatch | `country.py:802` + `:997` — `column_mapping(, ...)` then `mapping_info.pop('df_edit')` | runs a country/wave-module function named after the SUB-FRAME on that sub-frame, **before** the merge | no direct test before this PR | **used** for Guinea-Bissau; now pinned by a test | +| `_collapse_to_cluster_grain` (site 2) | `country.py:~4490` | projects household-grain `cluster_features` onto `(t, v)` | yes | not touched — it is why `Wave.cluster_features()` is blind here | +| PR #641 `Mali/2021-22` | `Mali/2021-22/_/data_info.yml` | deleted the `dfs:` block for a single-file extraction | yes | **template**; not applicable to either country here (see §5) | +| PR #639 `fix/323-malawi-config` | `Malawi/{2004-05,2016-17}/_/data_info.yml` | fixed 2004-05's cluster key and 2016-17's `df_geo` merge key | yes | **this branch is stacked on it**; its two "deliberately not fixed" comments are replaced | +| `tests/conftest.py::requires_s3` | `tests/conftest.py:71` | data-free-CI skip marker | yes | reused | + +## §3 Definitions & conventions in force + +- **D1, fix the merge, never reduce afterwards** — `CLAUDE.md` "Grain Collapse"; + `SkunkWorks/grain_aggregation_policy.org` §3a. `aggregation:` is dead config. +- **`dfs:` merges exist to be collapsed, not grown** — `CLAUDE.md` "Gotchas with + Teeth": *"Existing `dfs:` merges are grandfathered but should be collapsed + when touched"*. +- **The published cluster GPS is a displaced cluster fix stamped on every + household**, not household GPS — `CLAUDE.md` site-2 note (GH #161). Verified + here for both Malawi waves (0 of 768 / 0 of 717 EAs carry two coordinates). +- **`.first()` skips NA per column**, so a conflicting group yields a composite + belonging to no real row — `CLAUDE.md` "Grain Collapse". Wrong only when the + duplicate rows are DIFFERENT ENTITIES; `skipna=False` was drafted and + abandoned repo-wide. +- **EHCVM cluster identity**: `v: grappe`, `i: [grappe, menage]` — `CLAUDE.md` + "EHCVM countries". +- **Only 2016-17's cross-sectional half is `cs-17-` prefixed in the wave + scripts; every other half emits the raw wave hhid and relies on `id_walk` / + `panel_ids` chaining** — `Malawi/_/CONTENTS.org`, "plot_features (GH #167)". + Read *before* concluding anything about 2019-20's `i_prefix=''`; it is the + documented decision, not a defect (see §4, trap 3). +- **IHS4's GPS displacement is "nearly but not exactly EA-constant"** — 7 of + 779 EAs carry more than one `lat_modified` — `Malawi/_/CONTENTS.org`, + "GH #323: 2016-17 had no GPS". So the EA-constant-coordinate invariant this + PR asserts holds for 2010-11 and 2019-20 and is *documented not to* for + 2016-17; the test is parametrized accordingly and says why. + +## §4 Invariants & assumptions + +- **A merge key duplicated in BOTH sub-frames is a cartesian by construction** — + `_cartesian_keys` docstring; sound *and* complete. +- **The warm cache hides all of this.** L2-country is written post-collapse. + Every number here was measured cold, in an isolated `LSMS_DATA_DIR` with only + `dvc-cache` symlinked, with `LSMS_NO_CACHE=1`, against #627's core on + `PYTHONPATH` (asserted in-process, not assumed). +- **`Wave.cluster_features()` cannot see a cartesian for Malawi** — site 2 + projects to `(t, v)` first, giving 768 / 819 rows with or without the bug. + Tests must call `grab_data('cluster_features')`. (Guinea-Bissau declares no + `i`, so site 2 does not fire and its wave frame is the merged frame.) +- **An in-process `.first()` patch cannot see a `materialize: make` build** — + hence `runpy` (PR #646's method). All three #637 tables are `materialize: + make`. +- **"Exact duplicates" is not reassurance and invariance can be missingness** — + PR #646 / the Tanzania key. Disposed of here by there being no duplicate + groups at all on a non-null key, which is a stronger statement than "the + duplicates agreed". +- **A BROKEN KEY CAN PRODUCE ZERO DUPLICATES** (#637 trap 3, the Tanzania + `shocks` inversion). A key from the wrong namespace makes every replicated + row its own distinct "household", so the duplicate-count instrument reads + clean on a thoroughly broken key. **"0 duplicates" is therefore necessary, + not sufficient**; the discriminating check is a **per-wave** overlap of the + table's `i` against the roster's — per wave, because clean waves otherwise + carry broken ones through an aggregate check. Run for all three Malawi + sites: **100% in every wave**, matching literally (`101011000014` on both + sides), with no row-count inflation relative to source households × + strategies. Worth running rather than waving away: Malawi 2019-20 has the + surface shape of a namespace split — `cs_i` (`'cs-19-' + format_id`) in + `data_info.yml` for `sample` / `household_roster` / `cluster_features` vs. + `i_prefix=''` in all three wave scripts. **`CONTENTS.org` already records + that asymmetry as deliberate** (§3 above), and the measurement confirms it: + the prefix survives to the API on neither side (0 of 14,612 ids carry it, in + any table), so both are in the raw `case_id` namespace and they agree. + +## §5 Reuse decision + +| quantity | decision | reason | +|---|---|---| +| Malawi `df_geo` key | **re-key** `v: ea_id` → `i: case_id` (`cs_i` in 2019-20), `merge_on: [i]` | files are exactly 1:1 on `case_id`; #627's first named remedy | +| Malawi: delete the `dfs:` block (Mali's cure) | **rejected** | lat/lon exist only in the geovariables file; the cover page has none | +| Guinea-Bissau: re-key on `i` | **rejected** | the GPS file is genuinely grappe-grain and carries nothing finer; both copies of every duplicate agree on `vague` | +| Guinea-Bissau: single-file cure | **rejected** | cover page has no GPS columns at all (checked) | +| Guinea-Bissau: `df_geo` hook + `drop_duplicates()` | **taken** | #627's *second* named remedy — reduce the sub-frame to merge-key grain BEFORE the merge. Lossless: the 5 extra rows are byte-identical incl. GPS timestamp | +| Guinea-Bissau: `groupby().first()` / `keep='first'` | **rejected** | would silently choose between two disagreeing fixes if one ever ships; `drop_duplicates()` lets the guard fire instead | +| Guinea-Bissau: convert to a `materialize: make` script | **rejected** | far heavier than 59 rows warrant, and moves a YAML-expressible table onto the script path against the repo's stated preference | +| `.first(skipna=False)` at any #637 site | **rejected** | no multi-row groups exist; the repo abandoned that approach | +| `aggregation:` key anywhere | **rejected** | dead config, D1 | + +## §6 Open questions for the human + +- **The Guinea-Bissau hook is dispatched BY NAME off the sub-frame key.** This + is real, current behaviour on both `development` and #627's branch, but it is + undocumented and there was **no prior art for it** (`rg '^def df_geo'` across + all 40 countries: zero hits). It is heavily documented in three places and + pinned by a test, but a reviewer may prefer a different mechanism. If so, the + alternatives are ranked in §5. +- **2019-20 at HOUSEHOLD grain**: 8 IHS5 geo rows have a NULL coordinate while + their 15 EA siblings carry the EA's fix. Under the `v`-merge those 8 + households borrowed a sibling's value; under the `i`-merge they get their own + NULL. The returned `(t, v)` table is unchanged (`.first()` skips NA), and the + household-grain intermediate is now honest — but it *is* a behavioural + difference, and it is the only one. +- **`food_coping` / `months_food_inadequate` build only the cross-sectional + half in 2016-17 and 2019-20** (12,447 and 11,434 households; the IHPS panel + halves are absent). Observed while auditing #637; a coverage gap, not a key + defect, and out of scope here. + +--- +### Phase 3 — verification + +- `Malawi/{2010-11,2019-20}/_/data_info.yml` — **OK (§3, §5)**: `merge_on: [i]`, + no `aggregation:`, cured at the merge. Phantom 183,812 → 0 and 171,230 → 0. +- `Guinea-Bissau/2018-19/_/{data_info.yml,mapping.py}` — **OK (§3, §5)**: + reduced before the merge, de-duplication not aggregation. Phantom 59 → 0. +- Value preservation — **OK (§4)**: country-level `cluster_features` + bit-for-bit identical for both countries (`DataFrame.equals` True, index + equal, dtypes equal; Malawi 3,235 x 5, Guinea-Bissau 450 x 4). +- `tests/test_gh323_malawi_gb_cartesian.py` — **OK (§4)**: asserts on + `grab_data`, not `Wave.cluster_features()`. **Negative control run** (pre-fix + configs restored, fresh isolated data root): `6 failed, 10 passed` — the three + cartesian tests report 5410 / 196083 / 185842 and the three structural tests + fail, while the 10 source-invariant and cluster-count tests pass *with the bug + fully present*, which is exactly the blindness the module docstring warns + about. After: `16 passed`. +- `Malawi/_/malawi.py` — **OK (§3)**: no `skipna=False`, no reducer added; one + `dropna(subset=['plot_id'])` that makes an existing silent deletion explicit + and is a provable no-op (`groupby(dropna=True)` already removed those rows). diff --git a/.coder/ledger/323-malawi-key.md b/.coder/ledger/323-malawi-key.md new file mode 100644 index 000000000..063893dd2 --- /dev/null +++ b/.coder/ledger/323-malawi-key.md @@ -0,0 +1,236 @@ +# Prior-Art Ledger — GH #323 (Malawi, Site 2): the cluster key + +**Search tier used:** ripgrep + git, plus PR #634 / `.coder/ledger/323-uganda.md` +(the template for this task), the consolidation note +`slurm_logs/DESIGN_grain_collapse_sites_2026-07-13.org` (branch +`origin/docs/323-grain-collapse-sites`, Site 2 + D1/D2/D3), and PR #627's Site-4 +cartesian census. Every number below is a **cold** build against an *isolated* +`LSMS_DATA_DIR` (fresh tree, `dvc-cache` symlinked to the shared L1) with +`LSMS_COUNTRIES_ROOT` pointed at this worktree and asserted before any run. +`LSMS_NO_CACHE=1` alone was **not** trusted — it is soft for script-path L2-wave +parquets, and the collapse is baked into the cache it poisoned. + +## §1 Task, restated + +Malawi's `cluster_features` is declared `(t, v)` in `_/data_scheme.yml` but every +one of the five waves extracts it from the household **cover page**, so the frame +arrives at household grain and is projected onto the cluster by +`Wave.cluster_features` → `country._collapse_to_cluster_grain` → `.first()`. Where +the households of one `v` disagree about the cluster's own attributes, one +arbitrary household's answer is served as the cluster's — and `.first()` skips NA +per column, so the row it returns can be a **composite existing in no source +record**. The task: find, empirically, what actually identifies a cluster in each +wave and fix the identifier. Config only (`countries/Malawi/**`); no +`lsms_library/*.py`, no `aggregation:` keys, no declared reducer, no blanking to +``. + +## §2 Existing machinery (this task's area) + +| symbol | path | what it does | tested? | reuse / extend / new | +|--------|------|--------------|---------|----------------------| +| `_collapse_to_cluster_grain` | `country.py:4490` | Site 2's audit-then-`.first()`; emits `GrainCollapseWarning` | yes | **do not touch** (core, PR #617) — monkeypatched *read-only* to capture the pre-collapse frame | +| `_join_v_from_sample` | `country.py` | joins `cluster_features` onto household tables through `sample.v` | yes | the constraint: forces `sample.v` ≡ `cluster_features.v` | +| `_enforce_canonical_spellings` | `country.py` (`_finalize_result`) | `rural`/`RURAL` → `Rural` at API time | yes | **reuse** — the reason no `Rural` mapping is needed in YAML | +| auto-discovery categorical mapping | `country.py` (`_apply_categorical_mappings`) | column name ↔ `categorical_mapping.org` table name, case-insensitive | yes | **reuse**, and *also* apply the `region` table at extraction (§6.3) | +| `mappings: [table, from, to]` in `myvars` | `country.py:760` `map_formatting_function` | pulls a `categorical_mapping.org` table into a dict | yes (used by Malawi `strata` in all 5 waves) | **reuse** for 2016-17 `Region` | +| `psu` column | `Malawi/2004-05/Data/sec_a.dta` | the fully qualified 8-digit EA code, already in the file | — | **reuse** — no derivation, no composite, no new primitive | +| `cs_i` | `Malawi/2016-17/_/mapping.py`, `2019-20/_/mapping.py` | `'cs-17-' + format_id(...)`, keeps CS ids apart from panel `y{3,4}_hhid` | — | **reuse** — `df_geo` must build `i` the same way `df_main` does | +| `Rural: 0/1` inline `mapping:` blocks | all 5 waves | **already removed on `development`** by GH #602 | yes | nothing to do — do **not** re-add | +| `aggregation:` block | `Malawi/_/data_scheme.yml` (`interview_date`) | dead config; nothing reads it | n/a | **forbidden** to add more (D1); the existing one is out of scope | + +## §3 Definitions & conventions in force + +- `sample()` is the single source of truth for a household's cluster; `v` is + joined from it at API time — `CLAUDE.md` "`sample()` and Cluster Identity". + ⇒ **`sample.v` and `cluster_features.v` must be the same key.** +- `cluster_features` owns `v`; index `(t, v)` — `Malawi/_/data_scheme.yml`. +- Canonical `Rural` is `str` with spellings `Rural: [rural, RURAL, …]` — + `lsms_library/data_info.yml`. **Not** a 0/1 indicator. +- "NO AGGREGATION IN CORE" — `SkunkWorks/grain_aggregation_policy.org`, D1 of the + consolidation note. +- D2: NaN-key rows are deleted and reported, never retained. (Malawi has none: + every wave's cover page populates its cluster column for every household.) +- D3: Sites 2 and 4 get their own PRs. Site 4 is PR #627 — see §6.5. + +## §4 Invariants & assumptions + +- **The Malawi EA code is structured**: `region(1) + district(2) + TA(2) + EA(3)`, + 8 digits. Verified by decoding: the leading digit reproduces the reported + region for 11,280/11,280 households in 2004-05. +- **Frame vintage is not shared across the IHS2/IHS3 boundary.** 2004-05's 564 + EA codes overlap 2010-11's 768 in only **3** codes (1998 vs 2008 census frame). + The *format* is common to all five waves; the *sets* are not. So a cross-wave + join on `v` is meaningful from 2010-11 on and not before — and the 2004-05 fix + does not, and cannot, create a spurious panel link. +- **The IHPS waves are TRACKED panels.** `ea_id` in 2013-14 / the panel halves of + 2016-17 and 2019-20 is the **IHS3 baseline EA**, and the panel EA sets nest + cleanly: 2019-20-PN = 2016-17-PN = 102 ⊂ 2013-14 = 204 ⊂ 2010-11 = 768. So a + cluster's households genuinely live in different districts. Dispersal, not + collision — see §6.2. +- **The two halves of 2016-17 / 2019-20 do not collide.** `ea_id` is one national + keyspace; the single code shared between the 2016-17 CS and panel halves + (`30305580`) is the *same real EA* in the *same district*, sampled twice. +- `format_id` is auto-applied to `idxvars`, not `myvars` — `CLAUDE.md`. `psu` is + already an 8-character string in the source, so `sample.myvars.v: psu` needs no + formatter. +- **A cartesian merge cannot change a contested-cell count.** `nunique(dropna=True)` + per (cluster, column) is invariant to row repetition. Assumed by PR #627's + owner; **measured** here rather than assumed — §7. + +## §5 Reuse decision + +| quantity | decision | reason | +|----------|----------|--------| +| 2004-05 cluster key | **reuse the source's own `psu`** | Not a composite, not derived: the fully qualified code is already a column. 564 values, exactly 20 HH each, 0 contested cells. A `(dist, ta, ea)` composite would produce the identical partition (verified: 564 groups) with three columns instead of one, and would *not* be on the later waves' 8-digit keyspace. | +| 2013-14 / 2019-20 panel keys | **no change** | The contest is dispersal. Uganda §6.5 set the precedent (`comm` not re-keyed): a structured code with zero collisions in its frame year, whose multi-district groups show a dispersal signature, must not be split by current district. Splitting 2013-14 on `(district, ea_id)` yields 626 groups of **median 1 household** — 422 singleton pseudo-clusters — and would desynchronise `v` from the IHS3 baseline. | +| GPS reducer | **none** — no averaging, no median | Same argument that retired core's `.mean()` on 2026-07-13, and here it is decisive: the 2013-14 within-EA spread is *entirely* movers (§6.2), so a centroid would place a cluster in the middle of nowhere between an EA and the towns its emigrants moved to. | +| categorical reducer | **none declared** | D1. The key fix removes the conflicts that were fixable; the rest are reported. | +| `Rural` 0/1 mapping | **nothing** — already removed by GH #602 on `development` | A second implementation would fork the vocabulary. Canonical `spellings` already normalise the three cases. | +| 2016-17 `Region` | **extend**: apply the existing `region` table at extraction | The auto-discovery mapping already collapses `South`/`Southern` at API time, so this is output-neutral; it just stops the *collapse* from reading a spelling difference as a disagreement about the cluster. | +| 2016-17 `df_geo` merge key | **fix**: build `i` with `cs_i`, exactly as `df_main` does | Not a new mechanism — the transform already exists and `df_main` already uses it. The block simply failed to mirror it. | +| 2010-11 / 2019-20 cartesian `df_geo` merges | **leave, and document in place** | Site 4, PR #627 owns it (D3). The one-line config fix was implemented, measured, and then **reverted** so #627's cardinality guard keeps these two cells as evidence. §6.5. | + +## §6 Decisions, with the evidence + +1. **2004-05's `ea` is not a cluster id — it is an intra-TA sequence number.** + 110 distinct values for 564 EAs. Measured at source, contested cells by key: + + | key | groups | region | district | reside | + |-----|--------|--------|----------|--------| + | `ea` | 110 | 66 | 79 | 19 | + | `(dist, ea)` | 447 | 0 | 0 | 8 | + | `(dist, ta, ea)` | 564 | 0 | 0 | 0 | + | **`psu`** | **564** | **0** | **0** | **0** | + + `psu` ≡ `(dist, ta, ea)` exactly (each is unique within the other), with 20 + households in every group. The `(dist, ea)` row is the instructive one: it is + the "make `v` a district composite" reflex, and it is *wrong here* — it still + merges EAs that share a district but sit in different TAs. + +2. **2013-14 is dispersal, and the GPS proves it.** 188 of 204 EAs have + households at different coordinates — the broken-key signature. But restrict + to households that did not move (`dist_to_IHS3location` ≤ 1 km) and **all 204 + EAs collapse to exactly one coordinate**; `LAT_DD_MOD` is the EA-level + displaced fix of the household's *current* location, not per-household jitter. + Median within-EA spread 145 km, max 775 km — every kilometre of it a tracked + mover. Corroborated: 588/4,000 households report a district different from the + one encoded in their own `ea_id`, sitting a median **68 km** from their IHS3 + location, against **0.03 km** for the 3,412 that match. + + This is the finding that made the difference between fixing an identifier and + fabricating one. The brief's premise — "at 75%, the key is broken" — is right + about 2004-05 and wrong about 2013-14, and the reason is arithmetic: with ~20 + households per EA and 13% of them district-movers, `1 − 0.87²⁰ ≈ 94%` of EAs + are *expected* to contain at least one mover. The high **cell** rate is a + property of the metric on a tracked panel, not evidence of a merged key. + +3. **The 2016-17 `Region` conflict was a spelling, not a collision.** EA + `30305580` is in both halves; the CS file writes `Southern`, the panel file + `South`. Same district, same real EA. Applying the `region` table at + extraction removes the cell. 1 → 0. + +4. **2016-17 had no GPS at all, and it was a merge key.** `df_main` builds + Cross_Sectional `i` as `cs_i(case_id)`; `df_geo` declared the raw `case_id`, + so the `how='outer'` join matched nothing — 12,447 orphan geo rows next to + 14,955 coordinate-less main rows, and **0 of 880 clusters with GPS**. The two + files are exactly 1:1 on `case_id` (12,447 = 12,447, none on either side + alone), so mirroring the transform makes the join exact: **779 of 880** + clusters now carry coordinates (the other 101 are IHPS panel EAs, for which + IHS4 publishes no geovariables). Honest cost: 7 of those 779 EAs have + households whose `lat_modified` differs within the EA — IHS4's displacement is + nearly but not exactly EA-constant — so `Latitude`/`Longitude` show 7 + contested cells where before there were none, *because before there was no + coordinate*. + + This is **not** a Site-4 cardinality fix and 2016-17 is not on PR #627's + cartesian list. It is a non-matching key, which produces orphans, not a + product. + +5. **The two real cartesians were fixed, measured, and then reverted.** 2010-11 + (196,083 rows from 12,271; 183,812 phantom) and 2019-20 (185,842 from 14,612; + 171,230 phantom) merge a household-grain geo file on the cluster key `v`. + Collapsing both to `merge_on: [i]` was implemented and measured: contested + cells **0 and 250 — identical either way**, confirming §4's invariance claim + by measurement rather than by argument. The change was then reverted at the + coordinator's direction so PR #627's guard retains these cells as evidence; + the one-line fix is recorded in each wave's `data_info.yml` next to the + declaration that causes it, and in `CONTENTS.org`. #627 owns the decision. + +6. **`Rural` needed nothing.** The pre-#602 inline `mapping:` blocks were both + live and inverted (`rural: 0` in one file, `RURAL: 1` in another, in the *same + wave*), but GH #602 already removed all five on `development`. Verified + against the pre-collapse frames: `Rural` arrives as the raw label and + `_enforce_canonical_spellings` normalises it. Nothing added. + +## §7 Measured effect (cold build, isolated `LSMS_DATA_DIR`) + +Contested cells = clusters where `nunique(dropna=True) > 1` for that attribute, +counted on the frame captured *inside* `_collapse_to_cluster_grain` before it +runs — the only place the evidence still exists. + +| wave | before | after | clusters | note | +|------|--------|-------|----------|------| +| 2004-05 | 164 / 330 (49.7%) | **0 / 1,692** | 110 → **564** | Region 66→0, District 79→0, Rural 19→0 | +| 2010-11 | 0 / 3,840 | 0 / 3,840 | 768 | already correct | +| 2013-14 | 765 / 1,020 (75.0%) | 765 / 1,020 | 204 | dispersal — §6.2 | +| 2016-17 | 78 / 4,405 | 89 / 4,400 | 880 | Region 1→**0**; Lat/Lon 1→7 *because GPS now exists*: 0 → **779** clusters with coordinates | +| 2019-20 | 250 / 4,095 (6.1%) | 250 / 4,095 | 819 | dispersal — panel half only | +| **total** | **1,257 / 13,690** | **1,104 / 15,047** | 2,782 → **3,235** | | + +(The brief's baseline was 1,255; the 2-cell difference is in 2004-05 and does not +bear on anything. Its 424,607 source rows are 412,160 here — the 12,447 removed +are 2016-17's orphan geo rows, and ~355k of what remains is the known cartesian.) + +At the API, what the 2004-05 fix restored: **20 → 26 districts**, and the +settlement strata from `{Rural}` — every one of the 110 mega-clusters came out +rural — back to `{Rural, Urban}`. + +Two of the five waves (2004-05, 2010-11) now emit **no `GrainCollapseWarning` at +all**; before, four did. + +## §8 Verification + +- `sample.v` ≡ `cluster_features.v` in **every** wave: **0** `cluster_features` + `(t, v)` pairs unknown to `sample`, **0** orphaned `sample` clusters. +- `tests/test_gh323_malawi_cluster_key.py` — 14 tests, all config-level; + exercises no core aggregation. **Negative control run**: 7 of the 14 fail + against a pristine `origin/development` config tree (exported with + `git archive`, cold data dir), including every load-bearing one. +- Malawi-relevant slice (`-k "malawi or Malawi or 323 or schema or sample or + cluster"`): **680 passed, 58 skipped, 4 xfailed, 1 failed**. +- The 1 failure is **pre-existing and untouched by this branch**: + `tests/test_gh323_explicit_reducers.py::test_core_does_not_dispatch_the_reducers` + greps `lsms_library/country.py` for the substring `collapse_to_cluster_grain` + and finds PR #617's private `_collapse_to_cluster_grain`. This branch is + config-only and does not touch `country.py`. Same failure is reported on PR + #634. + +## §9 Deferred (deliberately) + +- **Site 4 / PR #627**: 2010-11 and 2019-20's cartesian `df_geo` merges. §6.5. +- **The `region`/`district` extraction-time harmonisation is applied only where + it removed a measured conflict** (2016-17 `Region`). Doing it everywhere is + output-neutral and tidier, but it is a diff across five waves and two tables + for zero measured gain, so it is not in this PR. +- **2016-17's 101 panel clusters and 2019-20's 102 have no coordinates**, because + IHS4/IHS5 publish geovariables for the cross-sectional half only. Pre-existing, + unrelated to #323, recorded in `CONTENTS.org`. +- **IHPS-2016 mixes baseline and current geography in one file** (`district` + baseline, `reside` current — 0 vs 277 households off their EA's mode). That is + the source's inconsistency; documented, not papered over. + +--- +### Phase 3 — verification + +- `Malawi/2004-05/_/data_info.yml` `sample.v` / `cluster_features.v` = `psu` — + **OK (anchored on §3, §5)**: same key in both, as `_join_v_from_sample` + requires; verified 0 unknown pairs. +- `Malawi/2016-17/_/data_info.yml` `df_geo.idxvars.i` — **OK (anchored on §2)**: + reuses the existing `cs_i`, mirroring `df_main`; no new transform. +- `Malawi/2016-17/_/data_info.yml` `Region` `mappings:` — **OK (anchored on §2)**: + same `mappings: [table, from, to]` form the wave's `strata` already uses; the + `region` table is unchanged. +- 2013-14 / 2019-20 — **OK (anchored on §5, §6.2)**: no change is the decision, + and the decision is pinned by a test that fails if the evidence for it changes. +- No `aggregation:` key added, no reducer declared, no value blanked to ``, + no file under `lsms_library/*.py` touched — **OK (anchored on §3, D1)**. diff --git a/.coder/ledger/323-mali-cartesian.md b/.coder/ledger/323-mali-cartesian.md new file mode 100644 index 000000000..db1081ac7 --- /dev/null +++ b/.coder/ledger/323-mali-cartesian.md @@ -0,0 +1,107 @@ +# Prior-Art Ledger — GH #323 site 4 / #627: Mali `cluster_features` cartesian + +> Per-task ledger. Inherits the repo §0 baseline in `STANDING.md`; cites +> `CLAUDE.md` and `lsms_library/data_info.yml` rather than re-copying them. + +**Search tier used:** ripgrep + git floor (gitnexus not consulted; this is a +config-only change touching no symbol). + +## §1 Task, restated + +`Mali/2021-22/_/data_info.yml` declared `cluster_features` as a two-sub-frame +`dfs:` block merged on the cluster key `v`. Both sub-frames were finer-grained +than `v`, so `Wave.grab_data`'s `pd.merge(..., on=['v','t'], how='outer')` was a +many-to-many CARTESIAN PRODUCT: 393,480 × 6,143 → 4,718,148 rows, **4,324,668 +of them phantoms** — 88% of the 4,907,774 phantom rows PR #627's 40-country +census found. `_normalize_dataframe_index` then collapsed the result to 513 rows +with `groupby().first()`, so nothing downstream ever saw it. Fix the merge (D1: +core never aggregates the explosion away afterwards). Config-only, no +`lsms_library/*.py`. + +## §2 Existing machinery (this task's area) + +| symbol | path:line | what it does | tested? | reuse / extend / new | +|--------|-----------|--------------|---------|----------------------| +| `Wave.grab_data` (`dfs:` region) | `lsms_library/country.py:989-1035` (merge at `:1033`) | outer-merges `dfs:` sub-frames on `merge_on ∪ {t}` | yes | **untouched** (core is out of scope) | +| `Wave._cartesian_keys` / `_merge_subframes` | `origin/fix/323-site4-dfs-merge` `country.py:~906-1000` | exact many-to-many detector + phantom count | 16 tests on that branch | **used as the measuring instrument only**; not merged here | +| `_normalize_dataframe_index` | `country.py:4540` | collapses non-unique declared index via `groupby().first()`, audited by `_audit_index_collapse` (`:4214`) | yes | untouched — it is the janitor, not the culprit | +| `_collapse_to_cluster_grain` (site 2, GH #161) | `country.py:4490` | projects household-grain `cluster_features` onto `(t, v)` | yes | **not reached**: Mali declares no `i` in `cluster_features` `idxvars` | +| `Mali/2021-22 sample.df_cover` | `Mali/2021-22/_/data_info.yml:85-97` | already reads `Rural` from `s00q04` of the SAME cover page | — | prior art proving `s00q04` is the milieu column | +| `Mali/2017-18 cluster_features` | `Mali/2017-18/_/data_info.yml:42-60` | reads `Region: s0q01`, `Rural: s0q04` from that wave's cover page | — | prior art for the cover-page-as-geography pattern | + +## §3 Definitions & conventions in force + +- **`dfs:` merges exist to be collapsed, not grown**: *"Do NOT use `dfs:` merge + blocks just to join `v` from a cover page — collapse to a single-file + extraction"* / *"Existing `dfs:` merges are grandfathered but should be + collapsed when touched"* — `CLAUDE.md`, "`sample()` and Cluster Identity" and + "Gotchas with Teeth". +- **D1, no aggregation in core**: *"Duplicates on a declared index mean the + IDENTIFIER IS BROKEN or a LEVEL IS MISSING — fix the index; do not declare a + reducer"*; `aggregation:` is dead config — `CLAUDE.md`, "Grain Collapse"; + `SkunkWorks/grain_aggregation_policy.org` §3a. +- **EHCVM cluster identity**: `v: grappe` (not `[vague, grappe]`), + `i: [grappe, menage]` — `CLAUDE.md`, "EHCVM countries"; + `Mali/_/CONTENTS.org` "Sampling Design". +- **Cluster GPS is a displaced cluster fix stamped on every household**, not a + household fix — `CLAUDE.md`, site-2 note (GH #161). Verified for Mali 2021-22 + (513/513 grappes carry exactly one distinct coordinate pair). + +## §4 Invariants & assumptions + +- **A merge key duplicated in BOTH sub-frames is a cartesian by construction** — + `_cartesian_keys` docstring, `origin/fix/323-site4-dfs-merge`. Sound *and* + complete; the row-count-ceiling heuristic is not. +- `ehcvm_conso_*.dta` is at **(household × food item)** grain — 767 rows per + grappe in Mali 2021-22. Any `dfs:` sub-frame drawn from it and keyed only on + `v` is finer than the merge key. +- `s00_me_*.dta` is the **household cover page** — 12 households per grappe. + Also finer than `v`. Two finer-than-key frames ⇒ cartesian. +- `groupby().first()` **skips NA per column**, so a collapsed row can be a + composite assembled from different source rows. Harmless here only because + Region / Rural / Lat / Lon are each provably constant within grappe — which is + now asserted in a test, not in a comment. +- **The warm cache hides this.** The L2-country parquet is written + post-collapse; every before/after measurement here was made in an isolated + `LSMS_DATA_DIR` with only `dvc-cache` symlinked in. + +## §5 Reuse decision + +| quantity | decision | reason | +|----------|----------|--------| +| `Region` (2021-22) | **repoint**: `ehcvm_conso.region` → `s00_me.s00q01` | same 9 labels, 513/513 grappes agree, constant within grappe; mirrors what 2017-18 already does | +| `Rural` (2021-22) | **repoint**: `ehcvm_conso.milieu` → `s00_me.s00q04` | ditto; `sample.df_cover` in this same file already uses `s00q04` | +| `Latitude` / `Longitude` | **reuse unchanged** (`s00_me.GPS__*`) | already came from the cover page; only the merge around it is deleted | +| the merge itself | **delete** | all four columns live in one file; a `dfs:` block with one source is not a join | +| a reducer / `aggregation:` key | **rejected** | D1 — a reducer on a cartesian "only puts a signature on the corpse" | +| `merge_on: [i]` (keep the merge, fix the key) | **rejected** | works, but leaves a `dfs:` block whose only purpose is to re-join a file to itself at the grain it already has; CLAUDE.md says collapse when touched | + +## §6 Open questions for the human + +- **`Rural` in `cluster_features` ships the raw French `Urbain`**, while + `sample.Rural` in the same wave maps it to `Urban`. Pre-existing and + corpus-wide (2014-15 shows `Urbain` too); deliberately NOT changed here + because this PR must be value-preserving. Blocks nothing; adjacent to GH #602. +- **2018-19 merges the 366,639-row consumption file against a 549-row + grappe-GPS file.** Not a cartesian (the right side is 1 row per grappe, so + it is m:1) and not fixed here, but it reads 366k rows to produce 551. The + cover page `s00_me_mli2018.dta` would supply Region/Rural at household grain; + the GPS genuinely lives in a separate file there, so the block cannot be + deleted, only slimmed. Deferred: it changes no value and is pure waste. + +--- +### Phase 3 — verification + +- `Mali/2021-22/_/data_info.yml cluster_features` — **OK (anchored on §3, §5)**: + single-file extraction, no `dfs:`, no `aggregation:`; the merge is cured, not + laundered. +- `tests/test_gh323_mali_cartesian.py` — **OK (anchored on §4)**: asserts at the + WAVE level because §4's cache/collapse note means a country-level assertion + passes with the bug present. **Negative control run** (pre-fix YAML restored, + fresh isolated `LSMS_DATA_DIR`): `2 failed, 9 passed` — the wave-level + cartesian test reports `4718148 == 6143` and the structural test reports the + `dfs:` block; the four country-level tests pass *with the bug present*, which + is exactly the blindness the docstring warns about. After the fix: `11 passed`. +- No `lsms_library/*.py` touched — **OK (anchored on §1)**: the D1 core patches + on `origin/fix/323-mali` were deliberately not reused; that branch is about + `pid`, a different defect. diff --git a/.coder/ledger/323-mali-pid.md b/.coder/ledger/323-mali-pid.md new file mode 100644 index 000000000..455cf59b2 --- /dev/null +++ b/.coder/ledger/323-mali-pid.md @@ -0,0 +1,158 @@ +# Prior-Art Ledger — GH #323 (Mali `pid` identifier) + +> Per-task ledger. Inherits the repo §0 baseline in `STANDING.md`; cites +> `CLAUDE.md` and `lsms_library/data_info.yml` rather than re-copying them. + +**Search tier used:** ripgrep + git floor (gitnexus MCP not reachable from this +worktree session). Every claim below was checked against the SOURCE `.dta` via +`get_dataframe()` — never against a cached parquet, which is written +POST-collapse and therefore cannot show the evidence (see §4). + +**Scope.** This ledger covers ONLY the `pid` identifier fix. The +`interview_date` / `visit` work that shared branch `fix/323-mali` is a separate +change requiring a new `{const: value}` core primitive; it is deliberately NOT +in this branch and is not assessed here. + +## §1 Task, restated + +Mali's `household_roster` and `individual_education` are declared with index +`(t, i, pid)` in `countries/Mali/_/data_scheme.yml`. In the 2014-15 wave both +wire `pid` in `idxvars` to source column `s01q`. `s01q` is not the person's +identifier. The declared index is therefore non-unique in the source, and +`_normalize_dataframe_index` reduces it with `groupby().first()` — silently +deleting rows. Fix the identifier so the declared index is unique at source, and +prove the recovered rows are real. Config-only: `countries/Mali/**`, no core. + +## §2 Existing machinery (this task's area) + +| symbol | path:line | what it does | tested? | reuse / extend / new | +|--------|-----------|--------------|---------|----------------------| +| `_normalize_dataframe_index` | `lsms_library/country.py` | collapses a non-unique DECLARED index with `groupby().first()` — the mechanism that ate the rows | yes | **do not touch** (core; owned by PR #614) | +| `df_data_grabber` | `lsms_library/local_tools.py` | builds `idxvars`/`myvars` from one source file; auto-applies `format_id` to `idxvars` | yes | reuse as-is | +| `pid(value)` | `countries/Mali/_/mali.py:13` | Mali's `pid` formatter; expects a 3-component composite | — | reuse (`[grappe, menage, s01q00]` is 3 components) | +| `people_last7days` (2014-15) | `countries/Mali/2014-15/_/people_last7days.py:48` | **already** builds pid as `mali_pid([grappe, menage, s01q00])` and asserts `(t,i,pid)` uniqueness | asserts | **prior art — this is the correct wiring, already in the repo** | +| `roster_to_characteristics` | `lsms_library/transformations.py` | derives `household_characteristics` from `household_roster`; applies the MonthsSpent residence filter | yes | untouched; consumes the fix | + +The single most important entry is the fourth: the correct `pid` formula was +**already written and asserted** one directory over, in this same wave, against +this same source file. `data_info.yml` and `people_last7days.py` had simply +drifted out of the same keyspace. Nothing new was invented here. + +## §3 Definitions & conventions in force + +- `household_roster` / `individual_education` index: `(t, i, pid)` — + `countries/Mali/_/data_scheme.yml:18,26`. Unchanged by this task. +- `pid` = the *person*. Per `lsms_library/data_info.yml` (Index Info), `pid` is + the within-household individual key; `i` is the household. +- Source variable labels, read from `EACIIND_p1.dta` metadata (ground truth, not + paraphrase): + - `s01q00` → `"Numero d'ordre"` — the person's own roster line number. + - `s01q` → `"Code du répondant"` — the roster line of whoever ANSWERED + section 1 on the household's behalf. A household-level fact. + - (For contrast, the same file's `s01q06` → `"Code du père dans le menage"`, + another line-number *reference*, mis-wired once before as education in + GH #171. Same family of bug: a reference column mistaken for a value.) +- EHCVM convention (`CLAUDE.md`): Mali is `v: grappe`, `i: [grappe, menage]`. + Preserved; only the `pid` component changed. + +## §4 Invariants & assumptions + +- **A declared index must be unique in the source.** Duplicates on a declared + index mean the IDENTIFIER IS BROKEN or a LEVEL IS MISSING — not "reduce me". + Per `SkunkWorks/grain_aggregation_policy.org` and decision D1 of + `slurm_logs/DESIGN_grain_collapse_sites_2026-07-13.org`, core does **not** + aggregate, and no reducer may be declared to paper over this. Mali is the case + that argument is made from: no reducer is correct — `first` keeps one person, + `sum` is meaningless on `Sex`. +- **The L2 parquet is written POST-collapse.** A warm read cannot see the lost + rows: the cached index is already unique. Every measurement in this ledger was + taken with `LSMS_NO_CACHE=1`. (This is *why* #323 was closed once and stayed + broken — `CLAUDE.md` cache section; DESIGN §"Why the old warning could not + fire".) +- **Core is owned centrally.** `lsms_library/*.py` is off-limits on this branch + (PR #614 Site 1/3, a Site-2 PR, a shared-helpers PR). Verified: this branch's + diff touches one file, `countries/Mali/2014-15/_/data_info.yml`. +- `format_id` is auto-applied to `idxvars` (`CLAUDE.md`), so the new + `[grappe, menage, s01q00]` composite is formatted exactly as the old one was; + no formatter change needed. + +## §5 Reuse decision + +| quantity | decision | reason | +|----------|----------|--------| +| Mali 2014-15 `pid` | **reuse** | Adopt the formula `people_last7days.py:48` already uses and asserts: `mali_pid([grappe, menage, s01q00])`. No new machinery. | +| the collapse itself | **do not touch** | Core (`_normalize_dataframe_index`). Owned by PR #614 under decision D1. | +| `aggregation:` YAML key | **rejected** | Would contradict §4. Declaring `{pid: first}` here "puts a signature on the corpse" (DESIGN D1). | +| `household_characteristics` | **no change** | Derived at runtime; it picks the fix up for free. | + +## §6 Open questions for the human + +- 469 of 3,804 households have a `s01q` ("Code du répondant") that VARIES across + member rows — i.e. different respondents answered for different members. That + is a property of the source instrument and is now simply unused by `pid`. It is + not carried into any table. If the respondent identity is ever wanted, it is a + `myvars` column, never an index level. **No decision blocked.** +- The `interview_date` / `visit` half of the original `fix/323-mali` branch is + deferred to its own PR (it needs a `{const: value}` idxvars primitive in core). + Not addressed here. + +--- +### Phase 3 — verification + +Anchored on the ledger, `LSMS_NO_CACHE=1`, `LSMS_COUNTRIES_ROOT=`. + +- **`Mali/2014-15/_/data_info.yml` `household_roster.idxvars.pid`** — `OK + (anchored on §2, §3, §5)`. Now `[grappe, menage, s01q00]`, the formula + `people_last7days.py:48` already used. Not a reinvention; a re-convergence. +- **`Mali/2014-15/_/data_info.yml` `individual_education.idxvars.pid`** — `OK + (anchored on §2, §3)`. Same source file, same block, same bug. +- **No `aggregation:` key added; no `lsms_library/*.py` touched** — `OK + (anchored on §4)`. + +Evidence (all cold-build): + +| check | before | after | source truth | +|---|---|---|---| +| `household_roster` 2014-15 rows | 5,149 | **37,175** | 37,175 rows in `EACIIND_p1.dta` | +| members/HH (mean) | 1.354 | **9.773** | 9.7726 | +| `(t, i, pid)` unique **in source** | **NO** — 5,149 groups over 37,175 rows | **YES** — 37,175/37,175 | — | +| `individual_education` 2014-15 rows | 2,093 | **4,097** | 4,097 rows with non-null `s02q23` | +| `household_roster` total (all waves) | 188,706 | 220,732 | +32,026 people | + +The "index unique" test on the *returned* frame is `True` both before and after — +that is precisely the disease (§4): the collapse **makes** it unique. The +load-bearing check is uniqueness of the declared key **in the source**, which is +the row above. + +Corroboration, spot-checked against raw `EACIIND_p1.dta`: +- HH `grappe=1, menage=3`: source n=9, ages `[0,1,2,3,5,7,20,28,43]`, 6M/3F. + API n=9, identical ages, 6M/3F, `pid` = `1003001…1003009`. +- HH `grappe=1, menage=6`: source n=15. API n=15, identical 15-age vector, 9F/6M. +- `s01q00` enumerates 1..n cleanly in **all 3,804** households; every household's + `s01q` value is itself one of that household's `s01q00` lines — confirming + `s01q` is a *reference into* the person numbering, not a person id. +- Derived `household_characteristics` 2014-15: 3,781 HH, 36,614 persons, mean + 9.68, max 84 (roster max 84). `log HSize` for HH 1003 = 2.197225 = ln(9) and + for HH 1006 = 2.708050 = ln(15) — matching the 9- and 15-member source + households exactly. The 561-person / 23-HH shortfall vs. 37,175 is the + documented MonthsSpent residence filter (`CLAUDE.md`), not a loss. + +Blast radius, checked exhaustively — every Mali (wave × table) declaring `pid`, +uniqueness of the declared key **in the source**: + +| wave | table | pid cols | rows | uniq(i,pid) | | +|---|---|---|---|---|---| +| 2014-15 | household_roster | `[grappe, menage, s01q00]` | 37,175 | 37,175 | fixed | +| 2014-15 | individual_education | `[grappe, menage, s01q00]` | 37,175 | 37,175 | fixed | +| 2017-18 | household_roster | `[grappe, exploitation, codeid]` | 94,071 | 94,071 | already OK | +| 2017-18 | individual_education | `[grappe, exploitation, codeid]` | 80,600 | 80,600 | already OK | +| 2018-19 | individual_education | `[numind]` | 46,014 | 46,014 | already OK | +| 2021-22 | individual_education | `[numind]` | 43,472 | 43,472 | already OK | + +2018-19 / 2021-22 `household_roster` declare `pid` inside a `dfs:` sub-block +(`s01q00a`, `membres__id`); their returned row counts are byte-identical before +and after and equal their source row counts (46,014 / 43,472), so they carry no +collapse. **2014-15 was the only affected wave.** + +Tests: `tests/test_roster_residence_filter.py` + `tests/test_schema_consistency.py` +— 231 passed against the worktree config tree. diff --git a/.coder/ledger/323-nigeria.md b/.coder/ledger/323-nigeria.md new file mode 100644 index 000000000..53258862f --- /dev/null +++ b/.coder/ledger/323-nigeria.md @@ -0,0 +1,200 @@ +# Prior-Art Ledger — GH #323 (Nigeria) + +**Search tier used:** ripgrep + git floor (the GitNexus MCP server is not mounted +in this subagent; blast radius established by direct call-site enumeration +instead — `_normalize_dataframe_index` has 4 call sites, all internal to +`country.py`). + +## §1 Task, restated + +`_normalize_dataframe_index` collapses a non-unique DECLARED index with +`groupby().first()`, silently discarding the dropped rows. For Nigeria the audit +reports 10 affected cells: `cluster_features` in all 8 W1–W4 quarters, and +`assets` in 2012Q3 + 2013Q1. The task is to fix the *cause* in Nigeria's config, +not to paper over the symptom, and to leave the collapse either impossible or +explicitly DECLARED. + +Two independent root causes, both `INDEX_INCOMPLETE`: + +1. **`v = ea` is not a cluster id.** In the GHS-Panel, `ea` is a serial unique + only *within* an LGA. W1's design is 500 EAs × 10 HH = 5,000 households, but + `nunique(ea) = 411`; `nunique(state, lga, ea) = 500`. So the key MERGES + distinct clusters, and the household→cluster collapse then stamps one + arbitrary EA's Region/District/Rural onto every household in the merged group + (890/5000 wrong District in W1 alone; 1283/5263 by W4). Class-1, silently + WRONG — and it leaks everywhere, because `sample()` publishes the same key and + `_join_v_from_sample()` stamps it onto every household table. +2. **`assets` W2 drops `item_seq`.** `sect5b_plantingw2` is a PER-UNIT roster + (one row per individual unit owned, each with its own age and resale value); + `dup(hhid,item_cd) = 14,493`, `dup(hhid,item_cd,item_seq) = 0`. `.first()` + kept unit #1 and discarded the rest: **₦147,297,485 (25.6%)** of reported + asset value, in each of the wave's two quarters. + +Two defects sat on top of (1): the `dfs:` block merged two HOUSEHOLD-level frames +on `v` (a cartesian product within each EA — W2's `cluster_features` was 62,538 +rows from 4,859 households), and the geo sub-df was silently dropped in 3 of 4 +waves (wrong column casing in W1/W3; W4's geovars file has no `ea` column at +all), so Latitude/Longitude were simply MISSING. + +## §2 Existing machinery (this task's area) + +| symbol | path:line | what it does | tested? | reuse / extend / new | +|--------|-----------|--------------|---------|----------------------| +| `_normalize_dataframe_index` | `country.py:4100` | reorders/drops index levels, then collapses duplicates with `.first()` (or `sum` for `_ADDITIVE_MEASURE_COLUMNS`) | yes | **leave unchanged** (D1: core does not aggregate). The composite key makes its collapse lossless for Region/District/Rural | +| `aggregation:` key | `Albania/_/data_scheme.yml:83` + 7 others | **DEAD**: appears only in the `_skip` meta-key sets (`country.py:2387`, `diagnostics.py:230`); parsed and discarded, never enforced | no | **do NOT use** — D1 rejects making it real; it stays dead config, so Nigeria declares none | +| `Wave.cluster_features` | `country.py:1168` | GH #161: when `i` is in the index, collapses HH→cluster with `.first()` / **`.mean()` for Lat/Lon** | partly | **leave unchanged** — this is #323 Site 2, owned centrally (PR #617), not per-country | +| `_join_v_from_sample` | `country.py:~1620` | left-merges `sample().v` onto every household table | yes | reuse unchanged (but see §4) | +| `community_prices_for_wave` | `nigeria.py:~1990` | builds `v = format_id(ea)` from the community questionnaire | yes | **extend** — same keyspace or the join dies | +| `df_data_grabber` "Trickier" form | `local_tools.py:1034` | `{newvar: ([cols], fn)}` → row-wise transform; auto-bound by NAME (Benin's `i()`) | yes | **reuse** — one `nigeria.v()` serves sample + cluster_features | +| `converted_categoricals:` | `country.py:933` | per-sub-df flag; when set, `get_dataframe(convert_categoricals=False)` → raw CODES | yes (EthiopiaRHS, Albania) | **reuse** — needed for the code-space key | + +## §3 Definitions & conventions in force + +- `sample` is the single source of truth for household→cluster; `v` lives there + and is joined at API time. Per `CLAUDE.md` "`sample()` and Cluster Identity". +- Canonical `assets` grain is **`(t, i, j)`** — `lsms_library/data_info.yml`, + `Index Info > index_info`. A 4-level Nigeria would break `Feature('assets')` + assembly against every 3-level country (the same hazard CLAUDE.md documents for + `food_expenditures`' `s` level). +- Canonical `cluster_features` grain is `(t, v)`; `cluster_features` OWNS `v`. +- class-2 (silently MISSING) is strictly safer than class-1 (silently WRONG). + +## §4 Invariants & assumptions + +- **A NaN `v` is NOT a safe "missing" value.** `_join_v_from_sample` left-merges, + so rows survive — but the guard at `country.py:~1272` documents that the + downstream `groupby()` in `roster_to_characteristics` / the food-derivation + pipeline **drops NaN keys**, silently swallowing those households from every + derived table. So the `ea == 0` "Moved" households cannot be given `v = NA`; + that would trade one silent-data-loss bug for another. They get an explicit + singleton (`moved-{hhid}`) instead — present, never pooled, never dropped. +- **The keyspace must be built from CODES, not labels.** Measured: in W2 the + community questionnaire's `state`/`lga` carry no Stata value labels while the + household file's do, so a label-built composite gives **0%** overlap between + `community_prices.v` and `sample.v`; a code-built one gives **100%**. A + label-built key would have silently severed community prices from their + households. +- `_finalize_result` does `df.dropna(how='all')` (`country.py:2217`): a row whose + every column the `unique` reducer NA's out disappears entirely. Harmless with a + correct key (Region/District are functions of the key), but it is why a + half-migrated state showed W2 at 377 instead of 410 groups. +- `@build_transform()` (`_build_registry.py:69`) folds the decorated function's + SOURCE into every table's cache fingerprint. Editing `_normalize_dataframe_index` + invalidates **all** cached parquets → the next build rebuilds from source. Any + before/after comparison must therefore rebuild both sides from source, or it + compares fresh output against a stale cache and reports phantom regressions. + +## §5 Reuse decision + +| quantity | decision | why | +|---|---|---| +| cluster id `v` | **new** (`nigeria.cluster_id` / `nigeria.v`) | no existing composite-key helper; auto-bound by name so `sample`, `cluster_features` and `community_prices` cannot drift apart | +| HH→cluster collapse | **leave to core** | Ethan's D1 (2026-07-13): core does NOT aggregate on declaration. The composite key makes the existing `.first()` collapse *provably lossless* for Region/District/Rural, which is the whole point — fix the KEY, don't declare a reducer over a broken one | +| `assets` item_seq | **not fixed here** — reported instead | needs two CORE edits (canonical `index_info` + `feature.py`); see §7 | +| Lat/Lon reducer | **leave to core (Site 2)** | `mean()` is retained; auditing the HH→cluster collapse is owned centrally, not per-country | + +## §6 What was built *(config only — `lsms_library/*.py` is UNTOUCHED)* + +This branch is the **config-only port** of `fix/323-nigeria`, off `origin/development`. +The original branch's +139-line `country.py` patch (`_aggregation_policy`, +`_apply_aggregation_policy`, `_unique_or_na`, and a `Wave.cluster_features` hunk) +was **stripped** under D1, together with both `aggregation:` keys it fed +(`cluster_features`, `assets`) — with core stripped those are dead config, since +`aggregation:` lives only in core's `_skip` meta-key set. + +- **`nigeria.py`** — `cluster_id(state, lga, ea, hhid)` + the auto-bound `v(row)`; + `community_prices_for_wave` builds `v` through the same helper. +- **wave YAMLs (all 5)** — `v: [state, lga, ea, hhid]` via a `converted_categoricals` + `df_key` sub-df (raw codes) alongside the labelled `df_main`; geo joined on `i` + (not `v`) — un-cartesianing the merge; per-wave geo column casing corrected. + W5's `sample` is fixed too: it has no `cluster_features`, but its `sample.v` is + stamped on every household table and was equally broken. +- **`data_scheme.yml`** — no `aggregation:` key. Comments record *why* the collapse + is now sound (the key, not a reducer), and carry the `assets` open defect. + +### Measured, per round (`LSMS_NO_CACHE=1`, vs. a real `development` baseline build) + +`v` nunique in `sample`, **development → this branch**: + +| wave | pp quarter | ph quarter | +|---|---|---| +| 2010-11 | 2010Q3: 411 → **500** | 2011Q1: 411 → **500** | +| 2012-13 | 2012Q3: 408 → 647 | 2013Q1: 408 → 647 | +| 2015-16 | 2015Q3: 402 → 792 | 2016Q1: 402 → 792 | +| 2018-19 | 2018Q3: 405 → 685 | 2019Q1: 405 → 685 | +| 2023-24 | 2023Q3: 404 → 911 | 2024Q1: 404 → 911 | + +W1's 500-EA design is recovered **in both rounds**. (Post-W1 counts include the +`moved-{hhid}` singletons: e.g. W3 = 486 real clusters + 306 movers = 792.) + +`cluster_features` Latitude present, **development → this branch**: 2010Q3 0 → 500, +2012Q3 409 → 645, 2015Q3 0 → 784, 2018Q3 0 → 671 (and identically in each wave's +ph quarter). Coordinates recovered in 3 of the 4 waves that declare the table. + +### Round safety (the pp/ph question) + +- **`v` is round-invariant**: 0 households change cluster between the pp and ph + quarter, in all 5 waves. It holds *by construction* — a wave folder has ONE + `data_info.yml` and both `Wave(t)`s read the SAME post-planting cover page, so + there is no second geo-coding to drift from. +- Against the *raw* cover pages the only pp/ph disagreement is the `Moved` + sentinel: 0 / 17 / 41 / 66 / 64 households (W1…W5) are in a real EA at planting + and coded `ea == 0` at harvest, i.e. they relocated between the two visits. + **Zero** real-EA→real-EA disagreements — so no geo-recode, no label/code drift, + no `.0` float-suffix artefact. Using the planting frame's cluster for the whole + wave is the right call: `v` is the *sampling* cluster, fixed by the frame. +- The `Moved` sentinel fires when it should and **only** when it should: 0 times in + W1 (nobody can have moved out of a frame they were just drawn from), and + 152/306/166/393 times in W2–W5. +- `cluster_features` carries **both** quarters with identical cluster sets and zero + attribute mismatches — neither round overwrites the other. 0 duplicate `(t,v)`. +- `community_prices` is correctly post-harvest-only. Structurally verified: across + all 5 waves there are **11 `sectc8*` files and every one is `_harvest`** — no + `sectc8*_planting*` exists. The planting community files that *do* carry + `item_cd` (`sectc2_plantingw1`, `sectc2a/b_plantingw3`) have **no price column**; + C2 is an availability module. No planting-round price data is being dropped. +- `sample` grain is unchanged by the rekey: 49,770 rows on `development` and on this + branch — one row per (household, round), 0 duplicate `(i,t)`. + +## §7 Residuals / honest gaps + +- **`assets` item_seq is NOT fixed and is still broken.** W2's per-unit roster + (`sect5b_plantingw2`, `item_seq` 1..15) is collapsed by `groupby().first()`, + keeping unit #1: true ₦576,299,043 → kept ₦429,001,558 → **₦147,297,485 (25.6%) + destroyed**, in *each* of 2012Q3 and 2013Q1. The original branch fixed this with + an `aggregation:` reducer, which D1 rejects. Declaring `item_seq` in Nigeria's + index does **not** close it either — *verified*: the canonical assets grain is + `(t,i,j)`, so `Feature('assets')` calls `_harmonize_country_frame`, drops the + extra level, and `_collapse_duplicate_index` re-reduces with `first()` (assets is + not in `_ADDITIVE_MEASURE_COLUMNS`). A 4-level Nigeria would fix + `Country.assets()`, leave `Feature('assets')` destroying the identical ₦147M, and + make Nigeria the library's only non-canonical assets grain. The real fix is two + CORE edits and belongs on its own PR: + 1. `lsms_library/data_info.yml` → `index_info: assets: (t, i, j, item_seq)` + 2. `lsms_library/feature.py` → `_ADDITIVE_MEASURE_COLUMNS['assets'] = ('Value',)` + (`Quantity` must stay `first` — it comes from the clean `sect5a` hh×item grid + and is merely repeated across the `item_seq` rows; `Age` wants `mean`.) + Recorded as a KNOWN OPEN DEFECT block in `Nigeria/_/data_scheme.yml`. +- **Lat/Lon still collapse with `.mean()`** (core's Site-2 default). 5 / 24 / 50 + clusters in W2 / W3 / W4 span more than one coordinate — the panel keeps a moved + household's ORIGINAL `ea` in the frame while the geovariables record its CURRENT + dwelling, so households up to ~890 km apart get a centroid in neither place. The + original branch NA'd these via a `unique` reducer; under D1 that is core's Site 2 + to own, not Nigeria's. Behaviour here is the historical one, not a regression. +- **`moved-` singletons inflate `cluster_features` row counts** (e.g. W3: 792 rows = + 486 real clusters + 306 one-household "clusters"). Deliberate: a NaN `v` would be + dropped by the downstream `groupby()` and delete those households from every + derived table. Analysts computing cluster-level means should be aware. +- **`v` is wave-local, not panel-stable.** LGA codes are recoded between waves, so + the composite is comparable within a wave, not across. The previous `v = ea` was + numerically comparable across waves but *meaningless* (it conflated clusters), so + no real cross-wave linkage is lost. +- **~0.2–0.4% of `community_prices` clusters do not match a `sample` cluster** in + W2/W3 (join rate 99.6% / 99.8%; 100% in W1/W4/W5) — community questionnaires + administered in EAs where no sampled household resolved. Left visible rather than + force-matched. +- **W5 declares no `cluster_features`**, so `community_prices` at 2024Q1 joins + `sample.v` at 100% but `cluster_features` at 0%. Pre-existing gap, not a + regression from this port. +- W1 `assets` still has no `Value`/`Age` (the W1 YAML never extracted them; the + source section has no such columns). Pre-existing, unrelated to #323. diff --git a/.coder/ledger/323-site2.md b/.coder/ledger/323-site2.md new file mode 100644 index 000000000..29ce08c7a --- /dev/null +++ b/.coder/ledger/323-site2.md @@ -0,0 +1,200 @@ +# Prior-Art Ledger — GH #323 SITE 2 (`Wave.cluster_features`) + +**Search tier used:** the six country branches that already diagnosed this site +(`fix/323-{albania,cambodia,kazakhstan,nigeria,tajikistan,tanzania}`), the +consolidating design note +(`slurm_logs/DESIGN_grain_collapse_sites_2026-07-13.org`, branch +`docs/323-grain-collapse-sites`), plus direct measurement of all 40 countries +against the pre-collapse wave frame with `LSMS_NO_CACHE=1`. + +## §1 Task, restated + +`Wave.cluster_features` (`country.py`, from GH #161) contains a **second, +hardcoded grain collapse**, entirely separate from `_normalize_dataframe_index`. +Seventeen countries declare `i: ` in their `cluster_features` `idxvars` (so +the YAML can merge a household-level GPS frame); the method reduced that +HOUSEHOLD-grain table to the declared `(t, v)` cluster grain with `.first()` for +attributes and `.mean()` for GPS — **before `_normalize_dataframe_index` ever +runs**. PR #614's audit therefore cannot see this loss: by the time Site 1 fires, +the rows are already gone. (The `.mean()` is now retired too — §5.) + +The reduction was licensed by prose and nothing else: + +> "Region/Rural/District are invariant within a cluster by construction of the +> LSMS-ISA sampling design." + +The claim is **false**, and its falsity is not benign. Where a cluster code is +unique only *within* a district, two real clusters collide on one `v` and +`.first()` keeps one of their Regions at random. The output is not a lossy summary +of the input — it is a **wrong row**. + +## §2 Existing machinery (do not reinvent) + +| symbol | path | what it does | reuse / extend / new | +|--------|------|--------------|----------------------| +| `_audit_index_collapse` | `country.py` | measures destroyed rows + `nan_key_rows` before a collapse; `None` when provably lossless | **reuse verbatim** | +| `_record_grain_report` / `_GRAIN_LEDGER` | `country.py` | emits `GrainCollapseWarning` (fatal under `LSMS_GRAIN_STRICT`) and files the report under `(country, table)` for the cache writer | **reuse verbatim** | +| `_format_grain_report` | `country.py` | renders the report | **extend** — a `site` key so Site 2's message says what actually happened | +| `to_parquet(grain_audit=)` / `read_parquet_grain_audit` / `_replay_grain_audit` | `local_tools.py`, `country.py` | stamps the audit into the L2 parquet schema metadata and replays it on the warm read | **reuse verbatim** — Site 2 files under the same ledger key, so it is carried for free | +| `LSMS_CACHE_SCHEMA` | `local_tools.py` | manual cache-invalidation lever | **bump 2 -> 3** (schema-2 parquets were stamped by a build blind to Site 2) | +| the six country branches' `Wave.cluster_features` hunks | `fix/323-*` | correct DIAGNOSIS, rejected MECHANISM (Design A: declared reducers) | **salvage evidence, discard code** | + +## §3 Definitions & conventions in force + +- **NO AGGREGATION IN CORE** — `SkunkWorks/grain_aggregation_policy.org`. Cited, + not restated. +- **D1 (Ethan, 2026-07-13)**: core does not aggregate. No `aggregation:` YAML key, + no declared-reducer mechanism. Audit and be loud, reusing #614's machinery. +- **D2 (Ethan, 2026-07-13)**: NaN-key rows are **deleted and reported**, not + retained. Retaining would change returned data library-wide. +- The `.first()`/`.mean()` split at this site predated the policy (GH #161) and was + the *last* aggregation core performed anywhere. **Retired 2026-07-13** — the + contract is now exceptionless. See §5. + +## §4 Invariants & assumptions (landmines) + +1. **`_audit_index_collapse` compares WHOLE ROWS via `reset_index()`.** Passing it + the household-grain frame with `i` still in the index makes *every* cluster with + two households look destructive — `i` is distinct by construction. ~100% false + positives, and the real finding drowns. **The dropped levels must be + `droplevel`'d before auditing.** This is not hypothetical: the first cut of this + patch did exactly that and reported **2,304** destroyed rows for Uganda 2019-20 + instead of the true **947**. It was caught only because a test asserted that a + *lossless* projection stays silent. +2. **`groupby().first()` skips NA per column** (verified, pandas 3.0.2). So a + conflicting cluster does not collapse to one of its households — it collapses to + a **composite** assembled from the first non-null value of each column + independently: a household the survey never interviewed. This also means the + design note's claim that "`first()` can keep an all-NaN row while discarding the + row with real coordinates" is **false**; measured `first_nan_but_mean_real = 0` + across the corpus. +3. ~~**Household GPS varies within a cluster by design.**~~ **REFUTED BY THE + CORPUS** — this was the assumption behind the `.mean()`, and it is wrong: in 4 of + the 5 GPS cells the fix is perfectly *constant* within each cluster (it is the + cluster's own displaced fix, stamped on each household). GPS is now audited and + `.first()`-ed like every other column; the `.mean()` is retired. See §5. +4. **The L2-wave parquet is written PRE-collapse** (`grab_data` caches before + `cluster_features()` reduces), so Site 2's audit re-runs on every wave read, + warm or cold. Only the **L2-country** parquet can hide it — hence the stamp. +5. **`i`-as-a-COLUMN is Site 2's twin and is out of scope.** Uganda's seven other + waves put `i` in a merge block, so it arrives as a column, the `'i' in + df.index.names` guard misses it, and `df.drop(columns='i')` launders an + household-grain frame into a `(t, v)` frame with unexplained duplicates that + Site 1 then collapses (9,890 destroyed + 934 NaN-key — all reported by #614). + Not silent, therefore not rerouted — and now that the `.mean()` is retired both + paths reduce identically, so the asymmetry is gone too. Documented in a comment. + +## §5 The `.mean()` on Latitude/Longitude — DECIDED: retired (Ethan, 2026-07-13) + +The `.mean()` was an aggregation performed by core, and thus in breach of the +contract in §3. It was referred up with numbers rather than decided unilaterally. +**Decision: option (b) — make it loud. Core aggregates nothing here.** + +### The numbers that decided it + +Census of every cell where the `.mean()` could fire (5 cells, all countries, +`LSMS_NO_CACHE=1`): + +| country | wave | clusters | clusters where `.mean()` actually averages | +|---------|------|---------:|-------------------------------------------:| +| Malawi | 2010-11 | 768 | **0** | +| Malawi | 2013-14 | 204 | **188** | +| Malawi | 2016-17 | 881 | **0** | +| Malawi | 2019-20 | 819 | **0** | +| Niger | 2021-22 | 555 | **0** | + +In **four of the five** cells the `.mean()` is a provable **no-op**: every household +in a cluster carries the same fix, because the published GPS *is* the cluster's +(displaced) fix stamped onto each household. It was never household GPS at all. + +In the one cell where it does fire — Malawi 2013-14 — the numbers say the data is +broken, not that a centroid is being computed: + +- median within-cluster GPS spread: **148 km**; max: **783 km** +- median distance between `.mean()` and `.first()`: **9.0 km**; max: **556 km** + +A 783-km "cluster" is not a cluster. And that same cell was *already* warning for +Region (93 clusters), District (165) and Rural (131) — the GPS non-constancy and the +attribute disagreement have **one shared root cause**: the cluster key is broken. +The `.mean()` was not deriving a centroid there; it was **averaging over a broken +identifier and hiding the evidence**. + +Two further arguments, both from §4.5: + +- It was applied **incoherently**. Whether a country's cluster coordinates came out + a *centroid* (`.mean()`, Site 2) or *one household's fix* (`.first()`, Site 1) was + decided by nothing more principled than whether its YAML put `i` in `idxvars` or + left it in a merge block. Nobody chose that. +- A country with genuinely per-household GPS has put a household-level column in a + cluster-grain table. The fix is to move the column, not to teach core to average. + +### Cost of the flip: zero, and verified as zero + +The `~100% false-positive` fear that motivated grandfathering does not materialize. +Measured **end-to-end through the real `Wave.cluster_features`**, before vs. after: + +| country | wave | destroyed (a) | destroyed (b) | warned before? | warns after? | +|---------|------|--------------:|--------------:|----------------|--------------| +| Malawi | 2010-11 | 0 | 0 | silent | **silent** | +| Malawi | 2013-14 | 3,225 | 3,540 | warns | warns | +| Malawi | 2016-17 | 1,831 | 14,277 | warns | warns | +| Malawi | 2019-20 | 3,038 | 4,838 | warns | warns | +| Niger | 2021-22 | 0 | 0 | silent | **silent** | + +**NEW warning cells: 0. Cells that stopped warning: 0.** The warning set is the same +15 cells; only the row counts inside three already-warning cells go up (corpus total +38,638 → **53,199** destroyed). Both silent GPS cells stay silent, which is the +whole point: where the survey stamps one fix per cluster, the projection is lossless +and the audit says nothing. + +> Trap worth recording: the *first* census used `nunique(dropna=True) > 1`, which +> does **not** catch a cluster where some households have a fix and others have NaN. +> The row-wise audit *does* count that as disagreement (missing counts as a value). +> So `gps_averaged_groups == 0` did **not** imply "no new warnings", and the claim +> had to be re-measured with the actual audit before it could be trusted. Malawi +> 2016-17 gains 12,446 rows entirely from that case. The conclusion survived, but it +> was not safe to assume. + +### Implemented + +`_collapse_to_cluster_grain` now audits the **whole frame** and reduces every column +with `.first()`. `_CLUSTER_GPS_COLUMNS`, `_gps_averaging_stats` and the +`gps_averaged_*` report keys are **deleted** — there is no per-column special case +left to drift. Pinned by `test_gps_is_not_averaged__core_does_not_aggregate`, +`test_gps_variation_now_warns`, `test_constant_gps_within_a_cluster_stays_silent` +and `test_no_column_is_special_cased_any_more`. + +## §6 Evidence — the corpus (30 HH-grain cells, 17 countries) + +15 of 30 cells destroy rows. **53,199 rows destroyed, 13,704 deleted on a NaN key** +(38,638 of the destroyed rows are attribute disagreements; the balance is GPS, now +that GPS is audited too — see §5). None of it visible to #614. Headline cells: + +| country | wave | HH rows | clusters | destroyed | NaN-key | disagreeing attributes | +|---------|------|--------:|---------:|----------:|--------:|------------------------| +| Malawi | 2004-05 | 11,280 | 110 | 10,581 | 0 | Region:66, District:79, Rural:19 | +| Guatemala | 2000 | 7,276 | 8 | 7,268 | 0 | Rural:8 | +| Pakistan | 1991 | 4,957 | 301 | 3,300 | 0 | Region:75, Language:141 | +| Tanzania | 2020-21 | 4,709 | 418 | 3,289 | 545 | Region:199, District:283, Rural:75 | +| Malawi | 2013-14 | 4,000 | 204 | 3,540 | 0 | Region:93, District:165, Rural:131 (+GPS) | +| Malawi | 2016-17 | 27,402 | 881 | 14,277 | **12,447** | Region:1, Rural:75 (+GPS) | +| **Kazakhstan** | 1996 | 7,224 | 135 | 209 | 0 | **Region:1, Rural:1** | + +**Kazakhstan is the one that matters** — the smallest number in the table and the +clearest corruption. Cluster `v=126` contains **178 Urban households and 2 Rural**; +`.first()` returns **`Rural`** for the whole cluster. Cluster `v=5` contains 29 +`Akmolins` and 2 `Aktobink`. This is silently *wrong* data, in the library today, +and #614 is structurally blind to it. + +Confirmed SILENT (correctly — these projections are provably lossless, which is +what Cambodia / Tajikistan / GhanaLSS were asking to be able to *say*, and they now +get it with no YAML and no country core code): Cambodia (0/252), Tajikistan +(0/767 cluster-waves), South Africa (0/355), Burkina Faso, China, Kosovo, Serbia, +Albania 2002/2005, Malawi 2010-11, Niger 2021-22. + +## §7 What I could not verify + +- The `disagreeing_cols` counts are per-column cluster counts, not row counts; the + `destroyed` column is the row count and is the authoritative figure. +- I did not re-derive the *correct* cluster key for any country. Every finding here + is a report, not a repair — per D1, fixing the identifier is country-side work. diff --git a/.coder/ledger/323-site4-dfs-merge.md b/.coder/ledger/323-site4-dfs-merge.md new file mode 100644 index 000000000..f32e89a61 --- /dev/null +++ b/.coder/ledger/323-site4-dfs-merge.md @@ -0,0 +1,438 @@ +# Prior-Art Ledger — GH #323 site 4 (the `dfs:` merge) + +**Search tier used:** ripgrep + git floor over `lsms_library/country.py`, every +`dfs:` block in `countries/*/*/_/data_info.yml` (**76** at the branch's base +`45aee170`; **80** after merging `development` — see §6 and §12; an earlier draft +of this line said 47, which was neither count), and the 17 core patches on the +`fix/323-*` branches. The affected surface is one function, so the floor is +complete. + +## §1 Task, restated +`Wave.grab_data`'s `dfs:` sub-dataframe merge did `pd.merge(..., how='outer')` +with no cardinality guard. When `merge_on` is non-unique in **both** sub-frames +the merge is many-to-many and emits a **cartesian product within each key +group** — it MANUFACTURES the duplicate rows that sites 1–3 of #323 then +silently collapse. This is upstream of the whole class. Fix the merge; do not +reduce the explosion afterwards. + +Second, smaller defect in the same block: the GH #515 optional-sub-df fallback +swallows a `KeyError` and drops the sub-df. Where that sub-df was the sole +supplier of a **required** declared column, the table is served with the column +100% absent, behind a warning. + +## §2 Existing machinery (this task's area) + +| symbol | path | what it does | tested? | reuse / extend / new | +|---|---|---|---|---| +| `Wave.grab_data` `dfs:` merge | `country.py:~1032` | `pd.merge(..., how='outer')` on `merge_on` | no | **extend** — cardinality guard + `merge_how:` | +| GH #515 optional-sub-df fallback | `country.py:~1000-1022` | swallows KeyError, drops sub-df with a warning | no | **extend** — hard error when the drop costs a required column | +| `Country._assert_built_required_columns` | `country.py:2355` | required-vs-optional scheme-column parsing (`_skip` set + `optional:`) | yes | **reuse** — factored out to module-level `_required_scheme_columns` and shared | +| `_merge_subframes` / `_required_scheme_columns` | `fix/323-ethiopia` | the same guard, entangled with a rejected Design-A core patch | its own tests | **salvage** — guard kept; the `aggregation:` reader discarded per D1 | +| `_grain_strict()` / `LSMS_GRAIN_STRICT` | `country.py:~4409` (PR #614, site 1) | warn-by-default / fatal-under-env escalation lever | yes (in #614) | **reuse the SYMBOL** — see the correction below | +| `_normalize_dataframe_index` | `country.py:~4190` | site 1 — the generic collapse | — | **do not touch** (PR #614) | +| `Wave.cluster_features` | `country.py:~1177` | site 2 — the hardcoded collapse | — | **do not touch** (PR #617) | + +## §3 Definitions & conventions in force +- **Core does not aggregate** — D1 of `DESIGN_grain_collapse_sites_2026-07-13.org`, + upholding `SkunkWorks/grain_aggregation_policy.org`. No `aggregation:` key is + wired up here. A cartesian is fixed by making the merge correct, never by + reducing afterwards. +- Required vs optional declared columns: a `data_scheme.yml` entry key is + REQUIRED unless `optional: true` (`country.py`, `_SCHEME_NON_COLUMN_KEYS`). +- `cluster_features` owns `v`; no other feature puts `v` in its index — CLAUDE.md. +- class-2 (silently MISSING) is safer than class-1 (silently WRONG) — but a + *required* column silently missing from a served table is not a tolerable + class-2; nothing downstream will ever notice it. + +## §4 Invariants & assumptions +- **The cartesian test is exact, not a heuristic.** A join on `keys` is + many-to-many *iff* some key value is duplicated in BOTH frames. Sound and + complete. The row-count ceiling `len(out) <= len(left) + len(right)` that the + Ethiopia branch's docstring claimed as a "proof" is sound but **not + complete** — two 3-row frames sharing one 2×2 key and one 1×1 key yield 5 + rows and never breach the 6-row ceiling. Rejected. +- **Null keys count.** `pd.merge` MATCHES null keys to each other, so a null key + duplicated on both sides explodes like any other. `dropna=False` in the + groupby is load-bearing. (Distinct from site 3, where `groupby().first()` + *deletes* NaN-keyed rows; here they *multiply*.) +- **Severity is split, deliberately.** The cartesian guard WARNS (fatal under + `LSMS_GRAIN_STRICT`, read via `_grain_strict()`) because raising would break + countries whose configs this PR may not touch, and because it changes no + returned data — it reports. The required-column check RAISES, because it is + the one failure that no downstream mechanism will ever catch. *(An earlier + draft of this bullet said its blast radius was "a single country whose config + fix is already written". It was **three** countries and ten cells — see §6 — + all now fixed and merged, §10.)* +- **What the required-column check actually asserts.** "A mis-named column in a + `dfs:` sub-df is fatal" — NOT "a required column is never absent". It fires + only when a sub-df was dropped, so a wave with no `dfs:` block is outside its + reach (Niger 2014-15, §11.3). And its escape hatch `optional: true` is + **country**-grain while the check is per-wave. + +## §5 Reuse decision + +| quantity | decision | reason | +|---|---|---| +| cartesian detection | **salvage + correct** Ethiopia's `_merge_subframes` | keep the both-sides-duplicated test; replace the row-count-ceiling justification, which is unsound as stated | +| `merge_how:` YAML key | **keep** | not dead config — core reads it (`data_info.get('merge_how', 'outer')`), all five Ethiopia waves set `merge_how: left`, and it is tested end-to-end here | +| `aggregation:` YAML key | **discard** | D1. It stays in `_SCHEME_NON_COLUMN_KEYS` only so an old config carrying one is not mistaken for a required column | +| required-vs-optional parsing | **reuse** — factored to module-level `_required_scheme_columns`, shared with `Country._assert_built_required_columns` | two guards that disagree on "required" mean one of them lies | +| `LSMS_GRAIN_STRICT` reader | **CORRECTED after review** — call `_grain_strict()`, do not re-read the env | see §11.1: the private copy was not bit-identical, and the divergence was measurable | + +## §6 The 40-country census (measured, `LSMS_NO_CACHE=1`, config tree at `45aee170`) + +> **Correction (post-review).** This section originally opened *"67 `dfs:` +> merges exercised across 19 countries (the other 21 declare none)."* The +> declaration count is wrong and is checkable statically: parsing every +> `countries/*/*/_/data_info.yml` at `45aee170` (the branch's base) yields +> **76 `dfs:` blocks across 20 countries** — the reviewer's independent count, +> reproduced here. 67 was the number of merges the sweep actually *executed*, +> which is smaller (Nepal's 2 blocks cannot load; some declared wave dirs are +> not in `Country.waves`). Two different quantities were reported as one. The +> per-cell table below is unaffected and was reproduced to the row by the +> reviewer. + +**8 cartesian cells / 5 countries / 4,907,774 phantom rows.** Every one is +`cluster_features` merged on the CLUSTER key `v` when both sub-frames are +household-grain. + +| country | wave | left | right | merged | phantom | +|---|---|---:|---:|---:|---:| +| Mali | 2021-22 | 393,480 | 6,143 | 4,718,148 | **4,324,668** | +| Malawi | 2010-11 | 12,271 | 12,271 | 196,083 | 183,812 | +| Malawi | 2019-20 | 14,612 | 11,434 | 185,842 | 171,230 | +| Ethiopia | 2013-14 | 5,262 | 5,287 | 65,508 | 60,221 | +| Nigeria | 2012Q3 | 4,859 | 4,802 | 62,538 | 57,476 | +| Nigeria | 2013Q1 | 4,859 | 4,802 | 62,538 | 57,476 | +| Ethiopia | 2015-16 | 4,954 | 4,954 | 57,786 | 52,832 | +| Guinea-Bissau | 2018-19 | 5,351 | 450 | 5,410 | 59 | + +Ethiopia — the only case the branches knew about — is **2.3%** of the total. +Mali 2021-22 alone is 88%: a 393,480-row (individual-grain) frame joined to a +6,143-row frame on `v`. + +**10 required-column errors / 3 countries.** All true positives; the data is +usually sitting in the file under a different name: + +| country | waves | cause | fix | +|---|---|---|---| +| Nigeria | 2010Q3, 2011Q1 | YAML asks `LAT_DD_MOD`; file has `lat_dd_mod` | casing | +| Nigeria | 2015Q3, 2016Q1 | YAML asks `lat_dd_mod`; file has `LAT_DD_MOD` | casing | +| Nigeria | 2018Q3, 2019Q1 | geo file is keyed `hhid`, YAML asks `ea` | re-key to `i: hhid` | +| Ethiopia | 2011-12, 2018-19, 2021-22 | casing / missing `ea_id` | fixed on `fix/323-ethiopia` → merged as #628 | +| Niger | 2011-12 | **the YAML pointed `df_geo` at the wrong file** | point it at the sibling `NER_EA_Offsets.dta` — merged in `3488b791` | + +Nigeria has been served **without GPS in six waves** for as long as the #515 +fallback has existed, because a `KeyError` on a case-mismatched column name was +swallowed and the table reported clean. + +> **Correction (post-review).** The Niger row originally read *"geo file has +> **no** lat/lon column at all"*, prescribing `optional: true`. Half right and +> wholly the wrong conclusion. `NER_HouseholdGeovars_Y1.dta` (4051 × 43) indeed +> has no coordinate column — re-verified here from source, derived raster +> covariates only. But the coordinates ship in a **sibling file in the same, +> already-DVC-tracked directory**: `NER_EA_Offsets.dta`, `271 × ['grappe', +> 'LAT_DD_MOD', 'LON_DD_MOD']`, 270 unique non-null grappes, latitudes +> 11.876–18.747 N — measured here, not quoted. `optional: true` would have +> deleted a real column *and*, because `data_scheme.yml` is country-grain, +> disarmed this PR's own guard for the three Niger waves that do carry +> coordinates. The fix that landed (`3488b791`) re-points the file; Niger +> 2011-12 now serves 270 clusters with 270 coordinates. **This is the exact +> failure mode CLAUDE.md warns about under "Adjudicating `absent` cells": never +> write an unevidenced "no module here" claim.** + +## §7 PP/PH round structure (coordinator's addendum) — measured, not assumed + +The hypothesis was that a sub-frame keyed without the round dimension could +generate a cartesian *across* rounds. **Empirically empty at this site:** + +- **0** `dfs:` sub-frame library-wide is drawn from a file whose **name** carries + a `_pp_` / `_ph_` round marker. PP/PH round-splitting happens exclusively on + the *script* path (`materialize: make`), which never enters + `Wave.grab_data`'s `dfs:` merge. + + > **Correction (post-review).** As originally written ("0 of the 67 sub-frames + > is drawn from a `_pp_`/`_ph_` round file") this overstated a **filename** + > test into a claim about provenance. Nigeria's `df_main` *is* a post-planting + > file — `Post Planting Wave 1/.../secta_plantingw1.dta` — it just does not + > carry the marker in its basename. The substantive conclusion is unchanged + > and rests on the two bullets below, which are about round *structure*, not + > filenames: `t` is constant within every `dfs:` merge in the corpus, so no + > merge here can cross rounds. +- **Tanzania** declares no `dfs:` block at all, so its `2008-15/` multi-round + folder cannot reach this code. +- **Nigeria** is PP/PH *and* has a `wave_folder_map`, and `Country.waves` + already returns ROUND labels (`2012Q3`, `2013Q1`, …) — so the census is + natively per-round for the one PP/PH country that has `dfs:` blocks. The + identical figures for 2012Q3 and 2013Q1 are the same folder read under two + `t` values, each independently cartesian. +- **Ethiopia** `cluster_features` sources the household **cover** file + (`sect_cover_hh_w2.dta`), not a sectional PP/PH file, and `wave_folder_map` + is empty. `t` is constant within the merge; there is no round dimension. + +## §8 Correction to the brief: the re-key is `household_id2`, not `household_id` + +The task brief said the Ethiopia fix re-keys `v: ea_id` → `i: household_id`. +**Measured, `household_id` is NOT unique** in either sub-frame of W2/W3 — it is +the *W1* id, blank for households new to the panel, so it repeats on the empty +value. Re-keying to it would have traded an EA-grain cartesian for a **null-key** +cartesian (`pd.merge` matches nulls), which is precisely what the `dropna=False` +in `_cartesian_keys` exists to catch. The unique key is **`household_id2`**, and +`fix/323-ethiopia` correctly uses it. + +Verified end-to-end: **this core + `fix/323-ethiopia`'s config = all 5 waves +clean**, 0 cartesians, 0 required-column errors, and `merge_how: left` honoured +(merged rows == left rows exactly, in all five). + +## §9 The suite is green, and the green is NOT proof + +> **Status note (post-review).** The three raises below are **fixed** — see +> §10. This section is kept because its *lesson* is what the reviewer, the +> Ethiopia agent and the Malawi agent each independently re-derived: a green +> suite was not evidence here, and neither is a `sane` coverage cell. The row +> counts and mtimes are a snapshot of `a54568f9`, not of the current branch. + +Full suite at `a54568f9`: **3 failed, 3546 passed, 128 skipped** — and the 3 are +exactly the 3 pre-existing failures (`test_currency::test_feature_ghana_per_wave`, +`test_table_structure::*[CotedIvoire/cluster_features]` ×2). **Zero new +failures.** + +That green does not mean what it appears to mean, and the reason is #323's own +pathology. Verified at the time by direct API call on a warm cache: + +``` +Country('Ethiopia').cluster_features() -> RuntimeError +Country('Nigeria').cluster_features() -> RuntimeError +Country('Niger').cluster_features() -> RuntimeError +``` + +The suite does not see it, for two different reasons: + +- **Ethiopia, Niger** — `test_table_structure` enumerates `var/*.parquet` and + reads them with `pd.read_parquet`, by its own stated contract ("only test what + is already cached"). It **never rebuilds and never checks the cache hash**. + Both countries still carry a `cluster_features.parquet` written *yesterday by + the pre-change code* (mtime 2026-07-12 21:16). The test reads the old file and + passes. +- **Nigeria** — has **no** cached `cluster_features.parquet` at all, so the cell + is simply absent from `_find_cached_parquets()` and drops out of the test + matrix silently. Not tested, not reported. + +This is the same sentence the design note opens with: *the bug hid behind the +cache that the bug poisoned.* Here it is hiding my own change's blast radius. +**Do not read the green tick as clearance for these three countries.** + +One correction to how this was framed, from re-deriving it under review: the +mechanism above is `test_table_structure`'s stated read-only contract, **not** +the v0.8.0 hash failing to notice a core change. The hash does notice — see +§11.5. Both facts are needed: a cache-reading test is blind to a build-path +guard *even though* the build-path guard correctly invalidates the cache, because +the test never asks for a build. + +## §10 Sequencing — RESOLVED (was "open questions for the human") + +The blocker is discharged. Every config named below is now an ancestor of +`origin/development`, and this branch has been merged with `development` so the +PR carries them. + +| country | fix | status | re-verified here, cold | +|---|---|---|---| +| Ethiopia | #628 (`3488b791`) + #644 | merged | ✅ 2,168 rows, 5/5 waves, Latitude on 2,146 | +| Niger | #628 (`3488b791`) | merged | ✅ 1,599 rows, 4/4 waves, 2011-12 now 270/270 coords | +| Nigeria | #625 (`194b55d0`) | merged | ✅ 5,248 rows, Latitude on 5,200, 8 round-waves | + +Method for that column: this branch merged with `origin/development`, imported +with `lsms_library.__file__` **asserted** into the worktree, config via +`LSMS_COUNTRIES_ROOT`, and an **isolated `LSMS_DATA_DIR`** wiped before the run +with only `dvc-cache` symlinked. `Country(c).cluster_features()` for all three: +**0 raises, 0 site-4 cartesian warnings.** + +**Still open:** + +- **When to flip `LSMS_GRAIN_STRICT` on in CI.** Corrected framing: this is + *not* site 4's decision to make. The variable is now read by sites 1, 2 and 4 + through one predicate, so turning it on makes **all three** fatal. Measured + today: with `LSMS_GRAIN_STRICT=1`, Ethiopia / Niger / Nigeria + `cluster_features()` all raise `GrainCollapseError` — from **site 2**, the + household→cluster projection, not from this site. Site 4's own census being + clean is necessary and nowhere near sufficient. +- **The remaining cartesian cells.** Mali 2021-22 (4,324,668 phantom) is fixed + and merged (#641). Ethiopia's two are cleared by #628. Malawi 2010-11 / + 2019-20 and Guinea-Bissau 2018-19 are written but **open** in #653. Nigeria + 2012Q3 / 2013Q1 (57,476 each) — check against #625, which re-keyed `v`. + §12 records the census as re-measured on the merged tree. + +## §11 Response to the adversarial review (2026-07-22) + +The review returned **FIX_FIRST** with the code judged correct — every headline +number reproduced to the row and all eight mutants were killed by the tests. +What follows is what changed and what is disputed. Where a finding is disputed +it is disputed **with a measurement**, not an argument. + +### §11.1 Reuse: `LSMS_GRAIN_STRICT` must be read through `_grain_strict()` — FIXED + +§2 of this ledger asserted the lever was *"not yet on `development`; defined +independently here, no symbol conflict."* That was true when written and is +false now: PR #614 merged, and `_grain_strict()` lives in this same module. The +private copy was **not** bit-identical — `os.environ.get('LSMS_GRAIN_STRICT')` +is truthy for `"0"` and `"false"`, `_grain_strict()` is not. Measured: +`LSMS_GRAIN_STRICT=0` made site 4 **raise** while site 1 stayed in warn mode. +One lever with two readers is a future defect; now there is one reader. +Discriminating test: `test_grain_strict_is_read_through_the_one_shared_predicate` +(fails on the old implementation for `0`/`false`/`no`/`off`). + +### §11.2 The required-column check judged "present" too early — FIXED + +It ran between `set_index` and `apply_derived`, while its country-level twin +`_assert_built_required_columns` runs **post-**`_finalize_result`. Two guards +sharing `_required_scheme_columns` but disagreeing on *when* presence is judged +is the same trap the shared helper was factored out to avoid. Moved below +`derived:`, `drop:` and the `df_edit` hook. Discriminating test: +`test_required_column_supplied_by_the_wave_hook_is_not_reported_absent` (raises +`RuntimeError` with the check in its old position). Latent when written — no +live config depended on it — fixed anyway, because latent is a property of +today's configs, not of the code. + +### §11.3 The escape hatch is country-wide, the invariant is not — DOCUMENTED + +`data_scheme.yml` is one file per **country**, so `optional: true` disarms the +column for every wave and every script-path build, while the check is per-wave. +The error message said *"if they are genuinely unavailable for this wave"*, +which reads as if the hatch were per-wave; it now says so plainly and points at +the column name as the real fix. Pinned by +`test_hard_error_says_optional_true_is_country_wide`. + +The reviewer's sharper point is recorded in CLAUDE.md and the skill: **read this +guard as "a mis-named column in a `dfs:` sub-df is fatal", not "a required +column is never absent."** It fires only on a *dropped sub-df*, so a wave with +no `dfs:` block can serve a declared column 100% absent and never trip it. +Verified: Niger **2014-15** declares `Latitude: float` and serves 270 clusters +with **0** populated, no raise, because that wave's `cluster_features` is a +single-file extraction. + +### §11.4 `merge_how:` prose overstated what `left` prevents — CORRECTED + +Three places (CLAUDE.md, the skill, the `merge_how` comment in `country.py`) +claimed the `outer` orphans *"collapse together into one phantom null-keyed +row"*. Re-measured on Ethiopia 2013-14 under the current config, two isolated +cold processes differing only in whether `merge_how: left` is present: + +| | wave rows | null-`v` rows | delivered clusters | ΣLatitude | `District` dtype | +|---|---:|---:|---:|---:|---| +| `outer` | 5,287 | 25 | 433 | 4070.3702 | float64 | +| `left` | 5,262 | 0 | 433 | 4070.3702 | int8 | + +The orphans do not survive to the delivered table: the cluster-grain collapse +**deletes** them, because `groupby` drops null keys. `DataFrame.equals` is +`False` between the two, but every *value* is identical — the only difference is +the widened dtype. So `merge_how: left` is not a data fix; it is worth declaring +because it stops manufacturing null-keyed rows for site 1 to delete and stops +the merge widening an integer column, and its cost is a lost diagnostic (under +`outer` the site-1 report *told* you 25 geo households had no cover page). + +**One refinement the review did not make, found while re-deriving it.** The +"phantom 434th EA" story is not false — it is about a *different config*. Before +#628, Ethiopia's `df_geo` declared `v: ea_id2`, so the geo file's orphan EA +arrived as a real, non-null 434th cluster. Since #628 `df_geo` is keyed on `i` +only, so orphans have a **null** `v` instead. `Ethiopia/_/CONTENTS.org` (~L531) +and `Ethiopia/2013-14/_/data_info.yml`'s `merge_how` comment still tell the +old-config story next to the new config. Not edited here (those files belong to +#628, and this PR touches no country config); flagged on the PR thread. + +### §11.5 "The hard error is cache-state-dependent" — DISPUTED as stated + +The finding's premise was: *"`lsms_library/country.py` is not part of the v0.8.0 +content hash, so a warm L2-country parquet is served without ever entering +`grab_data`."* **The premise is false, and it is measurable.** `Wave.grab_data` +carries `@build_transform()`, and `Wave._input_hash` folds +`btf=build_transforms_fingerprint(table)` — which walks the tagged closures' +ASTs — into every wave hash, which `Country._table_cache_hash` composes. +Measured, holding the config tree fixed and varying only the core: + +``` +origin/development btf(cluster_features) = 16e4e8c6c84e6a55b0... +this branch btf(cluster_features) = 9c475fa1150055b86c... +``` + +and mutating *only* `_merge_subframes`'s body moves it again +(`7438a585…`). Nigeria's real warm parquet at +`~/.local/share/lsms_library/Nigeria/var/cluster_features.parquet` grades +**`stale`** under this branch's core. So landing the guard does invalidate; +users are not silently served the pre-guard table on the ordinary path. + +**Two narrower bypasses are real** and the conclusion (verify cold) survives: + +1. A **hashless** pre-v0.8.0 parquet grades `legacy` → trust-once **and + re-stamp**, so it is served unguarded once and then looks `fresh` forever. +2. `assume_cache_fresh=True` skips the check outright. + +Recorded in CLAUDE.md on that narrower, correct basis. + +### §11.6 Merge order — DISCHARGED, see §10 + +The review's one HIGH finding. It named #625 as the last open leg; #625 merged +(`194b55d0`). All three countries re-verified cold above. + +### §11.7 Not accepted as a defect: "the fixtures might convert exceptions into skips" + + +Checked, since it would make the end-to-end tests vacuous. It does not happen +here. `tests/test_gh323_site4_dfs_merge.py` builds a synthetic one-wave country +in `tmp_path` from CSVs it writes itself — no microdata, no S3, no DVC, no +`requires_s3` marker and no `pytest.skip` anywhere in the module. Every one of +the 28 tests executes on every run, and two of them are proved discriminating by +reverting the change (§11.1, §11.2). + +## §12 The census, RE-MEASURED on the merged tree (2026-07-22) + +The §6 census was taken at `45aee170` and is a historical record. This branch is +now merged with `origin/development` (`4c236d11`), which carries #625 (Nigeria), +#628 + #644 (Ethiopia, Niger) and #641 (Mali). Re-swept from scratch, in an +isolated `LSMS_DATA_DIR` (wiped, only `dvc-cache` symlinked), with this +branch's core asserted on import: + +``` +DECLARED blocks: 80 | country/wave dirs: 48 | countries: 20 +EXERCISED merges: 90 # > 80: Nigeria's wave_folder_map replays one + # wave dir under two round labels +CARTESIAN cells: 3 +``` + +| cell | merged rows | phantom | owner | +|---|---:|---:|---| +| Malawi 2010-11 / `cluster_features` | 196,083 | 183,812 | **#653, open** | +| Malawi 2019-20 / `cluster_features` | 185,842 | 171,230 | **#653, open** | +| Guinea-Bissau 2018-19 / `cluster_features` | 5,410 | 59 | **#653, open** | + +Those three phantom counts are **identical to the ones §6 recorded**, measured +by a different sweep on a different day — the census reproduces. + +Cleared since §6: Mali 2021-22 (4,324,668 — #641 deleted the merge outright, +the cover page carried the geography), Ethiopia 2013-14 + 2015-16 (60,221 + +52,832 — #628's re-key to `household_id2`), Nigeria 2012Q3 + 2013Q1 (57,476 +each — #625's `v` re-key). **4,552,673 of 4,907,774 phantom rows gone, 92.8%; +355,101 remain, all three in one open PR.** + +Only failures in the sweep: Nepal 1995-96 and 2003-04 `sample`, +`PathMissingError` — no data in the repository, pre-existing and unrelated. +**0 required-column raises anywhere in the library.** + +### §12.1 Full suite on the merged branch (2026-07-22) + +`pytest tests/ -q -p no:randomly`, warm cache, this branch's core asserted: + +``` +2 failed, 3935 passed, 142 skipped, 9 xfailed, 1 xpassed (31m48s) +``` + +Both failures are +`test_table_structure::{TestTableStructure::test_declared_columns_present, +TestFeatureSanity::test_feature_is_sane}[CotedIvoire/cluster_features]`, and +both reproduce on a clean `origin/development` worktree with **none** of this +PR's code. **Zero new failures.** (§9's third historical failure, +`test_currency::test_feature_ghana_per_wave`, no longer fails on either.) + +Per §9 this is necessary and not sufficient: the suite still cannot see a +build-path guard, because `test_table_structure` never asks for a build. The +evidence that matters for this PR is the 90-merge cold census in §12, not this +line. diff --git a/.coder/ledger/323-tanzania-key.md b/.coder/ledger/323-tanzania-key.md new file mode 100644 index 000000000..427770356 --- /dev/null +++ b/.coder/ledger/323-tanzania-key.md @@ -0,0 +1,208 @@ +# Prior-Art Ledger — GH #323 (Tanzania, Site 2): the `cluster_features` cluster key + +**Search tier used:** ripgrep + git, plus +`slurm_logs/DESIGN_grain_collapse_sites_2026-07-13.org` (branch +`origin/docs/323-grain-collapse-sites`), the Uganda template +(`origin/fix/323-uganda-config` + `.coder/ledger/323-uganda.md`), and the +already-landed Tanzania config commit `3a387bca`. + +**Every measurement below is a COLD build** against an **isolated +`LSMS_DATA_DIR`** whose `dvc-cache` is symlinked to the shared L1 blob cache, so +L2 is rebuilt honestly while raw blobs are reused. `LSMS_NO_CACHE=1` alone is +NOT sufficient — it is *soft* for script-path L2-wave parquets, and Tanzania +`2008-15/cluster_features` is exactly such a script. Config tree = +`LSMS_COUNTRIES_ROOT=/lsms_library/countries`, asserted before any +number was trusted. + +## §1 Task, restated + +Tanzania is the worst Site-2 cell in the corpus: **2,104 of 7,167 +cluster-attribute cells contested (29.4%)**, 65.4% of them in 2012-13. Fix the +**identifier**, not the symptom. CONFIG-ONLY: nothing under `lsms_library/*.py`; +no `aggregation:` keys (D1); no reducer-by-declaration and no blanking-to-NA as +*the* fix. + +## §2 Existing machinery (this task's area) + +| symbol | path | what it does | reuse / extend / new | +|--------|------|--------------|----------------------| +| `_collapse_to_cluster_grain` | `country.py:4490` | Site 2's audit-then-`.first()` | **do not touch** (core, PR #617) — but it is the *instrument*: monkeypatched to measure | +| `reduce_to_agreed` / `collapse_to_cluster_grain` | `build_transforms.py:422/514` | lossless-or-loud grain reducer | **considered, NOT used** — see §6.1 | +| `Wave.column_mapping` → `final_mapping['df_edit']` | `country.py:802` | dispatches a function named after the table as that table's frame hook | **reuse** — the sanctioned YAML-path extension point (2019-20 / 2020-21) | +| `Tanzania/2008-15/_/cluster_features.py` | config | script-path extraction for the 4-round folder | **extend** | +| commit `3a387bca` | config | named the grain (`j`→`i`), `''`→`pd.NA` | **build on** — it made the collapse *visible*; it did not fix the key | +| `origin/fix/323-tanzania` | branch | the REJECTED Design-A patch (+167 lines of `country.py`) | **do not salvage the code**; the diagnosis is already in `3a387bca` | +| `Uganda/_/uganda.py` composite `v` (PR #634) | config | `DISTRICT/PARISH` composite key | **pattern considered, does not apply** — see §6.2 | + +## §3 Definitions & conventions in force + +- `sample()` is the single source of truth for a household's cluster; `v` is + joined at API time by `_join_v_from_sample` — `CLAUDE.md` §"`sample()` and + Cluster Identity". ⇒ **`sample.v` and `cluster_features.v` must be the same + key.** Verified per wave (§7). +- `cluster_features` owns `v`; index `(t, v)` — `Tanzania/_/data_scheme.yml`. +- "NO AGGREGATION IN CORE" — `SkunkWorks/grain_aggregation_policy.org`, D1 of + the consolidation note. +- Multi-round folder semantics: `.claude/skills/multi-round-waves.md` — the + `2008-15/` script emits all four rounds with `t` in the index; paths use + `wave_folder`, not `year`. +- NPS panel design: `.claude/skills/tanzania-panel-design.md` — the 2014-15 + extended/refresh split, and **that the NPS TRACKS movers**, which is the whole + root cause here. + +## §4 Invariants & assumptions (the landmines) + +- **The bug hides behind the cache it poisoned.** Site 2 writes the L2 parquet + *post*-collapse. Every number here is from an isolated data dir (§ header). +- **The geocode scheme is a property of the WAVE, not of the string's length.** + 2019-20's 8-character `'11014002'` is `01-1-014-002` = DODOMA under its own + 9-digit scheme, and IRINGA under the 8-digit one. `cluster_region()` therefore + *requires* the caller to name the scheme; pinned by test. +- **Residency must be evaluated per `(t, v)`, not per `v`.** A cluster can have + residents in one round and none in the next. The first cut grouped on `v` and + silently lost **12 cluster-waves**; caught by a test that pins per-round + cluster counts. +- **Stata writes an unpopulated STRING variable as `''`, not as missing.** The + first cut used `.notna()` on `sdd_cluster` and dropped 63 rows instead of 389. + Caught only because the split-off exclusion produced no cluster-count change. +- `format_id` is auto-applied to `idxvars`, not `myvars` — `CLAUDE.md`. + +## §5 Reuse decision + +| quantity | decision | reason | +|----------|----------|--------| +| what a cluster's Region/District/Rural ARE | **new, but no new primitive**: a residency FILTER in config | the columns are household attributes in a tracking panel; the fix is to stop asking non-residents, not to reduce their answers | +| the region/district crosswalk | **new**, checked in as `TZ_REGION_BY_CODE` in `tanzania.py` | mined from the 2008-09 frame, cross-checked against 2019-20 `t0_region` and 2020-21 `hh_a01_1`; a runtime mine would be silently re-derived on every build and is untestable | +| the household→cluster projection | **leave in core** (`Wave.cluster_features`) | see §6.1 | +| 2020-21 `v` | **re-key to `y5_cluster`** | `sample` already uses it; `clusterid` matched nothing and is NaN for 11.6% of rows | +| 2019-20 geography | **re-source to `t0_region`/`t0_district`** | `hh_a01_1` is the MOVER question — null for 89.9% of the households that can describe the cluster | + +## §6 Decisions, with the evidence + +1. **The config does NOT collapse to `(t, v)` itself** — no `reduce_to_agreed`, + no `collapse_to_cluster_grain` hook. Collapsing in the config would drive the + #323 instrument to 0 *for free* and blind the framework's Site-2 audit to + whatever remains. The frame is left at household grain and only the + *non-resident rows* are removed, so the before/after numbers are measured + with the same instrument and the residue is still announced by + `GrainCollapseWarning`. A test pins that `i` survives into the projection. + +2. **A composite key (the Uganda fix) is the WRONG fix here, and the data say + so.** Uganda's `v` was ambiguous: a parish code unique only within a + district. Tanzania's is not. In round 1 — the only round in which nobody has + moved — **all 409 clusters agree on all three attributes, 0 contested**, and + the cluster id's own region field maps 1:1 onto the region name (26/26 codes) + with its district field equal to the reported district code for **100.0%** of + rows. Making `v` a `(region, cluster)` composite *would* have zeroed the + Region column, but tautologically: it merely re-labels the mover's row with + the mover's region and calls the disagreement gone. Measured, and rejected on + that basis: + + | 2012-13 key | clusters | Region | District | Rural | + |---|---|---|---|---| + | `clusterid` | 409 | 242 | 314 | 247 | + | `(region, clusterid)` | 821 | **0** | 253 | 178 | + | `(region, district, clusterid)` | 1,165 | **0** | **0** | 142 | + + The composite inflates 409 real clusters into 1,165 by splitting each one + across the regions its movers went to. That is not a finer key; it is a + fabricated one, and it would desynchronise `v` from `sample`. + +3. **`strataid2` looked like the perfect cluster-invariant source and was + rejected on measurement.** It is `" - RURAL|URBAN"` and shows *zero* + within-cluster variation in all four rounds — because it is **100% NULL in + rounds 1-3**. The "invariance" was an artefact of missingness. Exactly the + trap #323 exists to catch; recorded so nobody re-proposes it. + +4. **Cluster attributes are NOT taken from the cluster's frame round and carried + forward.** Tried: per `v`, use the round the cluster first appears in. Round 1 + (409 clusters) and 2014-15 (419 new) come out at 0 contested, but the 82 + clusters that first appear in 2012-13 come out at 172 — they are pre-2012 + clusters *renumbered* after the Simiyu/Geita/Njombe/Katavi region splits, so + their "frame round" is not a frame round at all. Residency generalises + correctly where first-appearance does not. + +5. **`urban/rural` is not in the geocode.** Tested: the EA-code leading digit + predicts `urb_rur` for only 75.3% of round-1 clusters. So `Rural` stays a + reported attribute, and its 264 residual contested cells are real + sub-cluster variation, not decodable away. + +6. **2019-20 split-off rows are dropped, and that removes 100 clusters.** Their + `clusterid` is not a sampling cluster: under a single DODOMA code they pool + households whose own strata span DAR ES SALAAM, DODOMA and ZANZIBAR, and of + the 269 (of 326) whose parent household IS in the frame, only **31** carry + the parent's cluster. Emitting cluster rows + built from them is fabrication (the parent's "18 fabricated rows"). The honest + consequence — those 326 households' `sample.v` now points at a cluster the + table does not contain — is recorded in §9 rather than papered over. + +7. **Malformed ids are carried through, not repaired.** Three 2020-21 + `y5_cluster` values have the wrong field widths and one carries region code + `27`. Guessing the intended value is unfalsifiable; the tests pin the count so + it cannot grow. + +## §7 Measured effect (cold build, isolated `LSMS_DATA_DIR`) + +Contested cluster-attribute cells (`nunique(dropna=True) > 1` per column, on the +pre-collapse frame — the parent's instrument, reproduced to ±2): + +| wave | before | after | clusters | +|------|--------|-------|----------| +| 2008-09 | 0 | 0 | 409 → 409 | +| 2010-11 | 329 | **78** | 409 → 409 | +| 2012-13 | 803 | **147** | 409 → 409 | +| 2014-15 | 160 | **41** | 498 → 498 | +| 2019-20 | 257 | **72** | 247 → **147** | +| 2020-21 | 557 | **47** | 418 → **515** | +| **total** | **2,106** | **385** | 2,389 → 2,387 rows | + +By column: Region **706 → 3**, District 758 → 118, Rural 642 → 264. + +Rows destroyed by the collapse (framework's own Site-2 audit): **10,982 → +2,687**; NaN-key deletions **545 → 0**. + +`sample.v` ≡ `cluster_features.v`, `(t, v)` pairs in `cluster_features` unknown +to `sample`: + +| wave | before | after | +|------|--------|-------| +| 2008-09 / 2010-11 / 2012-13 / 2019-20 | 0 | 0 | +| **2020-21** | **417 of 417** | **0** | +| 2014-15 | 5 | 5 (pre-existing, §9) | + +## §8 Verification + +- `tests/test_gh323_tanzania_cluster_key.py` — 28 tests, all config-level; + exercises no core aggregation. +- **Negative control**: run against a pristine `origin/development` config tree + (`git archive` into a separate `LSMS_COUNTRIES_ROOT`, separate data dir), + **20 of 28 fail** — including every contested-cell ceiling, the + `sample.v` ≡ `cluster_features.v` test, both 2020-21 key tests and the + per-wave cluster counts. +- `tests/test_tanzania_grain_gh323.py` (19) and + `tests/test_tanzania_community_cluster_xwalk.py` pass. +- `tests/test_schema_consistency.py` + `tests/test_gh323_benin_togo.py` pass + (243 passed, 2 skipped in that slice). +- Pre-existing failure, NOT caused by this branch and NOT fixed by it (core is + out of scope): + `tests/test_gh323_explicit_reducers.py::test_core_does_not_dispatch_the_reducers` + — PR #617's private `country._collapse_to_cluster_grain` contains PR #618's + banned substring. Same failure is recorded in `.coder/ledger/323-uganda.md` §8. + +## §9 Deferred + +- **2019-20 `sample.v` for the 326 split-off households.** Their `clusterid` is + not a sampling cluster (§6.6) and now dangles. Blanking it changes `v` on every + 2019-20 household table for 27.5% of the wave — a semantic change that belongs + in its own PR, per the same reasoning D2 used to keep Burkina Faso's + `dropna=False` out of a bug fix. +- **5 of 498 2014-15 clusters unknown to `sample()`.** Present in the wave-level + `sample.parquet` (13/7/10/10/10 rows), lost between there and `sample()` — + i.e. in `_finalize_result`'s `id_walk`, on the panel-id side. Pre-existing on + `development`; unchanged here. +- **A `harmonize_district` table.** Most of the 118 residual District cells are + two spellings of one district (`MBINGA (NYASA)` / `MBINGA(NYASA)` / + `MBINGA NYASA` / `NYASA`) or a mid-panel district split (Chalinze/Bagamoyo, + Kibiti/Rufiji, Ubungo/Kinondoni). Label harmonisation, not a key defect. +- **A `Rural` cluster classification.** 264 residual cells are EAs whose + households are genuinely not all urban or all rural. diff --git a/.coder/ledger/323-uganda-crop-condition.md b/.coder/ledger/323-uganda-crop-condition.md new file mode 100644 index 000000000..f2ba2cbf0 --- /dev/null +++ b/.coder/ledger/323-uganda-crop-condition.md @@ -0,0 +1,292 @@ +# Prior-Art Ledger — #323 / #637: `condition` index level on Uganda `crop_production` + +**Search tier used:** ripgrep + git floor (gitnexus not exercised; no MCP index write attempted). + +## §1 Task, restated + +Uganda's `crop_production` is a script-path (`materialize: make`) table built by +`lsms_library/countries/Uganda/_/uganda.py::crop_production_for_wave` from the UNPS +post-harvest modules AGSEC5A (season A) / AGSEC5B (season B). Its declared index in +`Uganda/_/data_scheme.yml` is `(t, i, plot, j, u, season)`. The UNPS harvest question +6 records, per plot-crop-season, *how much was harvested **and in what condition*** +(green / fresh / dry-at-harvest / dried, crossed with the physical form: on the stalk, +with shell/cob, in the cob, in pods, as grain). The condition is not in the declared +index, so two harvest records that differ **only** by condition — e.g. 240 kg dry +coffee and 100 kg fresh coffee off the same plot — land on the same index tuple and are +summed by the de-duplication block at the end of `crop_production_for_wave`. Dry weight +is added to fresh weight. This ledger covers adding `condition` as an index level, +sourced per vintage, on a shared vocabulary. + +## §2 Existing machinery (this task's area) + +| symbol | path:line | what it does | tested? | reuse / extend / new | +|--------|-----------|--------------|---------|----------------------| +| `crop_production_for_wave` | `Uganda/_/uganda.py:922` | builds one wave's canonical frame from AGSEC5A/5B + AGSEC4A; already loops over a per-season `conditions:` list of column-sets | via wave-script asserts | **extend** — the loop variable is already named `cond`; it just never emitted the condition label | +| `CROP_COLMAPS` | `Uganda/_/uganda.py:1099` | per-wave, per-season source column map | no | **extend** — add a `condition:` key per condition dict | +| `_harmonized_codes(table)` | `Uganda/_/uganda.py:517` | reads a `Code \| Preferred Label` org table from `Uganda/_/categorical_mapping.org` into `{int: str}` | no | **reuse** — same call shape as `_harvest_unit_map()` | +| `harvest_units` org table | `Uganda/_/categorical_mapping.org:779` | the harvest **unit** code scheme (`1=Kg`, `10=Sack (100 kgs)`, …) | no | **model to copy** for a sibling `harvest_conditions` table | +| `_enforce_canonical_spellings` | `lsms_library/country.py:3948` | rewrites variant→canonical values; **line 3962 handles INDEX levels** via `df.rename(index=…, level=col)` | `tests/test_schema_consistency.py` reads the same YAML | **reuse** — this is what makes a `Columns:` declaration reach an index level | +| `_load_canonical_spellings` | `lsms_library/country.py:3921` | inverts `Columns...spellings` into `{variant: canonical}` | — | reuse (read-only) | +| `Columns.plot_features.Tenure` | `lsms_library/data_info.yml:159` | the house model for a canonical **vocabulary declaration**: `type: str`, a prose `note:`, and `spellings:` with *empty* variant lists | — | **model to copy** | +| `_normalize_dataframe_index` | `lsms_library/country.py:4099` | **drops any index level not in `data_scheme.yml`'s `index:`**, then collapses duplicates | — | constraint: the `index:` line MUST be updated or the level is silently dropped at API time | +| `harvest_kg` | `lsms_library/transformations.py:1132` | `groupby(['t','i',plot,'j']).sum()` over whatever levels are present | — | **unaffected** — name-based groupby, and it sums, so splitting rows leaves the total identical | +| `_canonical_index_levels` / modal-shape exclusion | `lsms_library/feature.py:58`, `:487` | `Feature()` drops per-country frames whose `tuple(index.names)` differs from the modal shape | — | constraint: `crop_production` is **not** in `index_info`; Uganda is already modal-excluded | +| `Country._/crop_production.py` | `Uganda/_/crop_production.py` | concatenates wave parquets + `id_walk`; index-agnostic | — | no change needed (docstring only) | + +## §3 Definitions & conventions in force + +- **Canonical schema single source of truth**: `lsms_library/data_info.yml`; + `tests/test_schema_consistency.py` reads it — never hardcode schema rules in tests + (`CLAUDE.md`, "Canonical Schema"). +- **`spellings` is an inverse dict** (canonical → accepted variants); + "The canonical values are simply `spellings.keys()`" — `lsms_library/data_info.yml:71-76`. +- **Index-level value *enumerations* are not schema-able**: "Acquisition source `s` is an + INDEX level, not a column. … Enforced in code (`lsms_library.transformations.S_VALUES`); + data_info.yml has no schema for index-level value enumerations." + — `lsms_library/data_info.yml:247-250`. See §5 for how this and `_enforce_canonical_spellings` + line 3962 are both true. +- **Countries extend, they do not force-fit**: "Countries may extend this list rather than + force-fit a local category" — `lsms_library/data_info.yml:170` (`Tenure`), and the same + stance for `TenureSystem` at `:188`. +- **`crop_production` is deliberately NOT registered in `index_info`**: "the plot-level ag + features (plot_labor/crop_production/plot_inputs) are NOT here: their per-country index + NAMES diverge (plot vs plot_id, crop vs j) and need harmonizing before registration -- + tracked separately." — `lsms_library/data_info.yml:32-34`. +- **Never write an unevidenced "no module here" claim** — `CLAUDE.md`, "Adjudicating + `absent` cells". Applied here to the `a5aq6b`/`a5aq6c` inversion comment: it was + checked per wave against Stata metadata rather than trusted. +- **`sane` is not `blessed`** — `CLAUDE.md`, "Coverage Matrix". No cell is blessed by + this PR; no human has read the per-condition numbers in analysis. + +## §4 Invariants & assumptions + +- **The declared `index:` in `data_scheme.yml` is load-bearing.** `_normalize_dataframe_index` + (`country.py:4160`) drops undeclared index levels and then collapses duplicates. Adding + `condition` to the parquet without adding it to `index:` would be a silent no-op that + still sums fresh onto dry. +- **The de-dup collapse uses `groupby(level=…)` with pandas' default `dropna=True`** + (`uganda.py:1084`), so any row with `pd.NA` in an index level is **silently dropped**. + This already loses 431 rows / 7.1 M native units in 2009-10 (NaN `plot`) — see §6. + Consequence for this task: `condition` must carry a **sentinel**, never `pd.NA`, exactly + as `u` already does (`df['u'] = … .where(df['u'].notna(), 'Unknown')`, `uganda.py:1078`). +- **`_harmonized_codes` returns `pd.NA` for a code whose `Preferred Label` is blank or for + a code absent from the table** (`uganda.py:534`, plus the `.get(c, pd.NA)` at the call + site). So off-scheme codes must be sentinel-filled by the caller. +- **`uganda.py` resolves `categorical_mapping.org` relative to CWD** (`../../_/`), i.e. it + assumes the wave-script working directory. Any harness must `chdir` into a wave `_/`. +- **`u`, `plot`, `j` and now `condition` are all *object* index levels**; the collapse's + `groupby` is on level names, so level ORDER in the tuple is not load-bearing for it — + but it is for anyone slicing positionally. +- **`t` is a string wave label**; `season` is `'A'`/`'B'`. + +## §5 Reuse decision + +| quantity | decision | reason | +|----------|----------|--------| +| condition code → label decode | **reuse** `_harmonized_codes` + a new `harvest_conditions` org table | identical shape to `harvest_units`; one table serves every wave because the integer code scheme is bit-identical across vintages (see `CONTENTS.org`) | +| per-vintage source column | **extend** `CROP_COLMAPS` with a `condition:` key per condition dict | the `conditions:` list already exists for the wide 2019-20 slots | +| canonical cross-country vocabulary | **extend** `data_info.yml` `Columns:` with a `crop_production.condition` entry in the `Tenure` style | `_enforce_canonical_spellings` (`country.py:3962`) *does* reach index levels — verified in code and by an executed unit test, not assumed | +| index-level value **enforcement** (reject an off-vocabulary value) | **deferred** — needs an `S_VALUES`-style constant in `transformations.py`, which is core and out of scope | see §6 | +| `index_info` registration of `crop_production` | **not done** | blocked on cross-country level-NAME harmonisation (`plot` vs `plot_id`, `crop` vs `j`) across 14 countries; `data_info.yml:32-34` says so explicitly. Uganda is already modal-excluded from `Feature('crop_production')` today, before and after this change. | +| reducer / `aggregation:` YAML | **rejected** | #323 D1: a new index level or nothing; `aggregation:` is dead config | + +## §6 Open questions for the human + +- **Enforcement follow-up (core).** `spellings:` *declares* the vocabulary and rewrites + known variants, but nothing rejects a value outside it. A follow-up core PR should add + either (a) a `CONDITION_VALUES` constant in `transformations.py` alongside `S_VALUES`, or + (b) a generic index-level enumeration check driven from `data_info.yml` — which would + also let `s` stop being special-cased. (b) is the better shape; (a) is the precedent. +- **`crop_production` in `index_info`.** Requires renaming `crop`→`j` (10 countries) and + `plot`→`plot_id` (or the reverse) before registration, and a decision on whether Uganda's + `season` and `condition` become canonical levels or get fabricated as NaN for the other + 13. Separate PR; sized in the report. +- **Pre-existing NaN-key row loss in the collapse** (431 rows / 7.1 M units in 2009-10, + 43 rows / 962.5 in 2011-12). Rows whose `plot` id is `pd.NA` vanish in + `groupby(level=…).sum()`. Not caused by, and not fixed by, this PR — it is a separate + defect and is unchanged by it. +- **2009-10 sentinel quantities.** 2009-10's `Quantity` sums to 3.18e8 native units, + ~300× every other wave, because `99999` is used as a missing sentinel and never stripped. + Separate defect. +- **Second condition slot dropped for 2019-20 season B and 2018-19 season B.** + `agsec5b.dta` carries `s5bq06a_2/b_2/c_2` labelled "(2019, condition2)" with 1 306 + non-null conditions; `CROP_COLMAPS['2019-20']['B']` reads only slot 1, so those harvest + records are absent from the table entirely. Deliberately NOT added here: it *adds mass*, + which would destroy the "totals unchanged" invariant this PR is verified against. + Separate issue. + +--- +### Measured outcome (cold, isolated `LSMS_DATA_DIR`, 2026-07-21) + +| wave | dup groups before | after | rows before | rows after | Quantity before = after | +|---|---:|---:|---:|---:|---:| +| 2009-10 | 670 | 181 | 24 997 | 25 485 | 310,664,565.8 | +| 2010-11 | 608 | 81 | 20 442 | 20 970 | 708,149.5 | +| 2011-12 | 653 | 55 | 19 552 | 20 152 | 1,128,560.3 | +| 2013-14 | 589 | 61 | 17 053 | 17 585 | 1,098,713.5 | +| 2015-16 | 764 | 63 | 18 874 | 19 576 | 1,050,367.4 | +| 2018-19 | 0 | 0 | 14 194 | 14 194 | 1,180,031.7 | +| 2019-20 | 370 | 18 | 15 369 | 15 721 | 1,175,600.8 | +| **total** | **3 654** | **459** | **130 481** | **133 683** | **317,005,989.0** | + +`Quantity_sold` (18,134,251.4) and `Value_sold` (9,066,365,499.8) totals are +likewise identical before and after. The 130 481 "before" figure is independently +corroborated by the pre-change `Feature('crop_production')` run. + +`Feature('crop_production')` is byte-identical before and after: 250 692 rows, the +same 8 kept countries, the same modal index, and the same 6 excluded frames +(`Ethiopia, EthiopiaRHS, Mali, Nigeria, Tanzania, Uganda`) — Uganda was already +excluded. + +Negative control: on the pre-fix tree `tests/test_uganda_crop_condition.py` gives +6 failed / 3 errors / 1 passed, including `expected the dry (240) and fresh (100) +coffee records to stay separate, got quantities [340.0]`. Post-fix, from a +physically cleared Uganda cache: 10 passed. + +### Phase 3 — verification + +- `crop_production_for_wave` (condition decode + sentinel) — **OK (anchored on §4)**: uses + `_harmonized_codes` per §5, sentinel-fills per the `u` precedent at `uganda.py:1078`, so + no new `pd.NA` index keys and no new silent row loss. +- `harvest_conditions` org table — **OK (anchored on §2)**: same `Code | Preferred Label` + shape as `harvest_units`; codes are the 20 that carry Stata value labels, nothing invented. +- `data_info.yml Columns.crop_production.condition` — **OK (anchored on §3)**: `Tenure` + shape, empty variant lists, prose `note:` carrying the "countries may extend" stance. +- `data_scheme.yml index:` — **OK (anchored on §4)**: required, else `_normalize_dataframe_index` + drops the level. +- No `aggregation:` key added; no reducer; no edit under `lsms_library/*.py` except the + config file `data_info.yml`. `transformations.py` untouched. + +--- + +## Phase 4 — post-review corrections (PR #649 adversarial review, 2026-07-21) + +Verdict was APPROVE-WITH-NOTES: the data path survived every attack, three +comments were inaccurate, one test gap was real. All measurements below were +re-derived independently (isolated `LSMS_DATA_DIR` with only `dvc-cache` +symlinked in, `LSMS_COUNTRIES_ROOT` pinned to the worktree and asserted, +Uganda cache physically removed, no `dvc` CLI) — not copied from the review. + +### F1 (MEDIUM) — code 99's justification was false by ~100x + +Old claim: "only 3 rows in the whole panel and none after 2011-12". +Measured: **381 raw source rows** carry code 99 with a reported measure and +**340 built rows** carry `other_condition`; **219 of the 340 are after +2011-12**. Per wave (built): 1 / 0 / 120 / 32 / 32 / 32 / 123. + +Vintage semantics confirmed from Stata metadata: 99 = `Others` (2011-12), +`Not Applicable` (2018-19, 2019-20), and **absent from the label set** in +2013-14 / 2015-16 (both ship 19 codes, no 99) though 32 rows per wave use it. + +**Behaviour unchanged.** The merge is kept and the *real* justification is +now written down: `t` is an index level so no wave-crossing tuple exists and +nothing is summed; 99 is the scheme's own residual slot in every vintage; and +splitting would need a third value for 2013-14/2015-16 where no wave says +what 99 means. The alternative (split into `other_condition` / +`not_applicable_condition`) is recorded as a follow-up that moves 340 rows +and needs its own before/after. + +### F2 (LOW) — the sentinel rate, not the off-scheme rate + +`data_scheme.yml` said "~1%". Measured sentinel share: **7 994 / 133 683 = +6.0% panel-wide, 23.2% in 2009-10**. "~1%" is right only for *off-scheme +codes*: 297 = 1.1% of 2009-10, 192 = 0.9% of 2010-11, 492 panel-wide. +`CONTENTS.org` already had the correct figures, so this was the summary +drifting from the analysis; the summary now agrees. + +### F3 (LOW) — `Source Label` is not verbatim + +Of the 216 (code, wave, season) label pairs in the five labelled waves, +**60 match the org column literally, 156 do not**. It is the 2018-20 wording +with whitespace collapsed, except **four codes — 24, 32, 42, 99** — which +take the longer 2009-16 phrasing; 99's text ("Others / Not Applicable") +appears in no wave at all. Also documented: code 45's *Preferred* Label +`dried_grain` is an inference from grid position (every wave writes only +"Dry - grain"), covering 40 411 / 133 683 rows = 30%. + +### F4 (MEDIUM) — a mis-wired colmap must fail loudly + +Two independent guards, because they catch different mistakes. + +1. **`uganda._require` raises `CropColmapError`** when a colmap NAMES a column + the source lacks. `None` stays the declared way to say "no such column". + Measured no-op: exactly one named column failed to resolve panel-wide + (2010-11's intercrop `flag: 'a4aq3'`, a column that does not exist in that + file — fixed to `None`, itself a no-op), and a cold rebuild is + byte-identical afterwards (133 683 rows; Quantity 317,005,989.0; + Quantity_sold 18,134,251.4; Value_sold 9,066,365,499.8; `intercropped` + non-null 75 625 on both sides; sorted frames `.equals()` True). +2. **Three tests**, thresholds set from measurement: + `test_crop_colmap_columns_resolve_in_source` (S3-guarded), + `test_sentinel_share_bounded_per_wave_season` (ceiling 40%; worst honest + cell 25.7%), `test_condition_varies_within_every_wave_season` (floor 10 + distinct; measured minimum 18). + +**Mutation proof** (Uganda cache physically cleared each run): + +| mutation | before | after | +|---|---|---| +| baseline | 10 passed | **13 passed** | +| `condition: 'a5aq6b_TYPO'` (2018-19 A) | 10 passed | **1 failed, 4 passed, 8 errors** | +| `condition: 's5aq06f_1'` (existing wrong column) | 10 passed | **1 failed, 12 passed** | + +The second mutation is the informative one: the build succeeds, the resolve +test passes, and the sentinel share is only **33.7%** — *under* the 40% +ceiling — so the variety test (2 distinct conditions vs a floor of 10) is what +catches it. Neither invariant alone suffices; both are kept. + +Also hardened: `test_fresh_and_dry_no_longer_collide`'s `skip`-on-empty is now +an assert, and the `crop_production` fixture re-raises a build failure when S3 +credentials are present (still skips without them, so the data-free CI job is +unaffected). + +### Found while doing F4, NOT raised by the review + +`crop_production.intercropped` is wired to the **seed-use question** in every +wave that populates it (`a4aq3` / `a4aq16` / `s4aq16`, all labelled "did you +use any seed/seedlings?", {1: Yes, 2: No}), not to the cropping-system +question (`a4aq7` "Cropping system" {1: Pure Stand, 2: Inter cropped}, or +`a4aq8` / `s4aq08` "What type of crop stand was on the plot?" {1: Pure Stand, +2: Mixed Stand}). Agreement between the wired flag and the true crop-stand +question is 48.5–52.5% — a coin flip. Rewiring moves data, so it is filed as +a Known Issue in `Uganda/_/CONTENTS.org`, not fixed here. + +### Prior art missed on the first pass (coordinator correction, 2026-07-21) + +I applied F1–F3 without reading `Uganda/_/CONTENTS.org` end to end — only the +sections PR #649 itself had added. Re-read in full afterwards. One piece of +genuine, pre-existing prior art was missed, and it bears directly on F3. + +**§"Unit handling for `food_acquired`"** (pre-dates the `condition` work; +untouched by `b28b5024`) records the house convention for cross-wave label +drift: the `u` table carries **one column per wave** (`2005-06` … `2019-20`) +holding that wave's raw questionnaire string, "unused at runtime but +preserv[ing] the cross-wave provenance". + +`categorical_mapping.org` therefore holds three shapes for one job: + +| table | shape | per-wave drift | +|---|---|---| +| `u` | Code, Preferred Label, one col per wave | representable | +| `harvest_units` | Code, Preferred Label | not recorded | +| `harvest_conditions` | Code, Preferred Label, `Source Label` | **lossy** | + +This reframes F3. The single `Source Label` column is not merely *where* the +"verbatim" claim went wrong — it is **why** it could: one cell cannot hold two +vintages' wordings, so codes 24/32/42 silently took the 2009-16 text and 99 +became a hand-made composite. The `u` table has no such failure mode; it +preserves each wave's string separately, typos and all (`Small cup wuth +handle(Akendo)`). + +Recommended follow-up (documented, not done): give `harvest_conditions` the +`u` table's shape. Runtime-inert — `get_categorical_mapping` reads only +`Code` + `Preferred Label` — and the 216 measured label pairs above are +exactly the data needed to populate it. Left to the maintainer because the +review scoped F3 to a prose correction. + +No **contradiction** was found: nothing pre-existing in `CONTENTS.org` asserts +anything about code 99's incidence, the sentinel rate, or the unlabelled +2009-10/2010-11 codes, so none of the F1/F2/F3 numbers overwrote a recorded +decision. Checked the LOGBOOK/status entries too — the only one is on +"Missing panel weight for 2019-20" (weights, unrelated); no `WAITING` caveats +anywhere in the file. diff --git a/.coder/ledger/548-panelids.md b/.coder/ledger/548-panelids.md new file mode 100644 index 000000000..4470f0028 --- /dev/null +++ b/.coder/ledger/548-panelids.md @@ -0,0 +1,136 @@ +# Prior-Art Ledger — GH #548 (panel-id collisions in bespoke `panel_ids.py`) + +> Per-task ledger. Inherits `.coder/ledger/STANDING.md` (§0 baseline). + +**Search tier used:** ripgrep + git (floor). gitnexus not consulted (call-graph +here is one script + two `local_tools` functions; grep was sufficient and the +line anchors below were re-grepped, not remembered). + +## §1 Task, restated + +`GhanaLSS/_/panel_ids.py` is a **bespoke country-level script** (`panel_ids: !make` +in `data_scheme.yml:108`) that writes `_/updated_ids.json` + `_/panel_ids.json` +**directly**, instead of routing its `{cur_i: prev_i}` linkage through +`local_tools.panel_ids()` → `local_tools.update_id()` the way the YAML-path +countries (Malawi) do. `PANELC.DAT` is **person-level** and records GLSS1 +households that **split** into two GLSS2 households; the script's +`dict(zip(cur_i, prev_i))` therefore encodes a **many-to-one** map (two live +1988-89 households → one 1987-88 id). `id_walk` renames both onto the same id and +`Country._normalize_dataframe_index`'s `groupby().first()` / additive `sum()` +collapses them — *inside* `load_from_waves`, i.e. **before** the L2-country +parquet is written, so the loss is baked into the cache and warm reads are silent. + +**Not** the mechanism #548 states (it says "rename-onto-occupied"); the targets +`101332` / `114008` are 1987-88 ids and are absent from the live 1988-89 id set, +so #536's `cur_set` guard is a **no-op** here. Verified below. + +## §2 Existing machinery (this task's area) + +| symbol | path:line | what it does | tested? | reuse / extend / new | +|--------|-----------|--------------|---------|----------------------| +| `local_tools.update_id` | `lsms_library/local_tools.py:1731` | inverts `{cur: prev}`, and when N cur-ids share one prev, mints `prev`, `prev_1`, … (`_N` split convention). Carries the **GH #504** guard: never mint a suffix equal to a live target or an already-emitted id. | `tests/test_id_walk.py`, `tests/test_update_id*.py` | **REUSE** — this is the guard GhanaLSS bypasses | +| `local_tools.panel_ids` | `lsms_library/local_tools.py:1788` | wave-sequenced driver: builds the `RecursiveDict` chain from the **pre-suffix** prev-ids, then calls `update_id` per wave | via country builds | **REUSE** (Burkina uses the DataFrame form with a dummy prior-wave row) | +| `local_tools._close_id_map` / `id_walk` | `local_tools.py:1843` / `1899` | closure-resolve + apply the per-wave rename; idempotent since #547 | `tests/test_id_walk.py` | **DO NOT TOUCH** (blast radius = every panel country) | +| `Country._normalize_dataframe_index` | `country.py:~4158` | `groupby(idx).first()` / additive `sum()` — the *silent* collapse that turns a duplicate `(i,t)` into a wrong number | — | leave; it is the amplifier, not the cause | +| `Mali/_/panel_ids.py:122` (`cur_set` from the **full** cover) | — | the **#536** guard: skip a rename whose target is a *live current-wave* id | PR #546 | **EXTEND** to GhanaLSS as defense-in-depth (0 hits today) and to Burkina (latent) | +| `Burkina_Faso/_/panel_ids.py:65` | — | same guard but built from **panel-only** candidates (the *unfixed* #536 pattern) | — | **HARDEN** (byte-identical today) | +| `diagnostics._check_panel_ids_targets_exist` | `lsms_library/diagnostics.py:1372` | asserts a chain entry's two endpoints canonicalise to the **same** id | run via `check_panel_consistency` | **EXTEND** — false-fails on *every* split household (Malawi: 1410/5686 = 25% today) | + +## §3 Definitions & conventions in force + +- **Split-household id convention** = `prev_id`, `prev_id_1`, `prev_id_2`, … — + minted by `update_id` (`local_tools.py:1731-1785`, docstring example). Malawi + IHPS ships 896/996/1726 such ids across its three panel waves. GhanaLSS must + use the same convention, not a bespoke one. +- **`panel_ids` (the `RecursiveDict`) stores PRE-suffix prev-ids** for the wave + being processed: `local_tools.panel_ids:1838` does `recursive_D.update(...)` + *before* `update_id` is applied at line 1839. Semantically right: `101332_1` is + a **1988-89** canonical id; no such household exists in 1987-88. `updated_ids` + (the rename map) carries the suffix; `panel_ids` (the lineage chain) does not. +- **`updated_ids` = per-wave `{raw_cur_id: post-walk canonical id}`**, consumed by + `id_walk` (`local_tools.py:1899`) inside `_finalize_result`. +- Cache: `.py`/`.json` under a country `_/` are in `_BUILD_INPUT_SUFFIXES` + (`country.py:448`) and feed `_table_cache_hash` (`country.py:~2285`) — editing + `panel_ids.py` / `updated_ids.json` busts the L2-country hash for **every** + GhanaLSS table, so the rebuild is automatic. Per `CLAUDE.md` §Cache Behavior. + +## §4 Invariants & assumptions + +- `STANDING.md §4` (all of it), plus: +- **`Country.updated_ids` is a `JSON_CACHE_METHOD`** (`country.py:75`) and + `load_json_cache` prefers `data_root()/{C}/_/updated_ids.json` over the in-tree + file (`country.py:2508-2511`) — and JSON caches carry **no content hash**. If + such a copy exists it **shadows** the regenerated map and this fix is silently + inert. Verified absent on this machine (`~/.local/share/lsms_library/GhanaLSS/_/` + does not exist); re-check before believing any AFTER number. +- The rename map must stay **injective** after the fix (no two live cur-ids → + one canonical id), and no minted `_N` id may equal a live 1988-89 cover id. + Both are now **asserted in the script** — a class-1 failure is converted to a + class-3 crash. +- GhanaLSS panel linkage exists **only** GLSS1↔GLSS2; the other five waves ship + empty maps (`panel_ids.py` docstring). Anything that changes a non-1988-89 + wave is a bug in the fix. + +## §5 Reuse decision + +| quantity | decision | reason | +|----------|----------|--------| +| split-id minting (`_N`) | **reuse** `local_tools.update_id` | the tested guard (#504) the bespoke script bypasses; hand-rolling a suffixer is the reinvention this ledger exists to prevent | +| chain `RecursiveDict` + per-wave rename map | **reuse** `local_tools.panel_ids` (DataFrame form + dummy prior-wave row, the Burkina pattern, `Burkina_Faso/_/panel_ids.py:40-46`) | one call gives both artifacts with the framework's own suffix/chain semantics | +| rename-onto-occupied guard | **extend** the #536 full-cover `cur_set` (`Mali/_/panel_ids.py:122`) | 0 hits today for GhanaLSS; keeps the two complementary guards *both* present so the class is closed, not half-closed | +| split-tolerant chain check | **extend** `diagnostics._check_panel_ids_targets_exist` | it false-fails on the framework's own `_N` convention (Malawi 25%); without this the fix would *look* like a regression | +| `id_walk` / `_close_id_map` / `update_id` internals | **new: NO** | DANGER ZONE — patching them puts 13 panel countries in the blast radius for a defect in one bespoke script | + +## §6 Open questions for the human + +- **Niger's 59 duplicate `(i,t)` tuples are NOT this bug** (pre-walk, from + `EXTENSION ∈ {0,1,2}` in `ECVMA2_MS00P1.dta` while the wave declares + `i: [GRAPPE, MENAGE]`). Needs a separate issue: *household identity omits + EXTENSION*. Not fixed here. +- Which split-off keeps the base id (`101332`) vs. `101332_1` is decided by + `PANELC.DAT` row order (first-seen wins) — the same rule `update_id` applies + everywhere. If Ghana's survey documentation designates a "continuing" + household, that would be a better rule; no such variable is in `PANELC.DAT`. + +--- +### Phase 3 — verification + +- `GhanaLSS/_/panel_ids.py` — **OK (anchored on §2/§5)**: now calls + `local_tools.panel_ids()` (hence `update_id`); no hand-rolled suffixer, no + touch to `id_walk`. `panel_ids.json` is byte-identical (§3: chain stores + pre-suffix ids); `updated_ids.json` changes exactly 2/714 entries + (`204922: 101332 -> 101332_1`, `255718: 114008 -> 114008_1`). +- `GhanaLSS` full-cover `cur_set` guard — **OK (anchored on §5)**: reuses #536's + formula (`prev in cur_set and cur != prev`) over `Y00A.DAT` (the 1988-89 + cover, 3194 households — the same file the wave's `sample` is built from); + 0 skipped today → byte-stable. Plus two post-conditions that convert a + future class-1 defect into a class-3 crash: the rename map must stay + injective, and no canonical id may collide with a live 1988-89 id. +- `Burkina_Faso/_/panel_ids.py` — **OK (anchored on §2)**: `cur_set` now built + from the full 2021-22 cover; both JSONs byte-identical (md5-verified), 95 + skipped chains unchanged. +- `diagnostics._check_panel_ids_targets_exist` — **OK (anchored on §3)**: accepts + `canon_ci == f'{canon_pi}_{N}'` because that *is* the library's split + convention. Malawi `panel_ids_targets_exist`: **fail (1410/5686 = 25% + "inconsistent") -> pass** — 1410 false positives removed, and those entries + now actually get their endpoint-existence check (0 missing). Burkina's + pre-existing `1088/3132 prev` failure is **byte-identical** on pristine + diagnostics (not caused, not masked). + +**Measured (cold rebuild, isolated data root, BEFORE = pristine `development`):** + +| table | before | after | note | +|---|---|---|---| +| sample | 56,328 | 56,330 | +2 | +| interview_date | 56,338 | 56,340 | +2 | +| household_roster | 246,584 | 246,594 | +10 persons | +| individual_education | 159,921 | 159,924 | +3 (rest of the recovered rows are all-NaN and dropped downstream) | +| food_acquired | 5,259,309 | 5,259,344 | +35 un-summed tuples | +| food_expenditures / food_prices / food_quantities | — | — | +29 / +35 / +35 | +| housing, plot_features, cluster_features, food_security | — | — | **0** (byte-identical) | + +Every row **not** in the two split lineages is byte-identical, including all of +1987-88. GH #323 collapse warnings on a cold build: sample 2->0, +interview_date 2->0, household_roster 10->0, individual_education 8->0. +`food_acquired` (i=101332, t=1988-89) `Expenditure`: **15,850 -> 5,570**, with +the split-off household appearing as `101332_1` with 10,280. diff --git a/.coder/ledger/602-spellings.md b/.coder/ledger/602-spellings.md new file mode 100644 index 000000000..e568176d2 --- /dev/null +++ b/.coder/ledger/602-spellings.md @@ -0,0 +1,112 @@ +# Ledger — GH #602: declared spellings are never enforced + +**Search tier used:** ripgrep + git (floor), plus a live-API cross-country sweep +(34 countries x every spellings-constrained `(table, column)`). gitnexus not used. +**Inherits:** `.coder/ledger/STANDING.md` §2/§3/§4 — cited, not restated. + +--- + +## §1 Task + +`lsms_library/data_info.yml` declares accepted values via `spellings` blocks, but +nothing **enforces** them. `_enforce_canonical_spellings()` maps *known variants* +and never *rejects unknown ones*. Uganda's `sample.Rural` was the literal string +`'0'` for 2,263 households (72% of the 2005-06 wave), so `df[df.Rural=='Rural']` +silently returned **zero rows** — and `is_this_feature_sane(...).ok` was `True`. + +Deliverable was the **cross-country sweep**; the check is how you do it. + +## §2 Existing machinery reused (did NOT rebuild) + +| symbol | path | why I reused it | +|---|---|---| +| `_enforce_canonical_spellings` | `country.py:3931` (STANDING §2) | the variant→canonical map. My check *normalizes through it* before testing membership, so it is correct on pre-finalize parquets **and** post-finalize API frames. | +| `_load_api_derived()` | `diagnostics.py:40` | exact prior-art pattern for reading `data_info.yml` `Columns` in diagnostics (module-level constant, same `(OSError, YAMLError)` handling). `_load_declared_vocabularies()` mirrors it. | +| `Check` / `SanityReport` / `is_this_feature_sane` | `diagnostics.py` | the check is one more `Check` in the existing battery — no new reporting machinery. | +| `spellings` block in `data_info.yml` | STANDING §3 | the *sanctioned* place to declare accepted variants. Mali's `Urbain` / Guinea-Bissau's `Urbano` were fixed by **extending the existing block**, not by writing per-country mapping code. | +| `categorical_mapping.org` auto-discovery | CLAUDE.md | GhanaLSS's `Semi-urban` was a *mislabelled row in an existing mapping table*, fixed there — not by adding a new transform. | + +**Did NOT reinvent:** a value-validation layer. `_check_value_constraints` +(`diagnostics.py:368`) already existed — but it reads the *country's* +`data_scheme.yml`, only acts on **list** declarations, and returns `warn`. Every +country declares `Rural: str` (a scalar), so it is skipped entirely. I did not +extend it (its `warn` status is exactly the status quo that let this rot); I added +a sibling that reads the **canonical** `data_info.yml` and returns `fail`. + +## §3 Definitions in force (cited) + +- Canonical schema = `lsms_library/data_info.yml` (STANDING §3). Tests read it; + **never hardcode schema rules** — so the check reads `Columns` at runtime. +- The vocabulary for a column is `spellings.keys()`, per the file's own header + comment ("The canonical values are simply spellings.keys()"). +- `Rural` semantics: cluster/HH urban-rural indicator, canonical `Rural`/`Urban`. + +## §4 Invariants I had to respect (the landmines) + +1. **Key on `(table, column)`, never column name alone.** `housing.Tenure` exists + in ~12 countries with a legitimately different vocabulary (dwelling tenure) and + `housing` declares none. A name-keyed check emits ~12 false failures. Pinned by + `test_housing_tenure_is_not_flagged`. +2. **Do not reuse `country._load_canonical_spellings()`.** Its final `if + variant_map:` guard **drops** every column whose variant lists are all empty — + `Affinity`, `Tenure`, `TenureSystem`. Reusing it would silently leave those + three unchecked. Hence a separate loader. Pinned by + `test_empty_variant_vocabularies_are_still_checked`. +3. **Normalize before membership.** Cached parquets are pre-`_finalize_result` + (STANDING §4) and legitimately hold *known variants* (Malawi's `rural`/`RURAL`). + `tests/test_table_structure.py` reads parquets directly, so a check that tests + membership without first applying the variant map fails Malawi/sample and every + other country whose parquet holds a variant. Pinned by + `test_passes_on_declared_variants`. +4. **`Rural` under `sample:` must NOT be `required: true`.** `required` is read by + `feature.py:316`; many countries' `sample` has no urban/rural column at all. +5. **Shared cache + concurrent agents.** `~/.local/share/lsms_library` is shared + (CLAUDE.md scrum-master addendum 1). Other worktrees were concurrently + rebuilding `sample`/`cluster_features` with *their* config and overwriting my + parquets (fresh mtime, stale content). All verification was redone under an + isolated `LSMS_DATA_DIR`. **A parquet-level result from the shared cache is not + trustworthy while other agents run.** + +## §5 What the issue got wrong (verify the diagnosis before fixing it) + +- **The Uganda proximate cause is a YAML key TYPE mismatch, not a missing mapping.** + Raw `urban` is object-dtype but *mixed*: python `int 0` (2263) + `str 'urban'` + (860). The declared key was the **string** `'0'`, which never matches, so the + value passed through and was stringified to `'0'`. Commit `9959b9f3` + (2026-04-14, "close GH #163 item 3") introduced exactly that string key and was + **silently dead for three months**. The fix is one character: `'0'` → `0`. +- **A second, independent gap the issue does not mention:** `sample` was absent + from `Columns:` entirely, so `_enforce_canonical_spellings` was a *total no-op* + on `sample()` — it did not merely fail to reject unknowns, it never mapped the + *known* variants. That is the Tajikistan bug (9,020 lowercase rows). +- **Uganda is the least severe find, not the only one.** Malawi + `cluster_features.Rural` was **sign-flipped between waves**; India `sample.Rural` + was **100% fabricated**. + +## §6 Instrument validation (a negative result from an unvalidated check is a +result about the check) + +`scratchpad/plant.py`: the check FIRES on planted violations in **columns and +index levels** (`'0'`, `'0.0'`, `'Urbain'`, trailing-space `'Urban '`, bad `Sex`), +is SILENT on clean frames, on known variants, on all-NaN columns, and on the +`housing.Tenure` trap. It also *sees* `Tenure`/`TenureSystem`/`Affinity`, which +the pre-existing loader drops. All 12 cases pass. + +## §7 Residual / deliberately not fixed + +Five `plot_features` `Tenure`/`TenureSystem` pairs (Albania, Cambodia, Kosovo, +Timor-Leste, Tajikistan) ship raw unharmonized survey labels. The check correctly +**fails** them; they are enumerated in `tests/test_declared_spellings.KNOWN_UNHARMONIZED` +and xfailed, **not** downgraded to `warn`. Mapping them needs the questionnaire +codebooks (Cambodia's "GIVEN BY THE GOVERNMENT OR LOCAL AUTHORITY"; Timor-Leste's +"Part owner", 81% of plots), and guessing would replace a *visibly* wrong value +with an *invisibly* wrong one — the exact class-1 failure this issue is about. + +Same reasoning for **India**: `stratum` provably carries no urban/rural information +(perfectly collinear with `state`: UP vs Bihar), so `Rural` is **deleted**, not +invented. Silently-missing (class 2) beats silently-wrong (class 1). + +**Pre-existing caveat surfaced, not introduced:** Malawi 2004-05 `ea` is not a +clean cluster key — 19 of 110 EAs contain both rural and urban households, so the +cluster-level aggregate picks one. My change only relabels the code as a word; it +does not alter which household is picked. diff --git a/.coder/ledger/645-to-parquet-null-coercion.md b/.coder/ledger/645-to-parquet-null-coercion.md new file mode 100644 index 000000000..6f7599dd7 --- /dev/null +++ b/.coder/ledger/645-to-parquet-null-coercion.md @@ -0,0 +1,168 @@ +# Prior-Art Ledger — GH #645: `to_parquet` destroys values that spell like nulls + +**Search tier used:** ripgrep + git (floor), plus a **live cross-country cold +sweep**: 28 data-bearing countries × `individual_education`, each in its own +process with its own empty `LSMS_DATA_DIR` (only `dvc-cache` symlinked to the +shared L1), plus targeted before/after cold builds of `Nigeria/food_acquired` +and `Guyana`/`South Africa` `housing`. gitnexus not used. +**Inherits:** `.coder/ledger/STANDING.md` §2/§3/§4 — cited, not restated. +**Base commit:** rebased onto `4c236d11`. The branch started at `45aee170`; the +`origin/development` ref moved **twice** mid-task (`eadafe75` India #611, then +`4c236d11` through #644) — see §6, and note that #644 changes Ethiopia's +baseline numbers, so every Ethiopia figure here was **re-measured** on +`4c236d11`. + +--- + +## §1 Task, restated + +`local_tools.to_parquet` serializes every `dtype == object` column as +`astype(str).astype('string[pyarrow]').replace({'nan': None, 'None': None, +'': None})`. It stringifies **first** and then tries to recover the nulls +by matching the resulting characters — but after `astype(str)` a genuine +missing and a legitimate value spelling `'None'` are the same characters, so +the recovery is wrong in principle, not merely in practice. + +`None` was the canonical library label for *"no education"* +(`categorical_mapping/harmonize_education.org`), so the write path nulled every +never-schooled person, and `Country._finalize_result`'s `dropna(how='all')` +(`country.py`) then **deleted the row** — `individual_education` has exactly one +column. Fix the write path, rename the hostile label, force one cache rebuild. + +## §2 Existing machinery (reused, did NOT rebuild) + +| symbol | path | what it does | tested? | decision | +|---|---|---|---|---| +| `to_parquet` | `local_tools.py` (STANDING §2) | the only sanctioned writer | yes | **extend in place** — 3-line change inside the existing coercion loop; no new writer, no new call site, no signature change | +| `LSMS_CACHE_SCHEMA` | `local_tools.py` | the manual library-version lever folded into `Wave._input_hash` / `Country._table_cache_hash` | cache tests | **reuse** — `4 -> 5`. This is exactly the lever's documented purpose (a library-wide change to the *pre-write* extraction logic whose per-table input hashes are unchanged); no new invalidation mechanism | +| `_enforce_canonical_spellings` | `country.py` (STANDING §2) | variant → canonical at API time, on columns AND index levels | `test_schema_consistency.py` | **reuse** — the `None -> No education` back-compat mapping is a `spellings` entry, not migration code. Historical caches resolve forward with no rebuild | +| `spellings` block in `data_info.yml` | STANDING §3 | the sanctioned place to declare accepted values | `test_declared_spellings.py` | **reuse** — followed the `plot_features.Tenure` / `TenureSystem` pattern (full vocabulary, empty variant lists) | +| `harmonize_education` org tables | `categorical_mapping/` + 25 country `categorical_mapping.org` | raw label → canonical level | via country builds | **edit data, not code** — the rename is 38 table rows; no mapping code changed | +| `tests/conftest.py` `requires_s3` | added on `development` in #648 | skips data-dependent tests in the data-free CI job | — | **reuse** — the three cold integration cases carry it | + +**Did NOT reinvent:** +- *A migration script for existing caches.* `LSMS_CACHE_SCHEMA` already forces + exactly one rebuild everywhere; a bespoke rewriter would be a second, + untested invalidation path. +- *A null-sentinel scrubber on the read path.* `transformations.py` + `_na_strings`, `country.py` `_NA_SENTINELS` and `conversion.py` already do + targeted, deliberate mop-ups on `Relationship` / `sex` / `age` / dates. Those + are *correct* (they operate on columns whose vocabulary genuinely excludes + these strings) and are left alone — see §4.3. Adding a general one would + re-create this bug one layer up. +- *A reducer or an `aggregation:` key.* GH #323 D1 forbids it; nothing here + needed one (`test_gh323_explicit_reducers.py` and + `test_gh323_grain_contract.py` both green, 280 assertions). + +## §3 Definitions & conventions in force (cited) + +- **Canonical schema** = `lsms_library/data_info.yml` (STANDING §3). The + education vocabulary is now declared there, so + `diagnostics._check_declared_vocabularies` enforces it. +- **Canonical education ladder** = `categorical_mapping/canonical_education_labels.org` + — 14 ordered levels + the non-ordinal `Unknown` sentinel. Level 0 renamed + `None` → **`No education`**. +- **Cache tiers / staleness** = `CLAUDE.md` §"Cache Behavior (v0.7.0+)" and the + `LSMS_CACHE_SCHEMA` comment block in `local_tools.py`. +- **`sane` is not `blessed`** = `CLAUDE.md` §"Coverage Matrix". Directly + load-bearing here: `Guatemala,individual_education,2000,sane,declared,20678` + certified a table missing 30% of its rows. +- **GH #323 D1 — no aggregation in core** = `SkunkWorks/grain_aggregation_policy.org` §3a. + +## §4 Invariants & assumptions (the landmines) + +1. **The pyarrow guard is still load-bearing.** Measured in this venv + (pandas 3.0.2 / pyarrow 23.0.1, `future.infer_string=True`): a genuinely + mixed object column still raises `ArrowTypeError` — `str`+`int`, `str`+`float`, + `str`+`Timestamp`, `str`+`list`/`tuple`/`dict`, `bool`+`str`, `str`+`Decimal` + all fail. The block may NOT be deleted. Pinned by + `test_mixed_type_object_column_is_still_stringified`. +2. **But it fires arbitrarily.** Under pandas 3.0 a *pure string* column is + inferred as `str`, not `object`, so it skips the block entirely. Only + whatever still lands as `object` (Stata reads: 16 of 32 columns in Ethiopia + 2011-12 `sect2_hh_w1.dta`) is affected. That is why exactly 3 of 25 + at-risk countries were hit — the other 22 were correct *by accident of dtype + inference*, not by design. **Narrowing the block to "genuinely mixed" + columns was considered and rejected** — see §5. +3. **`LSMS_NO_CACHE=1` MASKS this bug.** It skips the L2-wave write where the + rebinding coercion happens, so it returns the *right* answer and hides the + defect. Every measurement here used a genuinely fresh `LSMS_DATA_DIR`; the + integration tests do the same, in a subprocess, and say so. +4. **The two `to_parquet` call sites disagree.** `country.py` `Wave.grab_data` + **rebinds** the return value; the country-level writer **discards** it. That + is the whole mechanism of the cold≠warm signature, and why Guatemala (whose + loss is entirely at the wave level) is cold == warm == wrong and therefore + invisible to any A/B comparison. Left as-is — with a value-preserving write + the difference is unobservable, and `cold == warm` is now a *tested* + invariant rather than an accident (`test_cold_and_warm_builds_agree`). +5. **Declaring a `spellings` vocabulary is an assertion that FAILS a country.** + `diagnostics._check_declared_vocabularies` returns `Check(..., "fail")` for + any value outside `spellings.keys()`. Declaring only `No education` would + have turned every other education level into a violation in 24 countries. + Verified empirically instead of assumed — §5. +6. **`groupby(dropna=True)` deletes NaN index keys** (GH #323 §3b, open). + Relevant because the pre-fix code was nulling an *index level* in Nigeria + `food_acquired` (`u`). Measured: no rows were actually lost there — but the + combination is a live hazard, and one fewer NaN key is a strict improvement. + +## §5 Reuse decision & the two judgement calls + +| quantity | decision | reason | +|---|---|---| +| null-preserving write | **extend `to_parquet`** | capture `isna()` before the cast, `.mask(na)` after. Smallest change that makes the recovery well-posed | +| cache invalidation | **reuse `LSMS_CACHE_SCHEMA`** | documented lever, exact fit | +| back-compat for the old label | **reuse `spellings`** | applies at API time to columns *and* index levels; no rebuild needed | +| keep vs. narrow the object-dtype block | **KEEP** | narrowing changes stored dtypes for homogeneous non-str object columns (an object column of ints would be stored `int64` instead of the strings `'1'`,`'2'`), i.e. it would silently change returned data across the corpus — the exact class of failure this issue is about. The premise ("pyarrow can't take mixed object") is **still true**; what has changed is only that fewer columns reach it. Recorded, not acted on | +| declaring the full education vocabulary | **DO IT, on evidence** | swept 28 data-bearing countries cold at the API level, then audited **107 freshly cold-built parquets** (wave + country level) with the real `_check_declared_spellings`: **zero** off-vocabulary values after the one leak below. Cross-checked statically against all 20 `harmonize_education` tables (`test_harmonize_education_targets_stay_inside_the_declared_vocabulary`) | +| Ethiopia 2013-14 `76.0` | **fix at the wave `df_edit` hook**, not in the org table | the leak (1 row of 12,583, a bare float that never got a Stata value label) CANNOT be fixed in `categorical_mapping.org`: the table's `Original Label` column is mixed text, so `df_from_orgfile` reads every key as a **string** and a float source value can never match one — verified empirically after an org-row attempt failed (contrast Guyana, whose all-numeric table parses keys as floats). The hook reuses the shape of Uganda's tested `_/_education_helpers.py` and reads the vocabulary from `data_info.yml` (via the already-tested `diagnostics._DECLARED_VOCABULARIES`) rather than re-listing it. `Unknown` is the vocabulary's own definition for "unmappable", so this is not a guess. Zero effect on the API output (Ethiopia 59,092 before and after) | + +## §6 Notes for the human + +- `origin/development` advanced twice **during** this task (`45aee170` → + `eadafe75` India #611 → `4c236d11` through #644). A + `git checkout origin/development -- ` for the negative-control revert + therefore pulled in unrelated India work; caught and reverted by pinning the + base SHA. Worth knowing: in a shared-object-store worktree, `origin/*` refs + are not stable within a session — **pin the SHA for any A/B measurement.** +- **#644 interacts with this fix on Ethiopia, benignly but visibly.** It + re-keys 2013-14 `individual_education` onto the wave-native ids, which stops + 5,247 people collapsing onto one blank tuple. A side effect is that the + stray unlabelled `76.0` code **now survives to the API frame**, where before + it was collapsed away — so the `df_edit` hook added here is doing real work + on the current base, not just tidying a wave parquet. All Ethiopia numbers + in this ledger, the tests and the PR are measured on `4c236d11`. +- **`Nigeria/*/_/food_acquired.py` writes `fillna('None')` into the `u` index + level** (4 waves, 1,151 rows) — the same hostile-sentinel pattern, one table + over. Pre-fix those became NaN index keys; post-fix they are the literal + `'None'`. Not renamed here (out of scope, and `u` feeds the unit-conversion + tables), but it should be renamed to something like `Not recorded`. + +--- + +### Phase 3 — verification (anchored on this ledger) + +- `to_parquet` (`local_tools.py`) — **OK (§2, §4.1, §5)**: extended in place, no + new writer; the pyarrow guard retained and pinned by a test that asserts raw + `df.to_parquet` still raises on a mixed column. +- `LSMS_CACHE_SCHEMA = 5` — **OK (§2, §5)**: the documented lever, with a + comment stating the reason (content change, unchanged input hashes). +- `Columns.individual_education.Educational Attainment.spellings` — **OK (§3, + §4.5)**: full vocabulary, `Tenure`-pattern, verified against a 28-country cold + sweep before being declared. Not a REINVENTION of `_check_value_constraints` + (which reads the *country's* `data_scheme.yml` and only acts on list + declarations) — it is data for the existing canonical checker. +- 38 `harmonize_education` rows + `canonical_education_labels.org` — **OK + (§3)**: data edits onto the vocabulary of record; no mapping code touched. +- `tests/test_gh645_to_parquet_null_coercion.py` — **OK (§2)**: uses + `requires_s3` from `tests/conftest.py` (#648) rather than a private + creds helper; cold isolation per §4.3 rather than `LSMS_NO_CACHE`. +- `Ethiopia/2013-14/_/mapping.py::individual_education` — **OK (§2, §5)**: a + wave `df_edit` hook, the mechanism Uganda already uses for exactly this + ("unlabelled float junk code"); reads the vocabulary from the schema of record + instead of hardcoding a second copy of it. +- `country.py` `dropna(how='all')` log line — **OK (§1)**: reports, changes + nothing. Anchored on the fact that this step is where the value corruption + became a deletion; deliberately INFO, and the escalation policy is left to the + maintainer rather than invented here. +- **No CONTRADICTION found** with GH #323 D1: no `aggregation:` key, no reducer; + `test_gh323_explicit_reducers.py` + `test_gh323_grain_contract.py` green. diff --git a/.coder/ledger/STANDING.md b/.coder/ledger/STANDING.md index 5b787b786..d4ce51a2f 100644 --- a/.coder/ledger/STANDING.md +++ b/.coder/ledger/STANDING.md @@ -63,8 +63,11 @@ The authoritative sources. Quote these; do not restate schema rules from memory. - **IO is sanctioned-only.** Read with `get_dataframe`, write with `to_parquet`. Never `pd.read_stata`, `pyreadstat`, raw `dvc.api.open`, or absolute paths. - Never `dvc pull`/`dvc fetch` from the CLI (global lock; fails under concurrency). - See `CLAUDE.md` §"Data Access" anti-pattern table. + **Never invoke the `dvc` CLI at all** — not `pull`/`fetch`, not `add`/`push` + (global lock; 91.7% failure at 12 concurrent). Reads go through + `get_dataframe` / `get_data_file` (lock-free direct-S3); writes through + `push_to_cache_batch` (lock + backoff-with-jitter retry). Never `rm` a DVC + lock file. See `CLAUDE.md` §"Data Access" anti-pattern table. - **Do NOT bake `v` into feature parquets**; write `(t, i, …)` and let the framework join. Do NOT add `v` to feature `data_scheme.yml` indexes except `cluster_features`. diff --git a/.coder/ledger/roster-characteristics-wave-deletion.md b/.coder/ledger/roster-characteristics-wave-deletion.md new file mode 100644 index 000000000..af2d4c5c8 --- /dev/null +++ b/.coder/ledger/roster-characteristics-wave-deletion.md @@ -0,0 +1,129 @@ +# Prior-Art Ledger — roster-characteristics-wave-deletion + +> Per-task ledger. Inherits the repo §0 baseline in `STANDING.md`. Living +> snapshot; git history is the journal. + +**Search tier used:** ripgrep + git floor (gitnexus not consulted this session), +plus an empirical census: `Country(c).household_characteristics()` per-wave row +counts for all 40 countries that declare `household_roster`, warm cache. + +## §1 Task, restated + +`household_characteristics` is a **derived** table (`Country._ROSTER_DERIVED`, +`STANDING.md §2`): `country.py` aggregates `household_roster` across *all* waves +into one frame and hands it to `transformations.roster_to_characteristics()`, +which drops non-resident members using whichever residence-duration column is +present (`MonthsSpent` / `MonthsAway` / `WeeksAway`, per `CLAUDE.md` +§"MonthsSpent / MonthsAway / WeeksAway"). Because the frame handed to the +transform is the **country-level concat**, a residence column contributed by +*one* wave is unioned onto *every* wave. Where the resolved months series is +unusable for a wave, the keep-mask is all-False **for that whole wave** and +`household_characteristics` silently returns nothing for it, even though +`household_roster` is fully populated. 8 wave-cells are currently destroyed. +The task fixes the resolution + the guard, and repairs one wave whose +`MonthsSpent` is an untranslated French label — in **config**, not in code. + +## §2 Existing machinery (this task's area) + +| symbol | path:line | what it does | tested? | reuse / extend / new | +|--------|-----------|--------------|---------|----------------------| +| `roster_to_characteristics` | `lsms_library/transformations.py:177` | roster → household sex×age counts + `log HSize`; owns the residence filter (L251-265) | `tests/test_roster_to_characteristics_movers.py`, `tests/test_age_intervals.py`, `tests/test_uganda_api_vs_replication.py` | **extend** (the residence-resolution block only) | +| `Country._ROSTER_DERIVED` + dispatch | `lsms_library/country.py:3325`, `3515-3530` | resolves the transform by *name* at call time and passes the **all-waves** roster with `final_index` = the roster's own index levels | integration surface | reuse unchanged | +| YAML `mapping:` on a `myvars` entry | e.g. `Mali/2017-18/_/data_info.yml:76`, `Mali/2018-19/_/data_info.yml:163` (`MonthsSpent: [s1q13, {mapping: {Oui: 12, Non: 0}}]`) | maps a categorical source label to the canonical numeric months value **at config level** | per-country builds | **reuse** — this is the sanctioned pattern for D3 | +| `mover_sentinel` / NaN-in-`final_index` handling | `transformations.py:287-337` | GH #197 / #268: keeps movers rather than silently dropping them | `tests/test_roster_to_characteristics_movers.py` | untouched (same *class* of silent-drop bug, one layer down) | + +Searched by concept — "months present", "residence", "drop departed", "keep +mask" — not just by identifier. No other implementation of a residence filter +exists in the library; `roster_to_characteristics` is the only site. + +## §3 Definitions & conventions in force + +- **MonthsSpent / MonthsAway / WeeksAway semantics** — `CLAUDE.md` + §"MonthsSpent / MonthsAway / WeeksAway" (cited, not restated): `MonthsSpent` + = months present 0–12; `MonthsAway` → `12 - value`; `WeeksAway` → + `12 - weeks/(52/12)`. Filter excludes NaN (question not asked) and 0 months, + **except** infants (age < 1). **"Countries without any residence column are + unaffected — the old count-everyone behavior continues."** That sentence is + the definition the D2 guard restores at *wave* granularity. +- **Ethiopia W4–W5 switched months→weeks** — same section: "`WeeksAway`: … + Used by Ethiopia W4–W5 and Cambodia, where the questionnaire switched from + months to weeks." So Ethiopia legitimately carries **both** columns at country + level, disjointly populated. The `elif` chain contradicts this documented fact. +- **EHCVM binary → 12/0** — same section: "The binary `s01q12` … is mapped to + 0/12", done in the wave `data_info.yml` `mapping:` block. D3's fix follows + this pattern verbatim (Burkina Faso 2014 EMC `B3A`, a 6-month binary). +- **Derived tables are not registered** — `STANDING.md §3`; the fix must stay in + `transformations.py` + config, with no `data_scheme.yml` change. + +## §4 Invariants & assumptions + +- **The 1315-HH Uganda drift is the reason the filter exists** (`CLAUDE.md`, + same section). The all-NaN fallback is *exactly* the old count-everyone + behaviour, so it must fire **only** where the wave has no usable residence + datum at all — never for a wave where the filter currently works. Enforced by + the per-`t` (not global) guard + the before/after census. +- The roster handed to the transform is a **multi-wave** frame with `t` in the + index (`country.py:3523` derives `final_index` from `['t','v','i','m']`). + Any per-wave logic must group on `t` — and must not assume `t` is present + (the transform is also called directly in tests with a `(i, pid)` index). +- Residence columns arrive with mixed dtypes across waves (BF: `'6 mois ou + plus'`, `'12'`, `12.0`) — the country concat unions object + float. Resolution + must stay `pd.to_numeric(..., errors='coerce')`-based. +- `STANDING.md §4` repo-wide invariants unchanged: no `inplace=`, `pd.NA` for + string/categorical missing, config resolved via `countries_root()`. + +## §5 Reuse decision + +| quantity | decision | reason | +|----------|----------|--------| +| months-present series | **extend** `roster_to_characteristics` | only implementation; the bug is in its resolution step (per-column `elif` → per-row coalesce) | +| per-wave "no usable residence data" guard | **new** (inside the same function) | nothing in the library expresses "this column is unioned-in from another wave"; it is a property of the country-level concat, invisible at wave level. Kept ~10 lines, local to the transform. | +| Burkina Faso 2014 French labels → 12/0 | **reuse** the YAML `mapping:` pattern (EHCVM `Oui`/`Non`) | `CLAUDE.md` prescribes it; keeps language-specific labels out of `transformations.py` | + +## §6 Open questions for the human + +- **RESOLVED (Ethan, 2026-07-12): BF 2014 `B3A` stays `12 / 0`.** The decision + is subtle enough that someone will try to "fix" it later, so it is recorded + both here and in a comment above the mapping in + `Burkina_Faso/2014/_/data_info.yml`: + - `B3A`'s *variable label* is a **duration** question ("durant combien de mois + [NOM] a vécu dans le ménage"), but the **released variable is collapsed to a + 6-month binary**: `{1: '6 mois ou plus', 2: 'moins de 6 mois', 9: 'Valeur + manquante'}` — 77,268 / 943 / (332 NaN). + - So `'moins de 6 mois'` does **not** mean "lived zero months". Those 943 + people (1.2%) *did* live in the household, for 1–5 months. Read literally, + mapping them to `0` is false. + - This is exactly where `B3A` **differs from the EHCVM precedent it borrows**: + `s01q12` ("lived continuously 6+ months? Oui/Non") is a **membership test**, + so `Non → 0` is faithful there; `B3A` is a **binned duration**. + - We keep `12/0` anyway because the `0` encodes **"not a de-facto household + member"** (the standard LSMS 6-month rule), not a claim about months lived — + and consistency with the EHCVM family wins. The 943 are dropped by the + filter, as the EHCVM `Non` members are. + - `12/6` was rejected partly because **6 is the one value the respondent + explicitly ruled out**. If we ever choose to keep these members, the honest + fill is **3** (the 1–5 month interval midpoint), not 6. +- CotedIvoire 1985–89 and Mali 2021-22 now count **everyone** in the roster + (no residence question in those waves). That is the documented behaviour for + a country with no residence column, applied per-wave. It does mean those + waves are not filter-comparable with their EHCVM siblings. + +--- +### Phase 3 — verification (fill at task end) + +- `roster_to_characteristics` (residence-resolution block) — **OK (anchored on + §3)**: per-row coalesce `MonthsSpent → MonthsAway → WeeksAway` matches the + documented per-column semantics and is the only reading consistent with + "Ethiopia W4–W5 switched … to weeks" while W1–W3 use months. +- `roster_to_characteristics` (per-`t` all-NaN guard) — **OK (anchored on §3/§4)**: + restores "countries without any residence column are unaffected — the old + count-everyone behavior continues" at wave granularity; the census shows it + fires for exactly the 7 wave-cells with 0 non-null residence data and for no + other wave (Uganda's 8 waves byte-identical). +- Burkina Faso 2014 `MonthsSpent` mapping — **OK (anchored on §3, reuse §2)**: + same config-level `mapping:` idiom as the EHCVM `Oui`/`Non` → 12/0 waves; no + French in library code. +- No **REINVENTION**: no other residence/months-present filter exists in the + library (ripgrep over `months`, `spent`, `away`, `resident`). +- No **CONTRADICTION**: the filter still fires wherever it fires today; the only + waves whose counts move are the 8 that currently return nothing. diff --git a/CLAUDE.md b/CLAUDE.md index 4a4248140..7ce265f38 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -34,9 +34,30 @@ The returned DataFrame prepends a `country` index level. - `scrum-master-hpc` (shared sucoder skill at `~/.sucoder/skills/scrum-master-hpc/SKILL.md`) — dispatching subagents, worktrees, DVC lock hygiene. Read this before using the Agent tool for multi-country work. Library-specific addenda: 1. Subagents share the parquet cache at `~/.local/share/lsms_library/`, so concurrent agents building different countries don't conflict. 2. The venv is at `{repo_root}/.venv/bin/python` (not in worktrees) — set `PYTHONPATH` to the worktree so development-branch code is picked up. - 3. **`.venv/lib/python3.11/site-packages/lsms_library.pth` hardcodes the main-repo path** — `PYTHONPATH` alone does NOT redirect imports of `lsms_library` to a worktree. Worker agents that rebuild a feature to verify a YAML edit will silently run the main checkout's code. For **config** edits (`countries/{C}/_/...` — YAML/scripts/`.org`, the common case), set **`LSMS_COUNTRIES_ROOT=/lsms_library/countries`** so the live package reads the worktree's config tree (GH #436); this is the clean fix and needs no fresh venv. For **library-code** edits (`country.py`, `local_tools.py`, …) the `.pth` still pins to the main checkout, so either (a) verify via static diff only, or (b) have the agent install a fresh venv inside its worktree. See the `.pth`-pinned package imports pitfall in the scrum-master-hpc skill for detection and mitigations. + 3. **`.venv/lib/python3.11/site-packages/lsms_library.pth` hardcodes the main-repo path**, so a worktree agent can silently verify against the *main checkout's* code. **Always set `PYTHONPATH=`** — it *does* beat the `.pth` (corrected 2026-07-12; this entry previously claimed the opposite, and the wrong claim sent people to a needless fresh-venv build). + + **The actual trap is `python