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/lsms_library/countries/Mali/2021-22/_/data_info.yml b/lsms_library/countries/Mali/2021-22/_/data_info.yml index 1a0cd0f8d..89ed7f765 100644 --- a/lsms_library/countries/Mali/2021-22/_/data_info.yml +++ b/lsms_library/countries/Mali/2021-22/_/data_info.yml @@ -111,28 +111,45 @@ sample: - t cluster_features: - dfs: - - df_main - - df_geo - df_main: - file: ehcvm_conso_mli2021.dta - idxvars: - v: grappe - myvars: - Region: region - Rural: milieu - df_geo: - file: s00_me_mli2021.dta - idxvars: - v: grappe - myvars: - Latitude: GPS__Latitude - Longitude: GPS__Longitude - merge_on: - - v - final_index: - - t - - v + # GH #323 (site 4) / GH #627. This block USED to be a two-sub-frame + # `dfs:` merge: Region/Rural from `ehcvm_conso_mli2021.dta` and the GPS + # fix from `s00_me_mli2021.dta`, joined on `v`. That was a CARTESIAN + # PRODUCT, not a join, and by a wide margin the worst one in the corpus: + # + # ehcvm_conso is the EHCVM consumption file at (household x food item) + # grain -- 393,480 rows for 6,143 households in 513 grappes. s00 is the + # household COVER PAGE -- 6,143 rows, also finer than `v`. Merging two + # frames on `v` when BOTH repeat `v` pairs every consumption line-item + # with every household in its grappe: 393,480 x 6,143 -> 4,718,148 rows, + # of which 4,324,668 were phantoms that exist in no survey (88% of the + # 4,907,774 phantom rows PR #627's 40-country census found). + # `_normalize_dataframe_index` then mopped it up with groupby().first(), + # so the table looked clean at 513 rows while nearly 4.4M invented rows + # passed through it. + # + # The merge was never needed. s00_me_mli2021.dta carries the geography + # itself: s00q01 == `region` and s00q04 == `milieu`, verified per grappe + # against ehcvm_conso -- 513/513 grappes agree on BOTH, each is constant + # within its grappe, and the two files cover the identical 6,143 + # (grappe, menage) households. So this is a single-file extraction, the + # cartesian is cured at the merge (not laundered by a reducer afterwards), + # and the returned values are bit-for-bit what they were. + # + # Grain note: s00 is still household-grain (12 households per grappe), so + # the declared (t, v) index is collapsed by the framework exactly as + # before -- but now it de-duplicates 12 identical rows per grappe instead + # of ~9,200 (mean 9,197 = 767 line-items x 12 households). Region/Rural + # are provably constant within grappe (checked); + # Latitude/Longitude are the survey's per-grappe displaced fix stamped on + # every household of the grappe. + file: s00_me_mli2021.dta + idxvars: + v: grappe + myvars: + Region: s00q01 + Rural: s00q04 + Latitude: GPS__Latitude + Longitude: GPS__Longitude household_roster: diff --git a/lsms_library/countries/Mali/_/CONTENTS.org b/lsms_library/countries/Mali/_/CONTENTS.org index a78dff9d6..4b8b9116f 100644 --- a/lsms_library/countries/Mali/_/CONTENTS.org +++ b/lsms_library/countries/Mali/_/CONTENTS.org @@ -46,6 +46,89 @@ fix should map the numeric codes to canonical labels via Mali's in each wave's =data_info.yml=); pending that, downstream consumers should treat Mali separately. +* cluster_features (GH #323 site 4 / GH #627) + +** 2021-22 was the worst =dfs:= cartesian in the corpus — fixed + +Until this fix, =2021-22/_/data_info.yml= built =cluster_features= from a +two-sub-frame =dfs:= merge: + +| sub-frame | file | rows | grain | +|-----------+--------------------------+---------+--------------------------| +| df_main | ehcvm_conso_mli2021.dta | 393,480 | household × food item | +| df_geo | s00_me_mli2021.dta | 6,143 | household (cover page) | + +merged =on: [v]= — the *cluster* key. 513 grappes, so each side repeated +every key value (767 consumption line-items and 12 households per grappe). +A join on a key duplicated on *both* sides is a cartesian product by +construction: every line-item paired with every household of its grappe. + +: 393,480 × 6,143 -> 4,718,148 rows, 4,324,668 of them phantoms + +PR #627's 40-country census found 4,907,774 phantom rows in all; +*this one cell was 88% of them.* + +Why nobody noticed: =_normalize_dataframe_index= collapses the declared +=(t, v)= index with =groupby().first()=, which reduced 4.7M rows to 513 — +and the L2-country parquet is written *after* that collapse. The table +looked clean at every point anyone ever looked. The phantoms existed only +in the wave-level frame, upstream of the collapse. (See CLAUDE.md, "Grain +Collapse": /the bug hid behind the cache the bug poisoned/.) + +** The merge was never needed + +=s00_me_mli2021.dta= carries the geography itself. Verified against the +data, not inferred: + +| canonical | cover page | consumption file | agreement | +|-----------+------------+------------------+--------------------| +| Region | =s00q01= | =region= | 513/513 grappes | +| Rural | =s00q04= | =milieu= | 513/513 grappes | +| Latitude | =GPS__Latitude= | (absent) | — | +| Longitude | =GPS__Longitude= | (absent) | — | + +Both are *constant within grappe* in both files, the two files cover the +identical 6,143 =(grappe, menage)= households, and the label sets are the +same 9 regions / 2 milieux. So the block is now a *single-file +extraction* from the cover page. Per GH #323 decision D1 the cartesian is +cured *at the merge*; no reducer, no =aggregation:= key — a reducer on a +cartesian only signs the corpse. + +The result is bit-for-bit unchanged: all 3,006 country-level rows across +all four waves, all four columns, identical before and after (=DataFrame.equals= +=True=). This fix removes 4,324,668 invented rows and alters no value. + +Prior art within this same file: =sample.df_cover= already reads =Rural= +from =s00q04= of this very cover page, and the 2017-18 wave already reads +=Region: s0q01= / =Rural: s0q04= from *its* cover page. + +Pinned by =tests/test_gh323_mali_cartesian.py= (wave-level row counts for +all four waves — a country-level assertion cannot see this bug). + +** The other three waves are not cartesian + +Their =df_geo= is one row per grappe, so the merge is many-to-one: + +| wave | df_main | df_geo | wave rows | grappes | +|---------+--------------------------------+-------------------------------+-----------+---------| +| 2014-15 | eaci2014_agregatconso (3,804) | eaci_geovariables_2014 (989) | 3,804 | 989 | +| 2017-18 | eaci17_s00p1 (8,390) | eaci_geovariables_2017 (953) | 8,390 | 953 | +| 2018-19 | ehcvm_conso_mli2018 (366,639) | grappe_gps_mli2018 (549) | 366,639 | 551 | +| 2021-22 | s00_me_mli2021 (6,143) | — (single file) | 6,143 | 513 | + +*Deferred, not a defect*: 2018-19 still reads the whole 366,639-row +consumption file to produce 551 cluster rows. It is wasteful, not wrong — +its geo file is grappe-grain so no rows are manufactured — and it cannot be +collapsed to one file the way 2021-22 was, because the GPS genuinely lives +in a separate =grappe_gps_mli2018.dta= there. Slimming =df_main= to the +cover page =s00_me_mli2018.dta= would be a pure-performance change; left +alone so this PR alters no value anywhere. + +*Known cosmetic inconsistency (unchanged here)*: =cluster_features.Rural= +ships the raw French =Urbain= while =sample.Rural= maps it to =Urban=. +Pre-existing and visible in 2014-15 too; fixing it would change returned +values, which this PR deliberately does not do. Adjacent to GH #602. + * Household Presence / MonthsSpent The EHCVM questionnaire (used in the 2018-19 and 2021-22 waves) does diff --git a/tests/test_gh323_mali_cartesian.py b/tests/test_gh323_mali_cartesian.py new file mode 100644 index 000000000..06f676bd4 --- /dev/null +++ b/tests/test_gh323_mali_cartesian.py @@ -0,0 +1,166 @@ +"""GH #323 site 4 / GH #627 -- Mali's `cluster_features` merge must not MANUFACTURE rows. + +Mali 2021-22 was the single worst `dfs:` cartesian in the corpus: 88% of the +4,907,774 phantom rows PR #627's 40-country census found came from this one +cell. The block joined + + df_main ehcvm_conso_mli2021.dta 393,480 rows (household x food item) + df_geo s00_me_mli2021.dta 6,143 rows (household cover page) + +on the CLUSTER key `v`. Both sub-frames repeat `v`, so the merge was +many-to-many: every consumption line-item paired with every household of its +grappe. 393,480 x 6,143 -> 4,718,148 rows, 4,324,668 of them phantoms. +`_normalize_dataframe_index` then collapsed the wreck to 513 rows with +groupby().first(), which is why the table looked clean the whole time. + +WHERE THE TEST HAS TO LOOK. Do NOT assert on the country-level API frame: the +collapse makes it 513 rows with or without the bug, and its VALUES are +bit-for-bit identical before and after this fix (verified over all 513 grappes +x 4 columns). A country-level test therefore passes with the bug fully +present. The phantoms exist only in the WAVE-level frame, upstream of the +collapse -- so that is what these tests count. + +THE FIX IS TO THE MERGE, NOT A REDUCER (#323 decision D1). s00_me_mli2021.dta +carries the geography itself -- s00q01 == `region`, s00q04 == `milieu` -- so the +merge was never needed and the block is now a single-file extraction. The +equivalence that licenses that is asserted below rather than merely asserted in +a comment: if a future edit repoints Region/Rural at a column that does NOT +agree with the consumption file, the test says so. +""" +import pandas as pd +import pytest + +from lsms_library.country import Country + +# Wave-level `cluster_features` row counts. These are the PRE-collapse frames, +# so a cartesian shows up here and nowhere else. +# +# 2021-22 pre-fix: 4,718,148. The other three waves merge a household- or +# consumption-grain primary against a GEO file that is one row per grappe, so +# they are m:1 joins and were never cartesian -- they are pinned here as a +# regression net, not because they were ever broken. +EXPECTED_WAVE_ROWS = { + '2014-15': 3804, # eaci2014_agregatconso x eaci_geovariables_2014 (989 grappes) + '2017-18': 8390, # eaci17_s00p1 x eaci_geovariables_2017 (953 grappes) + '2018-19': 366639, # ehcvm_conso_mli2018 x grappe_gps_mli2018 (549 grappes) + '2021-22': 6143, # single file: s00_me_mli2021 (household cover page) +} + +# True cluster counts, straight off the source files. +EXPECTED_GRAPPES = {'2014-15': 989, '2017-18': 953, '2018-19': 551, '2021-22': 513} + + +@pytest.fixture(scope='module') +def mali(): + try: + return Country('Mali') + except Exception as exc: # pragma: no cover + pytest.skip(f'Mali unavailable: {exc}') + + +def _wave_cluster_features(c, wave): + try: + return c[wave].cluster_features() + except Exception as exc: # pragma: no cover + pytest.skip(f'Mali/{wave} cluster_features could not be built: ' + f'{type(exc).__name__}: {exc}') + + +@pytest.mark.parametrize('wave', sorted(EXPECTED_WAVE_ROWS)) +def test_wave_cluster_features_is_not_a_cartesian(mali, wave): + """The wave frame must not exceed the grain of its own sources. + + This is the assertion that fails with the bug present: pre-fix Mali 2021-22 + returned 4,718,148 rows against 6,143 households in 513 grappes. + """ + if wave not in mali.waves: # pragma: no cover + pytest.skip(f'{wave} not a Mali wave') + df = _wave_cluster_features(mali, wave) + expected = EXPECTED_WAVE_ROWS[wave] + assert len(df) == expected, ( + f'Mali/{wave} cluster_features returned {len(df)} wave-level rows, ' + f'expected {expected}. A blow-up here means the `dfs:` merge in ' + f'{wave}/_/data_info.yml has gone many-to-many again -- both ' + f'sub-frames repeating the merge key `v` produces a CARTESIAN ' + f'PRODUCT, not a join (GH #323 site 4). Fix the merge; do NOT add a ' + f'reducer downstream.' + ) + + +def test_2021_22_cluster_features_is_a_single_file_extraction(mali): + """No `dfs:` block -- the cover page supplies all four columns itself.""" + info = mali['2021-22'].resources.get('cluster_features') + assert info is not None, 'Mali/2021-22 declares no cluster_features' + assert 'dfs' not in info, ( + 'Mali/2021-22 cluster_features has grown a `dfs:` merge block again. ' + 's00_me_mli2021.dta carries Region (s00q01), Rural (s00q04) AND the ' + 'GPS fix, so no merge is needed; merging it against the ' + '(household x food item) consumption file on the cluster key `v` is ' + 'the 4.3M-phantom-row cartesian of GH #323 site 4.' + ) + assert info.get('file') == 's00_me_mli2021.dta', info.get('file') + + +@pytest.mark.parametrize('wave', sorted(EXPECTED_GRAPPES)) +def test_country_cluster_features_is_one_row_per_grappe(mali, wave): + """After the collapse: exactly one row per surveyed cluster, no more.""" + cf = mali.cluster_features() + if wave not in cf.index.get_level_values('t'): # pragma: no cover + pytest.skip(f'{wave} absent from cluster_features') + sub = cf.xs(wave, level='t') + assert len(sub) == EXPECTED_GRAPPES[wave], ( + f'Mali/{wave} cluster_features has {len(sub)} rows for ' + f'{EXPECTED_GRAPPES[wave]} grappes') + assert sub.index.is_unique + + +def test_cover_page_geography_agrees_with_the_consumption_file(): + """s00q01 == `region` and s00q04 == `milieu`, per grappe, 513/513. + + This equivalence is what makes dropping the merge value-preserving. It is + checked against the data rather than asserted in a YAML comment, because an + unevidenced "these are the same column" claim is exactly the kind of thing + that rots silently. + """ + from lsms_library.local_tools import get_dataframe + s00 = get_dataframe('Mali/2021-22/Data/s00_me_mli2021.dta') + conso = get_dataframe('Mali/2021-22/Data/ehcvm_conso_mli2021.dta') + for df in (s00, conso): + df['grappe'] = df['grappe'].astype(str) + + cover = (s00.groupby('grappe')[['s00q01', 's00q04']] + .agg(lambda s: sorted(set(s.astype(str))))) + cons = (conso.groupby('grappe')[['region', 'milieu']] + .agg(lambda s: sorted(set(s.astype(str))))) + + assert (cover['s00q01'].map(len) == 1).all(), 'Region not constant within grappe' + assert (cover['s00q04'].map(len) == 1).all(), 'Rural not constant within grappe' + + joined = cover.join(cons, how='outer') + assert len(joined) == 513, f'{len(joined)} grappes, expected 513' + assert joined.notna().all().all(), 'grappe sets differ between the two files' + + bad_region = joined[joined['s00q01'] != joined['region']] + bad_milieu = joined[joined['s00q04'] != joined['milieu']] + assert bad_region.empty, ( + f'{len(bad_region)} grappe(s) disagree on Region between the cover page ' + f'(s00q01) and the consumption file (region):\n{bad_region.head()}') + assert bad_milieu.empty, ( + f'{len(bad_milieu)} grappe(s) disagree on Rural between the cover page ' + f'(s00q04) and the consumption file (milieu):\n{bad_milieu.head()}') + + +def test_2021_22_gps_is_a_cluster_fix_not_a_household_fix(): + """One distinct (lat, lon) per grappe -- so `.first()` cannot pick wrongly. + + The published EHCVM GPS is the cluster's displaced fix stamped on every + household of the cluster (CLAUDE.md, GH #161). Verified here for Mali + 2021-22: 513/513 grappes carry exactly one distinct coordinate pair. + """ + from lsms_library.local_tools import get_dataframe + s00 = get_dataframe('Mali/2021-22/Data/s00_me_mli2021.dta') + s00['grappe'] = s00['grappe'].astype(str) + n = s00.groupby('grappe')[['GPS__Latitude', 'GPS__Longitude']].nunique() + assert (n['GPS__Latitude'] == 1).all() and (n['GPS__Longitude'] == 1).all(), ( + 'Mali 2021-22 GPS varies WITHIN a grappe; the (t, v) collapse would ' + 'then be picking one household\'s coordinates for the whole cluster.')