Skip to content

fix(#323) Site 4: the dfs: merge MANUFACTURES the duplicates — 4.9M phantom rows (UNBLOCKED: Ethiopia/Niger/Nigeria configs merged)#627

Merged
ligon merged 6 commits into
developmentfrom
fix/323-site4-dfs-merge
Jul 22, 2026
Merged

fix(#323) Site 4: the dfs: merge MANUFACTURES the duplicates — 4.9M phantom rows (UNBLOCKED: Ethiopia/Niger/Nigeria configs merged)#627
ligon merged 6 commits into
developmentfrom
fix/323-site4-dfs-merge

Conversation

@ligon

@ligon ligon commented Jul 13, 2026

Copy link
Copy Markdown
Owner

Closes Site 4 of GH #323 — the dfs: outer merge that manufactures the duplicate rows every other site then collapses. Upstream of the whole problem. See slurm_logs/DESIGN_grain_collapse_sites_2026-07-13.org.

Touches only Wave.grab_data's dfs: region. Sites 1 and 2 untouched.

Body revised 2026-07-22 after the adversarial review below returned FIX_FIRST. Every number here has been re-derived on the current branch (merged with development), cold, in an isolated LSMS_DATA_DIR. Claims the earlier body got wrong are marked struck and corrected — see the "Corrections" section.

✅ No longer blocked

Country('Ethiopia'|'Nigeria'|'Niger').cluster_features() used to raise under this PR. Those were true positives — 10 real config bugs the #515 fallback had been silently swallowing. All three fixes are now merged into development, and development is merged into this branch.

country fix re-verified cold against this branch
Nigeria (6 waves) #625 (194b55d0) ✅ 5,248 rows, Latitude on 5,200, 8 round-waves
Ethiopia (3 cells) #628 (3488b791) + #644 ✅ 2,168 rows, 5/5 waves, Latitude on 2,146
Niger (1 cell) #628 (3488b791) ✅ 1,599 rows, 4/4 waves; 2011-12 goes 0 → 270/270 coordinates

Nigeria had been served without GPS in six waves for as long as the #515 fallback existed — a casing mismatch (LAT_DD_MODlat_dd_mod) and a geo file keyed hhid where the YAML asks ea.

Niger's geo file has no lat/lon at all. False, and the correction matters. NER_HouseholdGeovars_Y1.dta has none (4051 × 43, derived raster covariates only) — but the coordinates ship in the sibling file in the same DVC-tracked directory, NER_EA_Offsets.dta: 271 × ['grappe', 'LAT_DD_MOD', 'LON_DD_MOD'], 270 unique non-null grappes, lat 11.876–18.747 N. Re-measured from source here. optional: true would have deleted a real column and, since data_scheme.yml is country-grain, disarmed this PR's own guard for the three Niger waves that do carry coordinates.

⚠️ The full suite is GREEN and that green is a LIE

Still true, and independently re-derived by three separate agents on this thread. test_table_structure reads var/*.parquet directly ("only test what is already cached") — it never rebuilds, so a cache-reading test is blind to a build-path guard.

One correction to the mechanism, though: that blindness is test_table_structure's read-only contract, not the v0.8.0 hash failing to notice a core change. The hash does notice — see the dispute under review finding 3 below.

The change

  • _cartesian_keys — exact many-to-many detection. A key value duplicated in both sub-frames is the cartesian, so the test is sound and complete. The row-count ceiling (len(out) <= len(left)+len(right)) was rejected: sound but not complete — two 3-row frames sharing a 2×2 and a 1×1 key yield 5 rows and never breach the 6-row ceiling. dropna=False is load-bearing: pd.merge matches null keys, so nulls explode like any other value.
  • _merge_subframes — warns by default with country/wave/table/keys and an exact phantom count; fatal under LSMS_GRAIN_STRICT, read through the same _grain_strict() predicate as sites 1–2 (fixed in review; see below). Warning-by-default keeps every currently-building country bit-for-bit unchanged.
  • merge_how:kept, because it is read config, not dead config: core reads it, all five Ethiopia waves and Niger 2011-12 set merge_how: left, and it is tested end-to-end. (Contrast the rejected aggregation: key, which nothing read.)
  • [feature-audit] household_roster / market=Region: _market_lookup: skipping cluster_features for Burkina_Faso/2014 (KeyError: 'lat_dd_mod not in columns of dataframe.') #515 KeyError-swallow → a dropped sub-df that costs a required declared column now raises, judged after derived: / drop: / the df_edit hook (fixed in review). A merely unavailable file still degrades softly.
  • Factored required/optional scheme parsing into one _required_scheme_columns, shared with Country._assert_built_required_columns.

No aggregation: key wired up. D1 honoured: a cartesian merge is fixed by making the merge correct, never by reducing afterwards.

The census — original (config at 45aee170)

67 dfs: merges across 19 countries. 76 dfs: blocks across 20 countries — a statically checkable declaration count, which is what this line claimed to be. 67 was the number of merges the sweep executed; two different quantities were reported as one.

8 cartesian cells / 5 countries / 4,907,774 phantom rows. All are cluster_features merged on the cluster key v with both sub-frames at household grain. (Reproduced to the row by the reviewer.)

country wave L × R 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 / 2013Q1 4,859 × 4,802 62,538 ea. 57,476 ea.
Ethiopia 2015-16 4,954 × 4,954 57,786 52,832
Guinea-Bissau 2018-19 5,351 × 450 5,410 59

The census — RE-MEASURED on the merged tree

Full re-sweep, isolated LSMS_DATA_DIR, 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 dir under two round labels
CARTESIAN cells: 3
cell merged phantom owner
Malawi 2010-11 196,083 183,812 #653, open
Malawi 2019-20 185,842 171,230 #653, open
Guinea-Bissau 2018-19 5,410 59 #653, open

Those three phantom counts are identical to the ones the original census recorded, measured by a different sweep on a different day — the census reproduces.

4,552,673 of 4,907,774 phantom rows (92.8%) are gone: Mali 2021-22 to #641 (which deleted the merge outright — the cover page carried the geography), Ethiopia's two to #628's household_id2 re-key, Nigeria's two to #625's v re-key. 355,101 remain, all three cells in one open PR.

Only failures in the sweep: Nepal 1995-96 / 2003-04 sample, PathMissingError — no data in the repo, pre-existing. 0 required-column raises anywhere in the library.

Three findings that contradicted the brief I was given

  1. The re-key is household_id2, not household_id. household_id is the W1 id, blank for new panel households, hence non-unique in both W2/W3 sub-frames. Re-keying to it would have traded an EA cartesian for a null-key cartesian. Independently confirmed on this thread.
  2. The [feature-audit] household_roster / market=Region: _market_lookup: skipping cluster_features for Burkina_Faso/2014 (KeyError: 'lat_dd_mod not in columns of dataframe.') #515 required-column error hits 3 countries, not 1 — 10 cells, all true positives.
  3. PP/PH round-crossing is empirically empty here. 0 of the 67 sub-frames comes from a _pp_/_ph_ file. That was a filename test overstated into a provenance claim — Nigeria's df_main is Post Planting Wave 1/.../secta_plantingw1.dta. The substantive conclusion stands on the right basis: t is constant within every dfs: merge in the corpus, so no merge here can cross rounds. Tanzania declares no dfs: block; Nigeria's Country.waves already yields round labels, so the census was natively per-round.

Corrections applied after the adversarial review

Full write-up in .coder/ledger/323-site4-dfs-merge.md §11.

# finding outcome
1 (HIGH) merge order: Nigeria's fix was still unlanded discharged#625 merged; development merged in; all three countries re-verified cold
2 (MED) optional: true is country-wide; the guard's invariant isn't what it looks like documented + message fixed — the guard is "a mis-named column in a dfs: sub-df is fatal", not "a required column is never absent". Niger 2014-15 serves Latitude 0/270 without raising, because that wave has no dfs: block
3 (MED) the hard error is cache-state-dependent because country.py isn't in the v0.8.0 hash DISPUTED — premise measured false. grab_data carries @build_transform(), and btf=build_transforms_fingerprint(table) folds into every wave hash. Holding config fixed and varying only the core: dev 16e4e8c6… vs branch 9c475fa1…; mutating only _merge_subframes's body moves it again. Nigeria's real warm parquet grades stale under this branch. Two narrower bypasses are real and are now documented on that basis: a hashless pre-v0.8.0 parquet grades legacy (trust-once and re-stamp), and assume_cache_fresh=True
4 (LOW) the required-column check judged presence before derived: / the hook fixed — moved below the hook, matching its country-level twin. Discriminating test added
5 (LOW) the merge_how prose overstates corrected in all three places, with a measurement — see below
nit ledger's "67 merges / 19 countries" corrected to 76 blocks / 20 countries
nit §7's _pp_/_ph_ claim corrected — filename test, not provenance

Plus one reuse fix the review's grep did not cover: _merge_subframes read LSMS_GRAIN_STRICT with a bare os.environ.get, while PR #614 had since merged _grain_strict() into the same module. The copy was not bit-identical — measured, LSMS_GRAIN_STRICT=0 made site 4 raise while sites 1–2 stayed in warn mode. Now one lever, one reader.

Finding 5, measured

Ethiopia 2013-14, 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 "collapse into one phantom cluster" — the cluster-grain collapse deletes them, because groupby drops null keys. Every delivered value is identical; only the dtype differs. So merge_how: left is not a data fix: it stops manufacturing null-keyed rows for site 1 to delete, and stops the merge widening an integer column. Its cost is a lost signal — under outer the site-1 grain report told you 25 geo households had no cover page.

Not fixed here, flagged for #628's owner: Ethiopia/_/CONTENTS.org (~L531) and Ethiopia/2013-14/_/data_info.yml's merge_how comment tell the "spurious 434th EA" story next to the new config. That story is true of the pre-#628 config, where df_geo was keyed v: ea_id2; since #628 it is keyed on i only, so the orphans get a null v instead. This PR touches no country config, so those two lines are left to their owner.

Tests

tests/test_gh323_site4_dfs_merge.py goes from 15 test functions / 16 collected items to 20 / 28 (both counted, not estimated). The reviewer killed 8 mutants against the original set. The 5 new functions cover the review fixes, and two are proved discriminating by reverting the change:

  • test_grain_strict_is_read_through_the_one_shared_predicate — 4 failures with the old env read.
  • test_required_column_supplied_by_the_wave_hook_is_not_reported_absentRuntimeError with the check in its old position.

The module has no skip, no xfail and no microdata — it builds a synthetic one-wave country in tmp_path from CSVs it writes itself, so nothing in it can go vacuous in a data-free CI job.

Note: the first build after this merges is a full cold rebuild library-wide — changing grab_data changes its @build_transform fingerprint, correctly invalidating every cached parquet. (Measured; this is also why review finding 3's premise is disputed.)

Ledger: .coder/ledger/323-site4-dfs-merge.md. Refs #323, #515, #614.

🤖 Generated with Claude Code

ligon added a commit that referenced this pull request Jul 20, 2026
fix(#323): Ethiopia + Niger geo — 3 of Ethiopia's 5 waves had ZERO coordinates (unblocks #627)
ligon added a commit that referenced this pull request Jul 21, 2026
…ost the column

Niger's site-4 cell was already fixed on `development` by 3488b79; PR #627's
body still lists it as "not yet written".  This re-derived the diagnosis
independently from the data, verified cold against #627's core that Niger is
genuinely cleared, and adds the regression test the fix did not carry.

WHAT THE CELL WAS.  `Niger/2011-12/_/data_info.yml`'s `cluster_features.df_geo`
pointed at `NER_HouseholdGeovars_Y1.dta` and asked for `lat_dd_mod` /
`lon_dd_mod`.  Under #627's core:

  RuntimeError: Niger/2011-12/cluster_features: sub-df(s) 'df_geo' ... loaded
  but do NOT carry the column(s) the YAML asks for, leaving required declared
  column(s) ['Latitude', 'Longitude'] ENTIRELY ABSENT from 'cluster_features'.
  First error: KeyError('lat_dd_mod not in columns of dataframe.')

Exactly one failing cell.  #627's brief said "Niger's geo file has no lat/lon at
all", which invites `optional: true`.  THAT IS THE TRAP, and it is the reason
this test exists.

THE EVIDENCE, from the data.  `NER_HouseholdGeovars_Y1.dta` is 4051 x 43 at
HOUSEHOLD grain and carries no coordinate column of any name -- only derived
raster covariates (dist_road, dist_popcenter, dist_market, af_bio_*, srtm, twi,
sq1..sq7, EVI/NDVI).  Withholding the coordinates is what a geovariables extract
is FOR.  But the coordinates ship in the sibling `NER_EA_Offsets.dta`, already
DVC-tracked in the same directory: 271 x (grappe, LAT_DD_MOD, LON_DD_MOD), 270
non-null grappes + one trailing null-key row, lat 11.876-18.747 N / lon
0.405-13.698 E.  Its grappe set matches ecvmamen_p1's exactly (270/270, no
orphans either way; all 3968 households covered), so the merge on v is 1:many,
not cartesian.  Wrong file AND wrong casing -- not missing data.

`optional: true` would also have been wrong in a second way nobody would have
noticed: `data_scheme.yml` is COUNTRY-grain, so silencing one wave disarms the
required-column guard for the three waves that DO have coordinates (2018-19
grappe_gps_ner2018, 2021-22 s00_me_ner2021 GPS__*).

2014-15 genuinely ships no geovariables and no offsets file, and its cover page
has no coordinate column -- honestly absent, correctly not wired.  It is invisible
to the #323/#515 guard, which fires only on a DROPPED sub-df.  No absent-verdict
is filed: the ECVMA-II questionnaire has not been read, and an unevidenced
closing verdict is worse than an open one.

VERIFIED (#627's core on PYTHONPATH with `lsms_library.__file__` asserted to
resolve there; this config tree via LSMS_COUNTRIES_ROOT, asserted; isolated
LSMS_DATA_DIR wiped before each run with only dvc-cache symlinked -- COLD,
because LSMS_NO_CACHE=1 alone is soft for script-path L2-wave parquets;
LSMS_GRAIN_STRICT=1 so any cartesian is fatal):

  before (pre-3488b791 config)   RuntimeError, as quoted above
  after                          builds, 1599 rows

             rows  Latitude  Longitude  Region
  2011-12     270       270        270     270    <- was 0
  2014-15     270         0          0     270    <- not shipped
  2018-19     504       493        493     504
  2021-22     555       555        555     555

  2011-12: 0 duplicate v, 0 null v, lat/lon inside Niger's bounding box.
  ALL 22 Niger tables build cold under LSMS_GRAIN_STRICT=1 -- 0 raises,
  0 cartesian warnings.

WHAT THIS ADDS.  9 tests, no config change (development already carries it).
Nothing in tests/ mentioned `EA_Offsets`, `LAT_DD_MOD`, or Niger
`cluster_features`; the reasoning lived only in a YAML comment, and prose is not
enforcement.  The tests pin the file, the casing, `merge_how: left` and the one
trailing null-key row that makes it load-bearing, the geovariables file's
ABSENCE of coordinates (the negative control, made permanent), that
Latitude/Longitude stay REQUIRED in data_scheme.yml, and the built table's
coverage + grain + freedom from a phantom null-key cluster.

Deliberately NOT included: origin/fix/323-niger's 3 `aggregation:` keys (dead
config, forbidden by D1) and its EXTENSION re-key / majority-vote cluster_features
/ plot_inputs reductions -- all Site 1 (#614) work routing through
`Niger/_/niger.py`, which GH #637 owns.  No file under lsms_library/*.py.

Ledger: .coder/ledger/323-niger-config.md.  Refs #323, #515, #627.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@ligon

ligon commented Jul 21, 2026

Copy link
Copy Markdown
Owner Author

Mali 2021-22 is owned and fixed: #641. That is 4,324,668 of your 4,907,774 phantom rows — 88% of the census, in one config-only cell.

Your census line reproduces exactly under this branch's detector:

Mali/2021-22/cluster_features: the `dfs:` merge of 'df_main' (393480 rows) with
'df_geo' (6143 rows) on ['v', 't'] is a many-to-many CARTESIAN PRODUCT, not a
join. 513 key value(s) are duplicated in BOTH sub-frames (e.g. ['v','t'] =
('1','2021-22'), 727 x 12 rows), MANUFACTURING 4324668 phantom rows...

After #641: 4,718,148 → 6,143 wave rows, phantom count 0.

Two things worth adding to your table's context:

  1. The merge was deletable, not merely fixable. s00_me_mli2021.dta (the df_geo cover page) carries the geography itself — s00q01 == region and s00q04 == milieu, agreeing on 513/513 grappes, each constant within its grappe, over the identical 6,143 (grappe, menage) households. So it collapses to a single-file extraction: cartesian cured at the merge per D1, no reducer, no aggregation: key. Worth checking whether Malawi's two cells and Guinea-Bissau's have the same shape — a df_geo that already carries what df_main was fetched for.

  2. Your "the green is a lie" warning was load-bearing here, in a second way. Not only can test_table_structure not see this — nothing at the country level can. Mali's country-level cluster_features is 513 rows for 2021-22 with or without the bug, and its values are bit-for-bit identical before and after the fix (DataFrame.equals() is True over all 3,006 rows × 4 columns). The 4.3M phantoms live only in the wave-level frame, upstream of _normalize_dataframe_index. So fix(#323): Mali 2021-22 cluster_features — delete the 4.3M-phantom-row cartesian (88% of #627's census) #641's tests assert at the wave level, and its negative control shows exactly that asymmetry: with the pre-fix YAML restored, the two wave-level tests fail and all four country-level tests pass right through the bug.

    The practical consequence for your LSMS_GRAIN_STRICT=1-in-CI plan: a cell can be cartesian-clean-looking at every cached and API-level instrument. _cartesian_keys at the merge really is the only place this is observable.

Mali's other three waves are clean, consistent with your census — their df_geo is one row per grappe (989 / 953 / 549), so those joins are m:1. One deferral noted in #641: 2018-19 reads a 366,639-row consumption file to produce 551 cluster rows. It manufactures nothing, but unlike 2021-22 it can't be collapsed to one file — its GPS genuinely lives in a separate grappe_gps_mli2018.dta.

Remaining unowned after #641: Malawi (2010-11 + 2019-20, 355,042 phantom) and Guinea-Bissau (2018-19, 59).

ligon added a commit that referenced this pull request Jul 21, 2026
…ost the column

Niger's site-4 cell was already fixed on `development` by 3488b79; PR #627's
body still lists it as "not yet written".  This re-derived the diagnosis
independently from the data, verified cold against #627's core that Niger is
genuinely cleared, and adds the regression test the fix did not carry.

WHAT THE CELL WAS.  `Niger/2011-12/_/data_info.yml`'s `cluster_features.df_geo`
pointed at `NER_HouseholdGeovars_Y1.dta` and asked for `lat_dd_mod` /
`lon_dd_mod`.  Under #627's core:

  RuntimeError: Niger/2011-12/cluster_features: sub-df(s) 'df_geo' ... loaded
  but do NOT carry the column(s) the YAML asks for, leaving required declared
  column(s) ['Latitude', 'Longitude'] ENTIRELY ABSENT from 'cluster_features'.
  First error: KeyError('lat_dd_mod not in columns of dataframe.')

Exactly one failing cell.  #627's brief said "Niger's geo file has no lat/lon at
all", which invites `optional: true`.  THAT IS THE TRAP, and it is the reason
this test exists.

THE EVIDENCE, from the data.  `NER_HouseholdGeovars_Y1.dta` is 4051 x 43 at
HOUSEHOLD grain and carries no coordinate column of any name -- only derived
raster covariates (dist_road, dist_popcenter, dist_market, af_bio_*, srtm, twi,
sq1..sq7, EVI/NDVI).  Withholding the coordinates is what a geovariables extract
is FOR.  But the coordinates ship in the sibling `NER_EA_Offsets.dta`, already
DVC-tracked in the same directory: 271 x (grappe, LAT_DD_MOD, LON_DD_MOD), 270
non-null grappes + one trailing null-key row, lat 11.876-18.747 N / lon
0.405-13.698 E.  Its grappe set matches ecvmamen_p1's exactly (270/270, no
orphans either way; all 3968 households covered), so the merge on v is 1:many,
not cartesian.  Wrong file AND wrong casing -- not missing data.

`optional: true` would also have been wrong in a second way nobody would have
noticed: `data_scheme.yml` is COUNTRY-grain, so silencing one wave disarms the
required-column guard for the three waves that DO have coordinates (2018-19
grappe_gps_ner2018, 2021-22 s00_me_ner2021 GPS__*).

2014-15 genuinely ships no geovariables and no offsets file, and its cover page
has no coordinate column -- honestly absent, correctly not wired.  It is invisible
to the #323/#515 guard, which fires only on a DROPPED sub-df.  No absent-verdict
is filed: the ECVMA-II questionnaire has not been read, and an unevidenced
closing verdict is worse than an open one.

VERIFIED (#627's core on PYTHONPATH with `lsms_library.__file__` asserted to
resolve there; this config tree via LSMS_COUNTRIES_ROOT, asserted; isolated
LSMS_DATA_DIR wiped before each run with only dvc-cache symlinked -- COLD,
because LSMS_NO_CACHE=1 alone is soft for script-path L2-wave parquets;
LSMS_GRAIN_STRICT=1 so any cartesian is fatal):

  before (pre-3488b791 config)   RuntimeError, as quoted above
  after                          builds, 1599 rows

             rows  Latitude  Longitude  Region
  2011-12     270       270        270     270    <- was 0
  2014-15     270         0          0     270    <- not shipped
  2018-19     504       493        493     504
  2021-22     555       555        555     555

  2011-12: 0 duplicate v, 0 null v, lat/lon inside Niger's bounding box.
  ALL 23 Niger tables build cold under LSMS_GRAIN_STRICT=1 -- 0 raises,
  0 cartesian warnings.

WHAT THIS ADDS.  9 tests, no config change (development already carries it).
Nothing in tests/ mentioned `EA_Offsets`, `LAT_DD_MOD`, or Niger
`cluster_features`; the reasoning lived only in a YAML comment, and prose is not
enforcement.  The tests pin the file, the casing, `merge_how: left` and the one
trailing null-key row that makes it load-bearing, the geovariables file's
ABSENCE of coordinates (the negative control, made permanent), that
Latitude/Longitude stay REQUIRED in data_scheme.yml, and the built table's
coverage + grain + freedom from a phantom null-key cluster.

Deliberately NOT included: origin/fix/323-niger's 3 `aggregation:` keys (dead
config, forbidden by D1) and its EXTENSION re-key / majority-vote cluster_features
/ plot_inputs reductions -- all Site 1 (#614) work routing through
`Niger/_/niger.py`, which GH #637 owns.  No file under lsms_library/*.py.

Ledger: .coder/ledger/323-niger-config.md.  Refs #323, #515, #627.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ligon added a commit that referenced this pull request Jul 21, 2026
…w cartesian

The `dfs:` block joined `ehcvm_conso_mli2021.dta` (393,480 rows, household x
food item) to `s00_me_mli2021.dta` (6,143 rows, household cover page) on the
CLUSTER key `v`.  Both sides repeat `v` across 513 grappes, so the merge was
many-to-many by construction: 393,480 x 6,143 -> 4,718,148 rows, of which
4,324,668 were phantoms that exist in no survey.  That is 88% of the 4,907,774
phantom rows PR #627's 40-country census found, from one unowned cell.
`_normalize_dataframe_index` collapsed the wreck to 513 rows with
groupby().first() and the L2-country parquet is written post-collapse, so the
table looked clean at every point anyone ever looked.

The merge was never needed.  s00_me_mli2021.dta carries the geography itself:
s00q01 == `region` and s00q04 == `milieu`, agreeing on 513/513 grappes, each
constant within its grappe, over the identical 6,143 (grappe, menage)
households.  So this is now a single-file extraction -- the cartesian is cured
AT THE MERGE (#323 decision D1), not laundered by a reducer afterwards.  Prior
art: `sample.df_cover` in this same file already reads Rural from s00q04, and
2017-18's cluster_features already reads Region/Rural off its cover page.

Value-preserving: the full country-level frame (3,006 rows x 4 columns, all
four waves) is bit-for-bit identical before and after -- DataFrame.equals()
True, index equal, dtypes equal.  4,718,148 -> 6,143 wave rows; 513 country
rows for 2021-22, exactly Mali's grappe count.

Mali's other three waves are not cartesian: their df_geo is one row per grappe
(989 / 953 / 549), so those merges are m:1.  2018-19 still reads a 366,639-row
consumption file to produce 551 cluster rows -- wasteful, not wrong, and it
cannot be collapsed to one file because its GPS lives in a separate
grappe_gps_mli2018.dta.  Deferred; documented.

Tests assert at the WAVE level on purpose: the country-level frame is 513 rows
with or without the bug, so a country-level assertion passes with the bug fully
present.  Negative control run with the pre-fix YAML restored in a fresh
isolated data root: 2 failed, 9 passed (the four country-level tests passed
right through the bug).  After: 11 passed.  Targeted regression set: 313
passed.  All measurements cold, in an isolated LSMS_DATA_DIR, with #627's
_cartesian_keys detector on PYTHONPATH.

Config-only: no lsms_library/*.py, no `aggregation:` key.

Ledger: .coder/ledger/323-mali-cartesian.md
Diagnosis: lsms_library/countries/Mali/_/CONTENTS.org

Refs #323, #627.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@ligon

ligon commented Jul 21, 2026

Copy link
Copy Markdown
Owner Author

✅ Niger is cleared — the blocker table above is stale

The body lists Niger as "not yet written". It has been written: 3488b791 ("fix(#323): geo config for Ethiopia + Niger — unblock the site-4 required-column raise") landed on development after this body was authored, and it covers both remaining rows of the table. I was dispatched to write Niger's fix, re-derived the diagnosis independently from the source data, arrived at the same answer, and then verified it cold against this branch.

Niger's one failing cell was 2011-12 / cluster_features:

RuntimeError: Niger/2011-12/cluster_features: sub-df(s) 'df_geo'
  (file: NER_2011_ECVMA_v01_M_Stata8/NER_HouseholdGeovars_Y1.dta) loaded but do
  NOT carry the column(s) the YAML asks for, leaving required declared column(s)
  ['Latitude', 'Longitude'] ENTIRELY ABSENT from 'cluster_features'.
  First error: KeyError('lat_dd_mod not in columns of dataframe.')

On "Niger's geo file has no lat/lon at all" — half true, and the half that isn't is a trap

NER_HouseholdGeovars_Y1.dta genuinely has no coordinate column of any name (4051 × 43, household grain, derived raster covariates only: dist_road, dist_market, af_bio_*, srtm, twi, sq1..sq7, EVI/NDVI). That invites optional: true.

But the coordinates ship in the sibling file in the same directory, already DVC-trackedNER_EA_Offsets.dta, the WB's standard offset-EA-centroid release: 271 × ['grappe', 'LAT_DD_MOD', 'LON_DD_MOD'], 270 non-null grappes + one trailing null-key row, lat 11.876–18.747 N / lon 0.405–13.698 E. The names the YAML asked for were the right names in the wrong case, pointed at the wrong fileexactly Nigeria's LAT_DD_MOD ↔ lat_dd_mod, one file over.

optional: true would have been wrong twice over: it would have deleted a real column, and — because data_scheme.yml is country-grain — it would have disarmed this PR's required-column guard for the three Niger waves that do carry coordinates.

The offsets file is EA-grain and its grappe set matches ecvmamen_p1's exactly (270/270, no orphans either way, all 3968 households covered), so the merge on v is 1:many, not cartesian.

Niger 2014-15 ships no geovariables and no offsets file of any kind, and its cover page has none — honestly absent, correctly not wired, and invisible to this guard anyway (which fires only on a dropped sub-df).

Verification against this branch

This branch's core on PYTHONPATH with lsms_library.__file__ asserted to resolve there; development's config tree via LSMS_COUNTRIES_ROOT, asserted; isolated LSMS_DATA_DIR wiped before each run with only dvc-cache symlinked — cold, since LSMS_NO_CACHE=1 alone is soft for script-path L2-wave parquets; LSMS_GRAIN_STRICT=1 so any cartesian is fatal.

result
pre-3488b791 config (negative control) raises, as quoted above
development today builds, 1599 rows
           rows  Latitude  Longitude  Region
2011-12     270       270        270     270    <- was 0 coordinates
2014-15     270         0          0     270    <- genuinely not shipped
2018-19     504       493        493     504
2021-22     555       555        555     555

2011-12: 0 duplicate v, 0 null v, coordinates inside Niger's bounding box.
All 23 Niger tables — every data_scheme entry plus the runtime-derived food/roster tables — build cold under LSMS_GRAIN_STRICT=1 with 0 raises and 0 cartesian warnings.

I did not rely on the suite; your warning about test_table_structure grading against pre-change parquets is correct, so every number above is from a cold build in a wiped data root.

One note on merge_how:

merge_how is introduced by this PR — it does not exist on development (grep -n merge_how lsms_library/country.py → nothing there). So Niger's merge_how: left is currently inert on development: the offsets file's trailing null-key row arrives through the default outer and is deleted by #614's Site-1 collapse instead of being dropped at the merge. Both paths end at 270 clean clusters with no null v — I checked the built table, not just the config — but it does mean this PR is what makes that line do what its comment says.

Suggested edit to the body

country status
Nigeria (6 waves) ✅ fixed by #625
Ethiopia (3 cells) ✅ fixed by 3488b791 on development
Niger (1 cell) ✅ fixed by 3488b791 on developmentverified cold against this branch

I verified Niger end to end; I did not re-verify Ethiopia's three cells, so someone should confirm that row independently before the table is called green.

#643 adds the regression test the fix did not carry — nothing in tests/ mentioned EA_Offsets, LAT_DD_MOD, or Niger cluster_features, so the reasoning lived only in a YAML comment, and prose is not enforcement. It pins the file, the casing, merge_how: left, the geovariables file's absence of coordinates (the negative control, made permanent), and that Latitude/Longitude stay required. No config change; no lsms_library/*.py.

🤖 Generated with Claude Code

@ligon

ligon commented Jul 21, 2026

Copy link
Copy Markdown
Owner Author

✅ Ethiopia is cleared — and the blocker table above is stale

The Ethiopia row says "config fix written but not yet filed". It was filed: 3488b791 / PR #628, now an ancestor of origin/development. Nobody had confirmed it against this branch's core, so I did. Full write-up in #644 (tests + ledger only — no cluster_features config change, because none is needed).

Method: development merged with fix/323-site4-dfs-merge (git merge-tree59b34cc2) on PYTHONPATH with lsms_library.__file__ asserted; config via LSMS_COUNTRIES_ROOT; isolated LSMS_DATA_DIR whose only warm tier is a symlinked dvc-cache.

Negative control — pre-3488b791 config, this core — reproduces your census exactly: 2011-12 / 2018-19 / 2021-22 RAISE; 2013-14 = 65,508 rows / 60,221 phantom; 2015-16 = 57,786 / 52,832 phantom.

Current development config, this core:

wave 2011-12 2013-14 2015-16 2018-19 2021-22
rows 3,969 5,262 4,954 6,770 4,959
raises no no no no no
cartesian cells 0 0 0 0 0

Country('Ethiopia').cluster_features()2,168 rows, unique (t, v), 2,146 with Latitude. All 25 declared Ethiopia tables build: 0 raised, 0 cartesian, 0 dropped sub-dfs. Your 16 tests in test_gh323_site4_dfs_merge.py pass against it.

Answering the question your census left open

Note #627 also lists Ethiopia 2013-14 (60,221) and 2015-16 (52,832) as cartesian cluster_features cells.

Both are gone. The same re-key that fixed the three raising cells also fixed both cartesians — which is why a census taken before #628 landed could not see it coming. 113,053 of the 4,907,774 phantom rows in your table are already cleared.

Suggested edit to your table:

country status
Ethiopia (3 cells) fixed by #628 — verified against this core: 5/5 waves build, 0 cartesian, 2,168 rows on a unique (t, v); both cartesian cells (60,221 + 52,832 phantom) also cleared

Two things from your PR that proved true in practice

  1. household_id2, not household_id — confirmed independently. household_id is blank for the W2 refreshment sample, so re-keying to it really would trade an EA cartesian for a null-key one.
  2. "the green is a lie" — hit it directly. Running my own tests against the pre-fix config, 3 of the raising waves passed because a stale {wave}/_/cluster_features.parquet shadowed the raise. Only the config-level assertions were unshadowable. Anyone re-verifying a bug: framework silently drops rows on duplicate canonical index (groupby().first()) #323 cell needs a genuinely cold data root, not just LSMS_NO_CACHE=1.

One thing that is not yours, but you will meet it

Country('Ethiopia').individual_education() returns 63,139 rows normally and 62,939 after c.panel_ids is touched first — id_walk is applied only once panel_ids resolves, and the lazy self.updated_ids probe in _aggregate_wave_data swallows a failed first attempt and records it as "attempted". Pre-existing, reproduces in the old config, unrelated to site 4 — but it means the same call can return two different row counts depending on what the process built first. Detail in #644; probably deserves its own issue.

(#644 also fixes a genuinely unfixed Ethiopia site-1 defect it turned up — individual_education W2/W3 keyed on the W1 baseline ids, destroying 5,247 people in W2 and merging two real students in W3. Separate from this PR's path; noted only so it is not double-counted as a #627 blocker.)

ligon added a commit that referenced this pull request Jul 21, 2026
…w cartesian

The `dfs:` block joined `ehcvm_conso_mli2021.dta` (393,480 rows, household x
food item) to `s00_me_mli2021.dta` (6,143 rows, household cover page) on the
CLUSTER key `v`.  Both sides repeat `v` across 513 grappes, so the merge was
many-to-many by construction: 393,480 x 6,143 -> 4,718,148 rows, of which
4,324,668 were phantoms that exist in no survey.  That is 88% of the 4,907,774
phantom rows PR #627's 40-country census found, from one unowned cell.
`_normalize_dataframe_index` collapsed the wreck to 513 rows with
groupby().first() and the L2-country parquet is written post-collapse, so the
table looked clean at every point anyone ever looked.

The merge was never needed.  s00_me_mli2021.dta carries the geography itself:
s00q01 == `region` and s00q04 == `milieu`, agreeing on 513/513 grappes, each
constant within its grappe, over the identical 6,143 (grappe, menage)
households.  So this is now a single-file extraction -- the cartesian is cured
AT THE MERGE (#323 decision D1), not laundered by a reducer afterwards.  Prior
art: `sample.df_cover` in this same file already reads Rural from s00q04, and
2017-18's cluster_features already reads Region/Rural off its cover page.

Value-preserving: the full country-level frame (3,006 rows x 4 columns, all
four waves) is bit-for-bit identical before and after -- DataFrame.equals()
True, index equal, dtypes equal.  4,718,148 -> 6,143 wave rows; 513 country
rows for 2021-22, exactly Mali's grappe count.

Mali's other three waves are not cartesian: their df_geo is one row per grappe
(989 / 953 / 549), so those merges are m:1.  2018-19 still reads a 366,639-row
consumption file to produce 551 cluster rows -- wasteful, not wrong, and it
cannot be collapsed to one file because its GPS lives in a separate
grappe_gps_mli2018.dta.  Deferred; documented.

Tests assert at the WAVE level on purpose: the country-level frame is 513 rows
with or without the bug, so a country-level assertion passes with the bug fully
present.  Negative control run with the pre-fix YAML restored in a fresh
isolated data root: 2 failed, 9 passed (the four country-level tests passed
right through the bug).  After: 11 passed.  Targeted regression set: 313
passed.  All measurements cold, in an isolated LSMS_DATA_DIR, with #627's
_cartesian_keys detector on PYTHONPATH.

Config-only: no lsms_library/*.py, no `aggregation:` key.

Ledger: .coder/ledger/323-mali-cartesian.md
Diagnosis: lsms_library/countries/Mali/_/CONTENTS.org

Refs #323, #627.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ligon added a commit that referenced this pull request Jul 21, 2026
…index bug

VERIFICATION, not re-implementation.  PR #627's body lists Ethiopia as an
unfixed blocker with three raising `cluster_features` cells.  That table is
stale: the fix landed as `3488b791` (PR #628) and is already an ancestor of
`development`.  Nobody had confirmed it against #627's actual core.  This does.

Measured against #627's core (`development` merged with
`fix/323-site4-dfs-merge` via `git merge-tree`, asserted at runtime through
`lsms_library.__file__`), cold, with an isolated `LSMS_DATA_DIR` whose only
warm tier is a symlinked `dvc-cache`:

  NEGATIVE CONTROL (pre-3488b791 config, same core) — reproduces #627's census
  exactly: 2011-12 / 2018-19 / 2021-22 RAISE; 2013-14 is a 65,508-row cartesian
  (60,221 phantom); 2015-16 is 57,786 rows (52,832 phantom).  113,053 phantom
  rows total.

  CURRENT config, same core: all five waves build, 0 cartesian cells, 0 phantom
  rows.  `Country('Ethiopia').cluster_features()` -> 2,168 rows on a unique
  (t, v), 2,146 with Latitude.  ALL 25 declared Ethiopia tables build: 0 raised,
  0 cartesian, 0 dropped sub-dfs.  #627's own 16 tests pass against this config.

The same re-key that cleared the three raising cells ALSO cleared both cartesian
cells — which is why #627's census, taken before #628 landed, could not see it.
Ethiopia is genuinely green; #627 is unblocked on Ethiopia's account.

THE ONE REAL GAP FOUND — `individual_education`, 2013-14 and 2015-16.  Unrelated
to #627, a #323 site-1 true positive, and unfixed until now.  Both waves indexed
education on the W1-BASELINE `household_id` / `individual_id` while
`household_roster` and `shocks` for the SAME waves correctly used the wave-native
`household_id2` / `individual_id2`:

  W2  the baseline pair is BLANK for the urban refreshment sample (no W1
      antecedent) — 5,248 of 23,785 rows, all landing on one tuple.  The
      framework's own audit: "DESTROYED 5,247 of 23,785 rows whose values
      DISAGREE ... Additionally 5,248 row(s) carry NaN in a declared index level
      and are DELETED OUTRIGHT".
  W3  split-off households inherit their parent's baseline id, so 16 rows in 8
      pairs collided and first() MERGED TWO REAL, DISTINCT PEOPLE — one recorded
      '10th Grade Complete', the other '9th Grade Complete'.

It also made education UNJOINABLE to the roster: 0 of the built (i, pid) pairs
matched a roster row in W2/W3, against 100% in W1/W4/W5.

Fixed by correcting the IDENTIFIER, per the standing rule — never by declaring a
reducer.  No `aggregation:` keys; no file under `lsms_library/*.py`.  After:
both wave frames unique (23,785/23,785, 23,393/23,393), both GrainCollapse
warnings gone, roster join 100% in all five waves, 59,092 -> 63,181 rows under
identical conditions.  id_walk + panel_ids still map household_id2 back to the
W1 baseline, so panel linkage is preserved.

DELIBERATELY NOT SALVAGED from the abandoned `fix/323-ethiopia` branch: its
`aggregation:` keys (dead config, contradicts D1 — a test now pins that Ethiopia
declares none); its `interview_date` mapping.py hooks (measured redundant —
`_enforce_canonical_dtypes` already honours the declared `Int_t: datetime`, and
the table returns datetime64, 8,236 rows, 0 nulls); its core patch.

Tests: 21 new, asserted at the WAVE level (post-collapse uniqueness holds by
construction and proves nothing).  Negative control run: 21/21 pass against the
current config, 17/21 FAIL against the pre-3488b791 tree on the same core.

Also corrects a `CONTENTS.org` claim that #628 left behind and that is now
false — the cluster collapse no longer takes `mean()` for GPS.  Do not assume
the flip is a no-op here: measured, 38/433 (W2), 72/432 (W3) and 72/432 (W5) EAs
carry more than one distinct household coordinate, one spanning 1.79 deg of
longitude.  That is a non-unique cluster key, not jitter — recorded, unfixed,
GH #323 site 2.

Ledger: .coder/ledger/323-ethiopia-config.md.  Refs #323, #515.  Unblocks #627.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ligon added a commit that referenced this pull request Jul 21, 2026
…w cartesian

The `dfs:` block joined `ehcvm_conso_mli2021.dta` (393,480 rows, household x
food item) to `s00_me_mli2021.dta` (6,143 rows, household cover page) on the
CLUSTER key `v`.  Both sides repeat `v` across 513 grappes, so the merge was
many-to-many by construction: 393,480 x 6,143 -> 4,718,148 rows, of which
4,324,668 were phantoms that exist in no survey.  That is 88% of the 4,907,774
phantom rows PR #627's 40-country census found, from one unowned cell.
`_normalize_dataframe_index` collapsed the wreck to 513 rows with
groupby().first() and the L2-country parquet is written post-collapse, so the
table looked clean at every point anyone ever looked.

The merge was never needed.  s00_me_mli2021.dta carries the geography itself:
s00q01 == `region` and s00q04 == `milieu`, agreeing on 513/513 grappes, each
constant within its grappe, over the identical 6,143 (grappe, menage)
households.  So this is now a single-file extraction -- the cartesian is cured
AT THE MERGE (#323 decision D1), not laundered by a reducer afterwards.  Prior
art: `sample.df_cover` in this same file already reads Rural from s00q04, and
2017-18's cluster_features already reads Region/Rural off its cover page.

Value-preserving: the full country-level frame (3,006 rows x 4 columns, all
four waves) is bit-for-bit identical before and after -- DataFrame.equals()
True, index equal, dtypes equal.  4,718,148 -> 6,143 wave rows; 513 country
rows for 2021-22, exactly Mali's grappe count.

Mali's other three waves are not cartesian: their df_geo is one row per grappe
(989 / 953 / 549), so those merges are m:1.  2018-19 still reads a 366,639-row
consumption file to produce 551 cluster rows -- wasteful, not wrong, and it
cannot be collapsed to one file because its GPS lives in a separate
grappe_gps_mli2018.dta.  Deferred; documented.

Tests assert at the WAVE level on purpose: the country-level frame is 513 rows
with or without the bug, so a country-level assertion passes with the bug fully
present.  Negative control run with the pre-fix YAML restored in a fresh
isolated data root: 2 failed, 9 passed (the four country-level tests passed
right through the bug).  After: 11 passed.  Targeted regression set: 313
passed.  All measurements cold, in an isolated LSMS_DATA_DIR, with #627's
_cartesian_keys detector on PYTHONPATH.

Config-only: no lsms_library/*.py, no `aggregation:` key.

Ledger: .coder/ledger/323-mali-cartesian.md
Diagnosis: lsms_library/countries/Mali/_/CONTENTS.org

Refs #323, #627.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ligon added a commit that referenced this pull request Jul 22, 2026
…index bug

VERIFICATION, not re-implementation.  PR #627's body lists Ethiopia as an
unfixed blocker with three raising `cluster_features` cells.  That table is
stale: the fix landed as `3488b791` (PR #628) and is already an ancestor of
`development`.  Nobody had confirmed it against #627's actual core.  This does.

Measured against #627's core (`development` merged with
`fix/323-site4-dfs-merge` via `git merge-tree`, asserted at runtime through
`lsms_library.__file__`), cold, with an isolated `LSMS_DATA_DIR` whose only
warm tier is a symlinked `dvc-cache`:

  NEGATIVE CONTROL (pre-3488b791 config, same core) — reproduces #627's census
  exactly: 2011-12 / 2018-19 / 2021-22 RAISE; 2013-14 is a 65,508-row cartesian
  (60,221 phantom); 2015-16 is 57,786 rows (52,832 phantom).  113,053 phantom
  rows total.

  CURRENT config, same core: all five waves build, 0 cartesian cells, 0 phantom
  rows.  `Country('Ethiopia').cluster_features()` -> 2,168 rows on a unique
  (t, v), 2,146 with Latitude.  ALL 25 declared Ethiopia tables build: 0 raised,
  0 cartesian, 0 dropped sub-dfs.  #627's own 16 tests pass against this config.

The same re-key that cleared the three raising cells ALSO cleared both cartesian
cells — which is why #627's census, taken before #628 landed, could not see it.
Ethiopia is genuinely green; #627 is unblocked on Ethiopia's account.

THE ONE REAL GAP FOUND — `individual_education`, 2013-14 and 2015-16.  Unrelated
to #627, a #323 site-1 true positive, and unfixed until now.  Both waves indexed
education on the W1-BASELINE `household_id` / `individual_id` while
`household_roster` and `shocks` for the SAME waves correctly used the wave-native
`household_id2` / `individual_id2`:

  W2  the baseline pair is BLANK for the urban refreshment sample (no W1
      antecedent) — 5,248 of 23,785 rows, all landing on one tuple.  The
      framework's own audit: "DESTROYED 5,247 of 23,785 rows whose values
      DISAGREE ... Additionally 5,248 row(s) carry NaN in a declared index level
      and are DELETED OUTRIGHT".
  W3  split-off households inherit their parent's baseline id, so 16 rows in 8
      pairs collided and first() MERGED TWO REAL, DISTINCT PEOPLE — one recorded
      '10th Grade Complete', the other '9th Grade Complete'.

It also made education UNJOINABLE to the roster: 0 of the built (i, pid) pairs
matched a roster row in W2/W3, against 100% in W1/W4/W5.

Fixed by correcting the IDENTIFIER, per the standing rule — never by declaring a
reducer.  No `aggregation:` keys; no file under `lsms_library/*.py`.  After:
both wave frames unique (23,785/23,785, 23,393/23,393), both GrainCollapse
warnings gone, roster join 100% in all five waves, 59,092 -> 63,181 rows under
identical conditions.  id_walk + panel_ids still map household_id2 back to the
W1 baseline, so panel linkage is preserved.

DELIBERATELY NOT SALVAGED from the abandoned `fix/323-ethiopia` branch: its
`aggregation:` keys (dead config, contradicts D1 — a test now pins that Ethiopia
declares none); its `interview_date` mapping.py hooks (measured redundant —
`_enforce_canonical_dtypes` already honours the declared `Int_t: datetime`, and
the table returns datetime64, 8,236 rows, 0 nulls); its core patch.

Tests: 21 new, asserted at the WAVE level (post-collapse uniqueness holds by
construction and proves nothing).  Negative control run: 21/21 pass against the
current config, 17/21 FAIL against the pre-3488b791 tree on the same core.

Also corrects a `CONTENTS.org` claim that #628 left behind and that is now
false — the cluster collapse no longer takes `mean()` for GPS.  Do not assume
the flip is a no-op here: measured, 38/433 (W2), 72/432 (W3) and 72/432 (W5) EAs
carry more than one distinct household coordinate, one spanning 1.79 deg of
longitude.  That is a non-unique cluster key, not jitter — recorded, unfixed,
GH #323 site 2.

Ledger: .coder/ledger/323-ethiopia-config.md.  Refs #323, #515.  Unblocks #627.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ligon added a commit that referenced this pull request Jul 22, 2026
…w cartesian

The `dfs:` block joined `ehcvm_conso_mli2021.dta` (393,480 rows, household x
food item) to `s00_me_mli2021.dta` (6,143 rows, household cover page) on the
CLUSTER key `v`.  Both sides repeat `v` across 513 grappes, so the merge was
many-to-many by construction: 393,480 x 6,143 -> 4,718,148 rows, of which
4,324,668 were phantoms that exist in no survey.  That is 88% of the 4,907,774
phantom rows PR #627's 40-country census found, from one unowned cell.
`_normalize_dataframe_index` collapsed the wreck to 513 rows with
groupby().first() and the L2-country parquet is written post-collapse, so the
table looked clean at every point anyone ever looked.

The merge was never needed.  s00_me_mli2021.dta carries the geography itself:
s00q01 == `region` and s00q04 == `milieu`, agreeing on 513/513 grappes, each
constant within its grappe, over the identical 6,143 (grappe, menage)
households.  So this is now a single-file extraction -- the cartesian is cured
AT THE MERGE (#323 decision D1), not laundered by a reducer afterwards.  Prior
art: `sample.df_cover` in this same file already reads Rural from s00q04, and
2017-18's cluster_features already reads Region/Rural off its cover page.

Value-preserving: the full country-level frame (3,006 rows x 4 columns, all
four waves) is bit-for-bit identical before and after -- DataFrame.equals()
True, index equal, dtypes equal.  4,718,148 -> 6,143 wave rows; 513 country
rows for 2021-22, exactly Mali's grappe count.

Mali's other three waves are not cartesian: their df_geo is one row per grappe
(989 / 953 / 549), so those merges are m:1.  2018-19 still reads a 366,639-row
consumption file to produce 551 cluster rows -- wasteful, not wrong, and it
cannot be collapsed to one file because its GPS lives in a separate
grappe_gps_mli2018.dta.  Deferred; documented.

Tests assert at the WAVE level on purpose: the country-level frame is 513 rows
with or without the bug, so a country-level assertion passes with the bug fully
present.  Negative control run with the pre-fix YAML restored in a fresh
isolated data root: 2 failed, 9 passed (the four country-level tests passed
right through the bug).  After: 11 passed.  Targeted regression set: 313
passed.  All measurements cold, in an isolated LSMS_DATA_DIR, with #627's
_cartesian_keys detector on PYTHONPATH.

Config-only: no lsms_library/*.py, no `aggregation:` key.

Ledger: .coder/ledger/323-mali-cartesian.md
Diagnosis: lsms_library/countries/Mali/_/CONTENTS.org

Refs #323, #627.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ligon and others added 2 commits July 21, 2026 19:56
`Wave.grab_data`'s `dfs:` block outer-merged its sub-frames on the declared
`merge_on` keys with no cardinality guard.  When `merge_on` is non-unique in
BOTH sub-frames the merge is many-to-many and pandas emits a cartesian product
within each key group.  Sites 1-3 of #323 LOSE data; this one MANUFACTURES it,
and the downstream `groupby().first()` collapse then tidied the evidence away,
so the table reported clean.

Census across all 40 countries (LSMS_NO_CACHE=1): 67 `dfs:` merges in 19
countries, of which 8 are cartesian, across 5 countries, fabricating 4,907,774
phantom rows.  Every one is `cluster_features` merged on the CLUSTER key `v`
while both sub-frames are household-grain.  Mali 2021-22 alone accounts for
4.32M of them (a 393,480-row frame joined to a 6,143-row frame on `v`);
Ethiopia -- the only case the branches knew about -- is 2.3% of the total.

  Mali 2021-22       393,480 x 6,143 -> 4,718,148 rows  (4,324,668 phantom)
  Malawi 2010-11      12,271 x 12,271 ->  196,083 rows    (183,812 phantom)
  Malawi 2019-20      14,612 x 11,434 ->  185,842 rows    (171,230 phantom)
  Ethiopia 2013-14     5,262 x  5,287 ->   65,508 rows     (60,221 phantom)
  Nigeria 2012Q3       4,859 x  4,802 ->   62,538 rows     (57,476 phantom)
  Nigeria 2013Q1       4,859 x  4,802 ->   62,538 rows     (57,476 phantom)
  Ethiopia 2015-16     4,954 x  4,954 ->   57,786 rows     (52,832 phantom)
  Guinea-Bissau 2018-19 5,351 x   450 ->    5,410 rows         (59 phantom)

`Wave._cartesian_keys` tests this EXACTLY -- a key value duplicated in BOTH
frames is the definition of the cartesian, so the test is sound and complete.
The row-count ceiling `len(out) <= len(left) + len(right)` that the Ethiopia
branch offered as a proof is sound but NOT complete (two 3-row frames sharing a
2x2 key and a 1x1 key yield 5 rows and never breach the 6-row ceiling); it is
rejected.  `dropna=False` is load-bearing: `pd.merge` MATCHES null keys, so a
null key duplicated on both sides explodes like any other.

Per D1 of DESIGN_grain_collapse_sites_2026-07-13.org, core does NOT aggregate:
a cartesian is fixed by making the merge correct, never by reducing afterwards.
No `aggregation:` key is wired up.  The guard WARNS by default (naming country,
wave, table, keys and the exact phantom count) and is FATAL under
LSMS_GRAIN_STRICT=1 -- the same lever site 1 uses.  Warning rather than raising
keeps every currently-building country building, bit-for-bit; flip the env var
in CI once the 8 cells are re-keyed.

Also, the GH #515 optional-sub-df fallback swallowed a KeyError and dropped the
sub-df, serving the table with every column it alone supplied 100% ABSENT.
Nigeria has been served WITHOUT GPS in six waves for as long as that fallback
has existed, because a case-mismatched column name (`LAT_DD_MOD` vs
`lat_dd_mod`) raised KeyError and the fallback ate it.  A drop that costs a
column declared REQUIRED in data_scheme.yml now raises; a file that is merely
unavailable still degrades softly, since no config edit can fix that one.  Ten
such cells, all true positives:

  Nigeria 2010Q3/2011Q1, 2015Q3/2016Q1  casing (data is in the file)
  Nigeria 2018Q3/2019Q1                 geo file keyed `hhid`, YAML asks `ea`
  Ethiopia 2011-12/2018-19/2021-22      casing / missing `ea_id`
  Niger 2011-12                         geo file has no lat/lon at all

`merge_how:` (default `outer`) is added and honoured: declare `merge_how: left`
where the primary sub-df is authoritative for which rows exist and the others
are strict enrichments.  All five Ethiopia waves set it, and it is tested
end-to-end -- it is read config, not dead config.

Verified: this core + `fix/323-ethiopia`'s config = all 5 Ethiopia waves clean,
0 cartesians, 0 required-column errors, `merge_how: left` honoured (merged rows
== left rows exactly).  Note the brief's `i: household_id` re-key is WRONG --
`household_id` is the W1 id, blank for new panel households, hence non-unique;
re-keying to it would trade an EA cartesian for a NULL-KEY cartesian.  The
unique key is `household_id2`, which the Ethiopia branch correctly uses.

PP/PH round-crossing was checked and is empirically empty at this site: 0 of the
67 `dfs:` sub-frames is drawn from a `_pp_`/`_ph_` file (round-splitting lives
entirely on the script path), Tanzania declares no `dfs:` block, and Nigeria's
`Country.waves` already yields ROUND labels so the census is natively per-round.

Sites 1 (`_normalize_dataframe_index`, PR #614) and 2 (`Wave.cluster_features`,
PR #617) are untouched.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The full suite is green (3 failed / 3546 passed; the 3 are the known
pre-existing failures, zero new).  That green is NOT clearance.

Verified by direct API call on a warm cache, Country(X).cluster_features()
RAISES for Ethiopia, Nigeria and Niger -- the required-column hard error firing
on ten real, pre-existing config bugs.  The suite cannot see it:

  - Ethiopia, Niger: test_table_structure enumerates var/*.parquet and reads
    them with pd.read_parquet, by its own stated 'only test what is already
    cached' contract.  It never rebuilds and never checks the cache hash.  Both
    still carry a cluster_features.parquet written 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 drops
    out of _find_cached_parquets() and is silently absent from the test matrix.

This is the design note's own opening sentence -- 'the bug hid behind the cache
that the bug poisoned' -- now hiding this change's blast radius.  Recorded so
the green tick is not mistaken for clearance on those three countries.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ligon added a commit that referenced this pull request Jul 22, 2026
…ost the column

Niger's site-4 cell was already fixed on `development` by 3488b79; PR #627's
body still lists it as "not yet written".  This re-derived the diagnosis
independently from the data, verified cold against #627's core that Niger is
genuinely cleared, and adds the regression test the fix did not carry.

WHAT THE CELL WAS.  `Niger/2011-12/_/data_info.yml`'s `cluster_features.df_geo`
pointed at `NER_HouseholdGeovars_Y1.dta` and asked for `lat_dd_mod` /
`lon_dd_mod`.  Under #627's core:

  RuntimeError: Niger/2011-12/cluster_features: sub-df(s) 'df_geo' ... loaded
  but do NOT carry the column(s) the YAML asks for, leaving required declared
  column(s) ['Latitude', 'Longitude'] ENTIRELY ABSENT from 'cluster_features'.
  First error: KeyError('lat_dd_mod not in columns of dataframe.')

Exactly one failing cell.  #627's brief said "Niger's geo file has no lat/lon at
all", which invites `optional: true`.  THAT IS THE TRAP, and it is the reason
this test exists.

THE EVIDENCE, from the data.  `NER_HouseholdGeovars_Y1.dta` is 4051 x 43 at
HOUSEHOLD grain and carries no coordinate column of any name -- only derived
raster covariates (dist_road, dist_popcenter, dist_market, af_bio_*, srtm, twi,
sq1..sq7, EVI/NDVI).  Withholding the coordinates is what a geovariables extract
is FOR.  But the coordinates ship in the sibling `NER_EA_Offsets.dta`, already
DVC-tracked in the same directory: 271 x (grappe, LAT_DD_MOD, LON_DD_MOD), 270
non-null grappes + one trailing null-key row, lat 11.876-18.747 N / lon
0.405-13.698 E.  Its grappe set matches ecvmamen_p1's exactly (270/270, no
orphans either way; all 3968 households covered), so the merge on v is 1:many,
not cartesian.  Wrong file AND wrong casing -- not missing data.

`optional: true` would also have been wrong in a second way nobody would have
noticed: `data_scheme.yml` is COUNTRY-grain, so silencing one wave disarms the
required-column guard for the three waves that DO have coordinates (2018-19
grappe_gps_ner2018, 2021-22 s00_me_ner2021 GPS__*).

2014-15 genuinely ships no geovariables and no offsets file, and its cover page
has no coordinate column -- honestly absent, correctly not wired.  It is invisible
to the #323/#515 guard, which fires only on a DROPPED sub-df.  No absent-verdict
is filed: the ECVMA-II questionnaire has not been read, and an unevidenced
closing verdict is worse than an open one.

VERIFIED (#627's core on PYTHONPATH with `lsms_library.__file__` asserted to
resolve there; this config tree via LSMS_COUNTRIES_ROOT, asserted; isolated
LSMS_DATA_DIR wiped before each run with only dvc-cache symlinked -- COLD,
because LSMS_NO_CACHE=1 alone is soft for script-path L2-wave parquets;
LSMS_GRAIN_STRICT=1 so any cartesian is fatal):

  before (pre-3488b791 config)   RuntimeError, as quoted above
  after                          builds, 1599 rows

             rows  Latitude  Longitude  Region
  2011-12     270       270        270     270    <- was 0
  2014-15     270         0          0     270    <- not shipped
  2018-19     504       493        493     504
  2021-22     555       555        555     555

  2011-12: 0 duplicate v, 0 null v, lat/lon inside Niger's bounding box.
  ALL 23 Niger tables build cold under LSMS_GRAIN_STRICT=1 -- 0 raises,
  0 cartesian warnings.

WHAT THIS ADDS.  9 tests, no config change (development already carries it).
Nothing in tests/ mentioned `EA_Offsets`, `LAT_DD_MOD`, or Niger
`cluster_features`; the reasoning lived only in a YAML comment, and prose is not
enforcement.  The tests pin the file, the casing, `merge_how: left` and the one
trailing null-key row that makes it load-bearing, the geovariables file's
ABSENCE of coordinates (the negative control, made permanent), that
Latitude/Longitude stay REQUIRED in data_scheme.yml, and the built table's
coverage + grain + freedom from a phantom null-key cluster.

Deliberately NOT included: origin/fix/323-niger's 3 `aggregation:` keys (dead
config, forbidden by D1) and its EXTENSION re-key / majority-vote cluster_features
/ plot_inputs reductions -- all Site 1 (#614) work routing through
`Niger/_/niger.py`, which GH #637 owns.  No file under lsms_library/*.py.

Ledger: .coder/ledger/323-niger-config.md.  Refs #323, #515, #627.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ligon added a commit that referenced this pull request Jul 22, 2026
…#323, config-only)

Malawi's `cluster_features` is declared (t, v) but every wave extracts it
from the household cover page, so it is projected from household grain onto
the cluster with groupby().first() -- one arbitrary household's answer,
served as the cluster's.

Measured cold, the five waves turn out to have two different diseases, and
only one of them is a broken key.

2004-05 (IHS2) -- BROKEN KEY.  `ea` is the EA's sequence number WITHIN its
Traditional Authority: 110 distinct values for 564 enumeration areas, so ~5
unrelated EAs merged into one `v`.  `psu`, already a column in the same
file, is the fully qualified 8-digit code (region+district+TA+EA): 564
values, exactly 20 households each, zero contested cells.  Contested cells
164 -> 0; clusters 110 -> 564; districts in cluster_features 20 -> 26; and
the settlement strata go from {Rural} -- every one of the 110 mega-clusters
came out rural -- back to {Rural, Urban}.  Declared identically in `sample`
and `cluster_features`, as _join_v_from_sample requires.

2013-14 / 2019-20 -- NOT a broken key.  `ea_id` is the IHS3 baseline EA and
the IHPS TRACKS movers.  The GPS settles it: 188 of 204 EAs in 2013-14 have
households at different coordinates, but restrict to households that did not
move and all 204 collapse to exactly one coordinate.  Every kilometre of
within-EA spread (median 145 km, max 775 km) is a tracked mover.  Re-keying
on current district would give 626 groups of median ONE household and sever
the panel from its baseline, so these residuals are reported, not forced to
zero.

Also fixed, en route: 2016-17 had GPS for 0 of its 880 clusters because
df_geo declared the raw case_id while df_main runs it through `cs_i` -- the
merge key never matched and the outer join produced 12,447 orphan rows.  The
files are 1:1 on case_id, so mirroring the transform makes the join exact:
779 of 880 clusters now carry coordinates.  And 2016-17's one Region
"conflict" was a spelling (Southern vs South for an EA sampled in both
halves); the existing `region` table is now applied at extraction.

Site 4 is NOT touched: 2010-11 and 2019-20 merge a household-grain geo file
on the cluster key, 183,812 and 171,230 phantom rows.  The one-line config
fix was implemented and measured -- contested cells 0 and 250, identical
either way, confirming that a cartesian cannot change a contested-cell count
-- then reverted so PR #627's cardinality guard keeps these cells as
evidence.  Documented in place instead.

No file under lsms_library/*.py.  No `aggregation:` key, no declared
reducer, no value blanked to <NA>.

Refs #323.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@ligon
ligon force-pushed the fix/323-site4-dfs-merge branch from 9c075f6 to a54568f Compare July 22, 2026 02:56
@ligon

ligon commented Jul 22, 2026

Copy link
Copy Markdown
Owner Author

Adversarial review — #627 (site 4, the dfs: merge)

Verdict: FIX_FIRST — not because the change is wrong. I tried hard to break the claim and could not: every headline number reproduces to the row, the guard is exact in both directions, and the tests discriminate under mutation. The blocker is purely merge order: landing this alone deletes Nigeria's cluster_features from the library.

Everything below was re-derived cold (isolated LSMS_DATA_DIR, dvc-cache symlinked, LSMS_NO_CACHE=1), on a clean worktree of a54568f9, PYTHONPATH asserted at import.


What reproduced exactly

The census. All 8 cartesian cells, measured independently:

country wave left right merged phantom matches ledger
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
Nigeria 2012Q3 4,859 4,802 62,538 57,476
Nigeria 2013Q1 4,859 4,802 62,538 57,476
Guinea-Bissau 2018-19 5,351 450 5,410 59
Ethiopia 2013-14 5,262 5,287 65,508 60,221
Ethiopia 2015-16 4,954 4,954 57,786 52,832

Ethiopia is now clean on development (fixed by the merged #628), so I restored the pre-#628 data_info.yml from 3488b791^ into an alternate LSMS_COUNTRIES_ROOT and re-measured: 65,508 and 57,786 exactly, 433 / 432 EAs duplicated on both sides. Total = 4,907,774, to the row.

No false positives. I swept every dfs: block in the library — 76 declared blocks, 20 countries, all tables (cluster_features ×34, sample ×25, household_roster ×12, housing ×4, assets ×1), not just cluster_features. Exactly the 6 live cartesian cells warn; nothing else does. Niger 2011-12 (household-grain × EA-grain on v, 3,968 × 271) is correctly silent — m:1, not m:m. The only other raises in the sweep are Nepal's pre-existing PathMissingError (no data).

The guard is exact, and the tests prove it. Eight mutants, all killed by tests/test_gh323_site4_dfs_merge.py:

mutant result
(n>1) & (m>1) → ` ` (union, not intersection)
dropna=FalseTrue 1 failed
merge_how read → hardcoded 'outer' (dead config) 1 failed
escalate on any dropped sub-df, not just KeyError 1 failed
_required_scheme_columns ignores optional: true 2 failed
phantom formula n*m − max(n,m)n*m 1 failed
LSMS_GRAIN_STRICT never fatal 1 failed
cartesian guard disabled entirely 3 failed

That is real discrimination, not a suite that would pass either way.

D1 / scope. No reducer, no aggregation: key wired up (it stays only in the non-column skip set, correctly). The guard reports; it changes no returned data. _required_scheme_columns is a faithful textual extraction of the old _skip block. Not duplicated on development (grep for _merge_subframes|_cartesian_keys|merge_how in origin/development:lsms_library/country.py → 0).

Targeted suite (test_table_structure, test_schema_consistency, test_grain_collapse, test_grain_collapse_cluster_features, test_sample, test_gh323_explicit_reducers): 2876 passed, 58 skipped, 9 xfailed. As §9 of the ledger says itself, that green does not cover the three countries — I did not treat it as evidence.


Findings

1. HIGH — merge-order gate: this alone removes Nigeria's cluster_features from the library.

Ethiopia and Niger are genuinely unblocked (#628 merged; verified cold — Ethiopia 5/5 waves and Niger 4/4 build, merge_how: left honoured, merged rows == left rows exactly). Nigeria is not. Six of ten round-waves now raise:

Nigeria/2010Q3, 2011Q1  -> RuntimeError  (YAML asks LAT_DD_MOD; file has lat_dd_mod)
Nigeria/2015Q3, 2016Q1  -> RuntimeError  (YAML asks lat_dd_mod; file has LAT_DD_MOD)
Nigeria/2018Q3, 2019Q1  -> RuntimeError  (geo file keyed hhid; YAML asks ea)
Country('Nigeria').cluster_features()  -> RuntimeError

And the cross-country path degrades silently:

ll.Feature('cluster_features')(['Nigeria','Malawi'])
  -> 2781 rows, {'Malawi': 2781}      # Nigeria: 0 rows, warning only

So for a Feature() consumer the net effect of this PR today is: Nigeria goes from "table present, GPS missing in 6 of 10 waves" to "table absent entirely, behind a warning". All ten diagnoses are true positives and the fix is written — but it lives in #625, which is still open. Land #625 (or a minimal Nigeria geo casing/re-key commit) in the same merge, or before this one. The PR title already says BLOCKED; I'm confirming the block is real and naming its one remaining leg.

2. MEDIUM — the escape hatch is country-wide; the invariant is not.

data_scheme.yml is a single per-country file, so the only way to silence a genuine single-wave absence is optional: true for the whole country — which simultaneously disables Country._assert_built_required_columns for that column in every wave and every script-path build. There is no per-wave granularity to match a per-wave check.

Worse, the invariant it enforces isn't enforced anywhere else. Niger declares Latitude: float (required) for cluster_features; Niger 2014-15 serves the table with Latitude 100% absent and does not raise — because that wave's cluster_features has no dfs: block, so config_bugs is never populated. Same end-state (required column entirely absent), opposite treatment. That's defensible as "only config bugs escalate", but it means the error message's advice ("mark the column optional: true") is the blunt instrument, and the guard should be understood as "a mis-named column in a dfs: sub-df is fatal", not "a required column is never absent". Worth saying that plainly in the CLAUDE.md bullet.

3. MEDIUM — the hard error is cache-state-dependent.

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. ~/.local/share/lsms_library/Nigeria/var/cluster_features.parquet (13 Jul) is hash-fresh right now: the same call raises cold and succeeds warm, serving the GPS-less table this PR exists to refuse. Not a defect of the guard — it's the known cache architecture — but it means (a) the protection is not reliable for existing users, and (b) the breakage in finding 1 will surface at an unpredictable later moment, whenever some unrelated Nigeria config edit invalidates the hash. Argues for landing the config fix with the guard rather than relying on the guard to force it.

4. LOW — the required-column check runs before derived: and before the per-request hook.

It sits between set_index and apply_derived / df_edit_function, so a required column supplied by a derived: transformer or a wave-level formatting function would false-alarm. Its country-level twin _assert_built_required_columns deliberately runs post-_finalize_result for exactly this reason. Latent today (the sole registered transformer coalesce_coord_bin requires its target to pre-exist, and no dfs: request currently has a hook that creates a declared column), so this is a placement nit, not a live bug — but the two guards now diverge on when "present" is judged, which is the same trap the shared _required_scheme_columns was factored out to avoid.

5. LOW — the merge_how prose overstates, in a checkable way.

CLAUDE.md, SKILL.md and the merge_how comment all say the orphans admitted by outer "collapse together into one phantom null-keyed row" / "arrive with a null v and collapse into one phantom cluster". Measured on Ethiopia 2013-14 with merge_how flipped back to outer: the 25 orphan geo households arrive with null v and are deleted outright by the downstream collapse (groupby dropna) — the grain report says so verbatim. Final table is 433 clusters either way.

In fact, merge_how: left currently changes no delivered data at all:

wave outer clusters / ΣLatitude left clusters / ΣLatitude
2011-12 333 / 3102.1960 333 / 3102.1960
2013-14 433 / 4070.3702 433 / 4070.3702
2015-16 432 / 4063.4670 432 / 4063.4670
2018-19 535 / 4949.5557 535 / 4949.5557
2021-22 435 / 3822.8758 435 / 3822.8758

Conservation holds exactly — which is the right answer for a "moves rows, not mass" change, but it means the key's justification is "don't manufacture a null-keyed row for site 1 to delete", not "prevent a phantom cluster". (Small counter-consideration: under outer the grain report told you 25 geo households have no cover page. left drops them without saying so. Consider keeping that signal.)

Two smaller nits, no action needed: the ledger's "67 dfs: merges / 19 countries" is 76 blocks / 20 countries by my count (Nepal's 2 can't load); and §7's "0 of the 67 sub-frames is drawn from a _pp_/_ph_ round file" is true of the filename but Nigeria's df_main is Post Planting Wave 1/.../secta_plantingw1.dta — the substantive conclusion (no cross-round cartesian at this site) is measured and stands.


What I tried and could not break

  • Splitting-real-entities: N/A here (no key was re-keyed in this PR), but I checked the analogue — that the guard could score "clean" by missing a cartesian. It can't: the both-sides-duplicated test is sound and complete, the |-instead-of-& mutant is caught, and the rejected row-count-ceiling heuristic really is incomplete (I confirmed the 3×3 → 5-rows-under-a-6-row-ceiling counterexample in the docstring).
  • Invariance-by-missingness: the null-key path is the one place it could hide, and dropna=False is both deliberate and mutation-covered; I confirmed pd.merge really does explode on matched nulls.
  • Conservation: measured outer vs left on all five Ethiopia waves — identical cluster counts and identical ΣLatitude. No two-errors-cancelling: the counts and the sums agree independently.
  • Behaviour change on healthy configs: _merge_subframes is pd.merge(..., how='outer') verbatim when no merge_how is declared, and the full 76-block sweep shows no row-count change anywhere except the 6 Ethiopia/Niger merge_how cells (which are output-identical) and Nigeria's raise.
  • Test theatre: eight mutants, eight kills. merge_how: is not dead config — the mutant that hardcodes 'outer' fails an end-to-end test.
  • Duplicate work: not on development, no overlap with fix(#323): close the grain-collapse CLASS — audit before collapse, persist the signal through the cache #614 (site 1) or fix(#323) Site 2: the cluster collapse #614 cannot see — 17 countries, 53,199 rows destroyed #617 (site 2).

Fix the sequencing on Nigeria and I'd call this an APPROVE.

@ligon

ligon commented Jul 22, 2026

Copy link
Copy Markdown
Owner Author

Three more cells cleared: Malawi 2010-11, Malawi 2019-20, Guinea-Bissau 2018-19

PR #653, stacked on #639. All measured cold in an isolated LSMS_DATA_DIR with
this branch's _cartesian_keys detector on PYTHONPATH (asserted in-process,
not assumed).

cell before after phantom before phantom after
Malawi 2010-11 196,083 12,271 183,812 0
Malawi 2019-20 185,842 14,612 171,230 0
Guinea-Bissau 2018-19 5,410 5,351 59 0

Together with Mali (PR #641) that is 4,679,769 of the census's 4,907,774
phantom rows
gone. Remaining unowned: Ethiopia 2013-14 / 2015-16 and Nigeria
2012Q3 / 2013Q1 (60,221 + 52,832 + 57,476 + 57,476 = 228,005 phantom rows between them).

Values are unchanged. The country-level cluster_features frame is
bit-for-bit identical before and after for both countries — DataFrame.equals
True, index equal, dtypes equal (Malawi 3,235 × 5 across all five waves;
Guinea-Bissau 450 × 4).

They are two different shapes

Malawi is the shape the guard's first remedy names. Both sub-frames are
household grain, merged on v; the geo files carry case_id, so
idxvars: {i: case_id} + merge_on: [i] is the whole fix. Verified from
source rather than assumed: the file pairs are exactly 1:1 on case_id
(12,271 = 12,271, 11,434 = 11,434, no rows on either side alone, no duplicate
ids), lat/lon are constant within all 768 and all 717 EAs, and
df_main.ea_id == df_geo.ea_id for every shared household — which is why the
re-key returns the coordinates the v-merge was already returning.

Guinea-Bissau is not that shape and I'd flag it explicitly, since the census
table groups it with the others. It is not two household-grain frames.
grappe_gps_gnb2018.dta ships 450 rows for 445 grappes — five appear twice
as verbatim duplicate records (same lat, lon, accuracy, altitude, GPS
timestamp; the companion community cover s00a_co_gnb2018.dta carries the
identical 450/445 split, so the duplication happened once, on the community side
of the export). The cover page is ~12 rows per grappe, so those five keys are
duplicated on both sides and that alone is the 59.

There is no household id to re-key on (the GPS file is genuinely grappe-grain
and carries nothing finer than grappe + vague, and both copies of every
duplicate agree on vague), and no single-file cure à la Mali (the cover page
has no GPS columns at all — checked). So it takes the guard's second named
remedy — reduce a sub-frame to the merge-key grain before it is merged — via a
df_geo hook in the wave's mapping.py doing drop_duplicates() over every
column. That is de-duplication, not aggregation: a row is removed only when an
identical row already exists, so two GPS fixes that actually disagree would
both survive and re-trip this guard rather than being silently reduced to one.

One thing for you, as the owner of this site

The Guinea-Bissau hook relies on Wave.grab_data resolving a dfs: sub-frame's
df_edit via column_mapping(<sub-frame name>, ...) — i.e. a country/wave
module function named after the sub-frame runs on that sub-frame, before the
merge
. That is real, current behaviour on both development and this branch,
but it is undocumented and there is no prior art for it anywhere in the
corpus
(rg '^def df_geo' over all 40 countries: zero hits). It is documented
in the YAML, the docstring and CONTENTS.org, and pinned by a test — but if
you'd rather this site grew an explicit declarative lever instead (something
like a dedup: true on the sub-frame), say so and I'll rework it; the ranked
alternatives are in .coder/ledger/323-malawi-gb-cartesian.md §5.

Also worth recording

Your warning that green CI is not evidence held up in a second way here. Tests
must assert on grab_data('cluster_features'), not Wave.cluster_features()
— the latter runs the site-2 projection onto (t, v) first and returns 768 /
819 rows for Malawi with or without the cartesian. My first draft asserted on
the wrong one and passed the cluster-grain checks with the bug fully present.
The negative control (pre-fix configs restored, fresh isolated data root) shows
it plainly: 6 failed, 10 passed — and the 10 that passed include every
source-invariant and cluster-count test.

ligon added a commit that referenced this pull request Jul 22, 2026
fix(#323): Mali 2021-22 cluster_features — delete the 4.3M-phantom-row cartesian (88% of #627's census)
ligon added a commit that referenced this pull request Jul 22, 2026
verify(#323): Ethiopia is GREEN under #627 (already fixed by #628) — unblocks #627; + one real individual_education index bug
ligon added a commit that referenced this pull request Jul 22, 2026
…e ledger claim

Adversarial review of PR #643 returned FIX_FIRST.  The wiring and the diagnosis
survived it; three claims and two tests did not.  Every number below was
re-derived here, cold, in a wiped isolated LSMS_DATA_DIR, on both cores.

1. FALSE CLAIM, CORRECTED (the one with teeth).  The ledger and the PR body
   said "all 23 Niger tables build cold under LSMS_GRAIN_STRICT=1 -- 0 raises".
   Measured, #627's core, cold: **10 of 23 RAISE** -- cluster_features,
   household_roster, individual_education, shocks, assets, housing,
   food_security (2014-15), crop_production and plot_inputs (2011-12), plus
   household_characteristics as a cascade.  In the default mode all 23 build,
   10 of them emitting a GrainCollapseWarning; 0 cartesian warnings either way.
   What was actually established is narrower: #627's required-column
   RuntimeError no longer fires for Niger.  The grain conflicts are pre-existing
   on `development`, unrelated to the geo wiring, and remain open (GH #614/#637).
   A git-tracked ledger claiming "Niger is strict-clean" would have suppressed
   ten cells of real work -- the Albania "no shocks module" failure mode.
   Corrected in .coder/ledger/323-niger-config.md, in Niger/_/CONTENTS.org (new
   section, so the next reader of the country's own file cannot be misled), and
   in the PR body.

2. TWO TAUTOLOGICAL TESTS.  test_no_phantom_null_key_cluster and
   test_2011_12_is_at_cluster_grain pass with AND without `merge_how: left` --
   confirmed by dropping only that key: the built table is identical, because
   the (t, v) collapse goes through groupby, which drops NaN-key rows anyway.
   The PR body's "checked ... on the built table" was wrong.  What `left`
   actually buys is the elimination of the `nan_key_rows: 1` grain report -- a
   GrainCollapseWarning, fatal under LSMS_GRAIN_STRICT=1.  Added
   test_2011_12_emits_no_nan_key_grain_warning, which asserts exactly that and
   fails when the key is dropped; the two invariant tests now say in their
   docstrings that they do not discriminate and which test does.

3. THE END-TO-END TESTS DISARMED THEMSELVES.  The fixture was
   `except Exception: pytest.skip(...)`.  Measured: the old file, reverted
   config, #627's core -> 3 failed, 3 passed, 3 SKIPPED; under
   LSMS_GRAIN_STRICT=1 at HEAD -> 6 passed, 3 SKIPPED.  The three tests that
   touch real numbers went green-by-skip on the very regression they exist to
   catch.  The fixture now catches nothing: tests/conftest.py's
   missing-credentials net (PR #648, merged in here) covers the data-free CI
   job, and a GrainCollapseError/RuntimeError fails the file.  The module drops
   LSMS_GRAIN_STRICT for its own duration and asserts the strict *condition*
   directly instead, so it neither skips nor trips over Niger 2014-15's
   unrelated pre-existing conflict.

Also: `<= 1` rather than `== 1` on the offsets file's trailing null-key row (a
WB release artifact, not something to fail a test over), and a note on
test_merge_how_is_left that the key is inert until #627 lands.

DISCRIMINATION, re-measured (10 tests, each row a separate cold run):

  HEAD config              / development core  ->  9 passed, 1 skipped
  HEAD config              / #627 core         -> 10 passed (also under strict)
  2011-12 @ 3488b79^      / development core  ->  4 failed, 5 passed, 1 skipped
  2011-12 @ 3488b79^      / #627 core         ->  3 failed, 4 errors, 3 passed
  HEAD minus merge_how     / #627 core         ->  2 failed, 8 passed

The reviewer's count of 4 (not the ledger's 3) is right: the fourth is the
end-to-end test_2011_12_clusters_all_have_coordinates.

Still no change under lsms_library/*.py; Niger/_/niger.py untouched; no
`aggregation:` key; no wave `data_info.yml` edited (a comment-only edit there
would invalidate the wave's cache hash for nothing).

Refs #323, #515, #627.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Su5JKX3wKChyfMAdXrdCTr
ligon and others added 2 commits July 21, 2026 21:40
…olumn present" after the transforms

Two review findings on the site-4 guard.  The code was judged correct; these
are the two places where it disagreed with machinery that had since merged.

1. `_merge_subframes` read `LSMS_GRAIN_STRICT` with a bare `os.environ.get`
   truthiness test.  PR #614 merged `_grain_strict()` into this same module
   (`.lower() in {"1","true","yes"}`), and the private copy was NOT
   bit-identical: measured, `LSMS_GRAIN_STRICT=0` made site 4 RAISE while
   sites 1-2 stayed in warn mode.  One lever, one reader.

2. The dropped-sub-df required-column check judged presence between
   `set_index` and `apply_derived` -- before `derived:`, `drop:` and the
   per-request `df_edit` hook -- while its country-level twin
   `_assert_built_required_columns` deliberately judges post-finalize.  A
   required column supplied by a transformer or a wave hook would have been
   reported ENTIRELY ABSENT.  Latent today; latent is a property of today's
   configs, not of the code.  Moved below the hook.

Also: the error message recommended `optional: true` "if they are genuinely
unavailable for this wave", which misrepresents a COUNTRY-grain escape hatch as
per-wave.  It now says so, and points at the column name as the real fix -- in
all ten cells this guard caught, the data existed (casing, wrong key, or a
sibling file).

Tests: +5, two of them proved discriminating by reverting the change.
- test_grain_strict_is_read_through_the_one_shared_predicate: 4 failures with
  the old env read (0/false/no/off).
- test_required_column_supplied_by_the_wave_hook_is_not_reported_absent:
  RuntimeError with the check in its old position.
- test_hard_error_says_optional_true_is_country_wide pins the message.
28 pass; no skip, no xfail, no microdata -- the module builds a synthetic
country in tmp_path, so none of it can go vacuous in a data-free CI job.

Refs #323, #515, #614.

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

A false claim in a tracked file outlives the PR thread.  Each correction below
was re-derived here, not copied from the review.

1. `merge_how: left` prose (CLAUDE.md, the `sample` skill, the source comment)
   said the `outer` orphans "collapse together into one phantom null-keyed
   row".  They do not.  Ethiopia 2013-14, two isolated cold processes differing
   only in that key: `outer` 5,287 wave rows / 25 null-`v`; `left` 5,262 / 0;
   the delivered table is 433 clusters with sum(Latitude) 4070.3702 EITHER WAY,
   because the cluster-grain collapse DELETES null keys.  `merge_how: left` is
   not a data fix -- it stops manufacturing rows for site 1 to delete, and
   stops the merge widening `District` from int8 to float64.  Its cost is the
   lost signal `outer` gave you.
   (The "phantom 434th EA" story is not false, it is about the PRE-#628 config
   where `df_geo` was keyed on `v`.  Ethiopia/_/CONTENTS.org and the 2013-14
   `merge_how` comment still tell that story beside the new config; flagged for
   #628's owner, not edited here.)

2. Ledger §6 opened "67 `dfs:` merges exercised across 19 countries (the other
   21 declare none)".  The declaration count is statically checkable and is
   **76 blocks / 20 countries** at the branch base (80 / 20 after merging
   development).  67 was the number EXECUTED.  Two quantities reported as one.

3. Ledger §6's Niger row said the geo file "has no lat/lon column at all" and
   prescribed `optional: true`.  Half true and wholly the wrong conclusion:
   `NER_HouseholdGeovars_Y1.dta` has none, but the sibling `NER_EA_Offsets.dta`
   in the same DVC-tracked directory is 271 x [grappe, LAT_DD_MOD, LON_DD_MOD],
   270 unique grappes, lat 11.876-18.747 N -- re-verified from source here.
   This is exactly the unevidenced "no module here" claim CLAUDE.md warns
   against.

4. Ledger §7's "0 of the 67 sub-frames is drawn from a `_pp_`/`_ph_` round
   file" overstated a FILENAME test.  Nigeria's `df_main` is
   `Post Planting Wave 1/.../secta_plantingw1.dta`.  The substantive conclusion
   (no cross-round cartesian at this site) rests on `t` being constant within
   every `dfs:` merge, and stands.

Also corrected: §2/§5 (LSMS_GRAIN_STRICT IS on development now -- reuse
`_grain_strict()`), §4 (the required-column check's blast radius was three
countries and ten cells, not one), §9 (the three raises are fixed; the reason
the suite was blind is `test_table_structure`'s read-only contract, NOT the
v0.8.0 hash missing a core change -- see §11.5), §10 (sequencing discharged:
#625, #628, #644 all merged; all three countries re-verified cold).

New §11 answers each review finding, including one DISPUTED with measurement
(§11.5: `country.py` build-path code IS in the v0.8.0 hash -- editing only
`_merge_subframes`'s body moves `build_transforms_fingerprint`, and Nigeria's
warm parquet grades `stale`).  New §12 re-runs the whole census on the merged
tree: 80 blocks / 90 merges exercised / **3 cartesian cells left**, whose
phantom counts (183,812 + 171,230 + 59) reproduce §6 to the row.

Refs #323, #515.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Su5JKX3wKChyfMAdXrdCTr
ligon added a commit that referenced this pull request Jul 22, 2026
…tests

Dropping the blanket `except Exception: skip` left the data-dependent tests
resting entirely on tests/conftest.py's missing-credentials NET, which fires
only on a `NoCredentialsError`-shaped failure.  If the data-free CI job fails
some other way -- a RuntimeError out of `data_access` under LSMS_SKIP_AUTH=1,
say -- the net does not catch it and the job goes red for an environmental
reason.  So the six data-dependent tests now carry `requires_s3` explicitly,
which is what the other data-touching modules do.

`from conftest import requires_s3` does NOT work: the repo has a root-level
`conftest.py` and it wins on sys.path, so the plain import raises ImportError
(measured).  Rather than grow yet another private `_aws_creds_available` copy
-- the duplication PR #648 exists to end -- `tests/conftest.py` is loaded by
path under its own module name.

Unchanged: 10 passed on #627's core, 9 passed / 1 skipped on `development`'s,
and dropping only `merge_how: left` still fails exactly two.

Refs #323, #627, #648.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Su5JKX3wKChyfMAdXrdCTr
@ligon ligon changed the title fix(#323) Site 4: the dfs: merge MANUFACTURES the duplicates — 4.9M phantom rows (BLOCKED: needs Ethiopia+Niger config) fix(#323) Site 4: the dfs: merge MANUFACTURES the duplicates — 4.9M phantom rows (UNBLOCKED: Ethiopia/Niger/Nigeria configs merged) Jul 22, 2026
@ligon

ligon commented Jul 22, 2026

Copy link
Copy Markdown
Owner Author

Review applied — 5 findings addressed, 1 disputed with a measurement

Thank you for the mutation testing; it is the part of the review that did the most work, and it is why I did not touch _cartesian_keys at all. Pushed as b42ce5b6 (code + tests) and 7af2ff00 (tracked text), on top of a merge of origin/development (fe9f6c8b).

Everything below was re-derived on the merged branch, cold: isolated LSMS_DATA_DIR wiped before each run with only dvc-cache symlinked, lsms_library.__file__ asserted into the worktree, config via LSMS_COUNTRIES_ROOT. Where I quote one of your numbers it is because I measured it again, not because I copied it.


1 (HIGH) merge order — discharged

#625 merged (194b55d0) while the review was being written; it was the last leg. development is now merged into this branch. All three countries, cold, this core:

rows Latitude non-null site-4 cartesian warns raises
Nigeria 5,248 (8 round-waves) 5,200 0 none
Ethiopia 2,168 (5 waves) 2,146 0 none
Niger 1,599 (4 waves) 1,318 0 none

Niger 2011-12 goes from 0 coordinates to 270/270. Niger 2014-15's 0/270 is honest absence, and is finding 2's example.

Title updated from BLOCKED to UNBLOCKED.

2 (MEDIUM) the escape hatch is country-wide — accepted, documented, and the message fixed

You are right that the guard should be read as "a mis-named column in a dfs: sub-df is fatal", not "a required column is never absent", and CLAUDE.md + the skill now say exactly that. I verified your Niger 2014-15 example rather than quoting it: that wave declares Latitude: float, has no dfs: block (file:/idxvars:/myvars: only), and serves 270 clusters with 0 populated — no raise.

The error message also recommended optional: true "if they are genuinely unavailable for this wave", which misrepresents a country-grain hatch as per-wave. It now says so in the message itself and points at the column name as the real fix — in all ten cells this guard caught, the data existed. Pinned by test_hard_error_says_optional_true_is_country_wide.

3 (MEDIUM) cache-state dependence — disputed as stated

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) — an AST walk over the tagged closures — into every wave hash, which Country._table_cache_hash composes. Holding the config tree fixed and varying only the core:

origin/development   btf(cluster_features) = 16e4e8c6c84e6a55b0…
this branch          btf(cluster_features) = 9c475fa1150055b86c…
mutate only _merge_subframes' body -> 7438a5859b7bcba3f5…

And the concrete artefact you cited, ~/.local/share/lsms_library/Nigeria/var/cluster_features.parquet, grades stale under this branch's core, not fresh. So landing the guard does invalidate; users on the ordinary path are not silently served the pre-guard table.

Two narrower bypasses are real, and your conclusion ("verify cold, don't rely on the guard to force the config fix") survives on them:

  1. a hashless pre-v0.8.0 parquet grades legacy → trust-once and re-stamped with the new hash, so it is served unguarded once and looks fresh forever after;
  2. assume_cache_fresh=True skips the check outright.

CLAUDE.md now records it on that narrower, correct basis. This also corrects §9 of my own ledger, which had implied the same wrong mechanism: the suite was blind because test_table_structure never asks for a build, not because the hash missed the change.

4 (LOW) the required-column check judged presence too early — fixed

Moved below derived:, drop: and the df_edit hook, so "present" means what it means to _assert_built_required_columns. You called it a placement nit; I took it as the live one, because two guards sharing _required_scheme_columns while disagreeing on when is precisely the failure the shared helper existed to prevent. Discriminating test test_required_column_supplied_by_the_wave_hook_is_not_reported_absent raises RuntimeError with the check in its old position and passes with it moved.

5 (LOW) the merge_how prose overstates — confirmed, corrected in all three places, and refined

Re-measured on Ethiopia 2013-14, two isolated cold processes differing only in that key:

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

Your reading is right: the orphans are deleted by the collapse, not collapsed into a phantom cluster. One refinement — DataFrame.equals is False between the two, but every value is identical; the only difference is that outer's null-keyed rows widen District from int8 to float64. So "changes no delivered data at all" is very nearly right, and the residue is a dtype rather than a number. CLAUDE.md, the skill and the source comment now carry the table above, plus your point that outer gave a diagnostic signal left suppresses.

Where the old story does belong. It is not fabricated — it describes the pre-#628 config, where 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 get a null v. Ethiopia/_/CONTENTS.org (~L531) and Ethiopia/2013-14/_/data_info.yml's merge_how comment still tell the old-config story beside the new config. I have not edited them — this PR touches no country config and they belong to #628 — but they should be corrected by their owner.

Nits — both correct, both fixed

  • "67 dfs: merges / 19 countries"76 blocks / 20 countries at 45aee170, your count, reproduced by parsing every data_info.yml. 67 was the number executed. Two quantities reported as one.
  • §7's _pp_/_ph_ claim → it was a filename test overstated into a provenance claim; Nigeria's df_main is Post Planting Wave 1/.../secta_plantingw1.dta. The conclusion now rests where it should: t is constant within every dfs: merge in the corpus.

One reuse fix your grep did not cover

You grepped development for _merge_subframes|_cartesian_keys|merge_how — correctly zero. But LSMS_GRAIN_STRICT's reader had landed: PR #614 put _grain_strict() in this same module, and site 4 was reading the variable itself with a bare os.environ.get. Not bit-identical — measured:

LSMS_GRAIN_STRICT=0  ->  _grain_strict() == False  (sites 1-2 warn)
                     ->  site 4 RAISED

Now if _grain_strict():. Discriminating test fails on the old form for 0/false/no/off.

Which turns up something for your "flip it on in CI" plan. The lever is shared, so with LSMS_GRAIN_STRICT=1 today, Ethiopia / Niger / Nigeria cluster_features() all raise GrainCollapseError — from site 2, the household→cluster projection, not from here. Site 4's census being clean is necessary and nowhere near sufficient. The ledger's old "flip it once the 8 cartesian cells are re-keyed" is corrected accordingly.

Not accepted as a defect: vacuous end-to-end tests

Checked, since it would void the discrimination claims. 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, and no pytest.skip anywhere in the module. All 28 collected items execute on every run, including in a data-free CI job.


Census re-measured on the merged tree

DECLARED blocks: 80 | country/wave dirs: 48 | countries: 20
EXERCISED merges: 90     # >80: Nigeria's wave_folder_map replays one dir under two round labels
CARTESIAN cells: 3
cell merged phantom owner
Malawi 2010-11 196,083 183,812 #653, open
Malawi 2019-20 185,842 171,230 #653, open
Guinea-Bissau 2018-19 5,410 59 #653, open

Those three phantom counts are identical to the original census's, from a different sweep on a different day — it reproduces. 4,552,673 of 4,907,774 phantom rows (92.8%) are gone (Mali → #641, Ethiopia ×2 → #628, Nigeria ×2 → #625); 355,101 remain, all three cells in #653. Only sweep failures: Nepal ×2, PathMissingError, no data, pre-existing. 0 required-column raises library-wide.

New ledger sections: §11 (finding-by-finding) and §12 (the re-measured census). §1, §2, §4, §5, §6, §7, §9 and §10 all carry corrections marked as such rather than silently rewritten.

🤖 Generated with Claude Code

ligon added a commit that referenced this pull request Jul 22, 2026
fix(#323/#627): Malawi's and Guinea-Bissau's cartesian dfs: merges, and #637's three Malawi .first() sites
…xisting on development

`pytest tests/` on the merged branch: 2 failed, 3935 passed, 142 skipped.
Both failures are the CotedIvoire/cluster_features pair, and both reproduce on
a clean origin/development worktree with none of this PR's code — verified by
running exactly those two node ids there, not by assertion.  Zero new failures.

Recorded with the §9 caveat attached: a green suite is necessary and not
sufficient here, because `test_table_structure` reads cached parquets and never
asks for a build.  The evidence for this PR is the cold 90-merge census (§12).

Refs #323.

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

ligon commented Jul 22, 2026

Copy link
Copy Markdown
Owner Author

Full suite on the merged branch (4a7df10c)

2 failed, 3935 passed, 142 skipped, 9 xfailed, 1 xpassed   (31m48s)

Both failures are test_table_structure::{test_declared_columns_present, test_feature_is_sane}[CotedIvoire/cluster_features], and both reproduce on a
clean origin/development worktree with none of this PR's code — I ran
exactly those two node ids there rather than assuming it. Zero new failures.
(The third historically-failing test, test_currency::test_feature_ghana_per_wave,
no longer fails on either.)

Recorded in the ledger as §12.1, with the §9 caveat attached: this is necessary
and not sufficient, because test_table_structure reads cached parquets and
never asks for a build. The evidence for this PR is the cold 90-merge census in
§12, not this line.

@ligon

ligon commented Jul 22, 2026

Copy link
Copy Markdown
Owner Author

Independent verification of the review remediation — #627

Verdict: READY. All five findings plus both nits are addressed or disputed with evidence, and the evidence holds up when re-derived from scratch. I re-ran both negative controls myself rather than accepting the report, and re-measured every checkable number independently.

Environment: fresh worktree at 4a7df10c, PYTHONPATH + LSMS_COUNTRIES_ROOT both asserted in-process, every build in an isolated LSMS_DATA_DIR wiped beforehand with only dvc-cache symlinked. No dvc CLI invoked.


Finding-by-finding

# finding claimed my verdict
1 (HIGH) merge order — #625 unlanded discharged CONFIRMED
2 (MED) country-wide escape hatch documented + message fixed CONFIRMED
3 (MED) hard error is cache-state-dependent disputed DISPUTE UPHELD — I reproduced it
4 (LOW) required-column check placement fixed CONFIRMED (negative control re-run)
5 (LOW) merge_how prose overstates corrected CONFIRMED (re-measured)
nit 67 merges / 19 countries → 80 blocks / 20 countries CONFIRMED (re-counted)
nit §7 _pp_/_ph_ filename test, not provenance CONFIRMED

1. 194b55d0 (#625), 3488b791 (#628) and #644 are all ancestors of 4a7df10c. Built all three countries cold, in one process, from an empty data root:

Nigeria   5,248 rows | Latitude on 5,200 | 8 round-waves | 0 raises | 0 cartesian warnings
Ethiopia  2,168 rows | Latitude on 2,146 | 5 waves       | 0 raises | 0 cartesian warnings
Niger     1,599 rows | Latitude on 1,318 | 4 waves       | 0 raises | 0 cartesian warnings

Row-for-row what the PR body claims. The HIGH block is genuinely gone.

2. The RuntimeError message contains COUNTRY-grain and EVERY wave of {country}, pinned by test_hard_error_says_optional_true_is_country_wide. I re-derived the Niger 2014-15 example from config rather than quoting it: that wave's cluster_features is a single-file extraction with no dfs: block, Latitude: float is declared required in Niger/_/data_scheme.yml, and the wave's myvars carries no coordinate — so it serves the column absent and never trips the guard. Exactly the asymmetry the finding named, and now stated plainly in CLAUDE.md and the skill.

3 — the dispute, independently reproduced. The reviewer's premise was that country.py is outside the v0.8.0 content hash. It is not, for this table. Holding the config tree fixed and varying only the core:

origin/development (4c236d11)  btf(cluster_features) = 16e4e8c6c84e6a55b0…
this branch (4a7df10c)         btf(cluster_features) = 9c475fa1150055b86c…

Both match the values reported. I then made my own mutation to _merge_subframes' body (phantom formula n*m − max(n,m)n*m, a different mutation than the one reported) and the fingerprint moved again to c0f133d1b6b5ad6e6b…. And grading the real warm artefact the review cited:

~/.local/share/lsms_library/Nigeria/var/cluster_features.parquet
  embedded 083869713b5d23c2…
  current  530e23e3bbc5cc95…  (branch core)   -> STALE

So the premise is measured false and the dispute is credible. The two narrower bypasses the response accepted are real — I read local_tools.cache_freshness: a hashless parquet returns 'legacy' (docstring: "trust-once, caller should re-stamp"), and assume_cache_fresh=True skips the check. CLAUDE.md now records the finding on that corrected, narrower basis, and the reviewer's operational conclusion ("verify cold") is preserved.

4 & the negative controls — re-run by me, not taken on trust. Both are committed changes, so git stash would have proved nothing; I patched the file back to its pre-review form each time.

control expected I measured
if _grain_strict():if os.environ.get('LSMS_GRAIN_STRICT'): 4 failures 4 failed, 24 passed — exactly the 0/false/no/off parameters
required-column block moved back above derived:/drop:/hook 1 failure 1 failed, 27 passedtest_required_column_supplied_by_the_wave_hook_is_not_reported_absent, with the RuntimeError
restored 28 pass 28 passed

Both new tests really do discriminate. I also read the module: 20 test functions, 28 collected items, tmp_path CSVs only — no pytest.skip, no xfail, no requires_s3, no microdata. Nothing in it can go vacuous in the data-free CI job.

5 — re-measured, not quoted. I built a symlinked alternate LSMS_COUNTRIES_ROOT with a real copy of Ethiopia, ran 2013-14 in two separate cold processes with a wiped data root, differing only in whether merge_how: left was present:

wave rows null-v delivered clusters ΣLatitude District dtype 434th EA present?
outer 5,287 25 433 4070.3702 float64 no
left 5,262 0 433 4070.3702 int8 no

Confirms the corrected prose: the orphans are deleted by the collapse, not collapsed into a phantom cluster; only the dtype differs.


The census, swept independently

I parsed every data_info.yml and then exercised every declared block cold, with my own detector script (not the PR's sweep):

DECLARED blocks: 80 | country/wave dirs: 48 | countries: 20
EXERCISED: 88 grab_data calls + 2 that raised PathMissingError = 90 attempts
CARTESIAN cells: 3
REQUIRED-COLUMN RAISES: 0
cell merged phantom
Malawi 2010-11 196,083 183,812
Malawi 2019-20 185,842 171,230
Guinea-Bissau 2018-19 5,410 59

Identical to §12 to the row, from an independent sweep. The only other errors are Nepal 1995-96 / 2003-04 sample (PathMissingError, no data in the repo, pre-existing) — which also reconciles the 88-vs-90 count.

Also checked the ledger's one remaining "still open" claim, since it is tracked text: with LSMS_GRAIN_STRICT=1, Ethiopia, Niger and Nigeria cluster_features() all raise GrainCollapseError from the site 1/2 report, not from site 4. §10's corrected framing is accurate.

And the two [CotedIvoire/cluster_features] failures reproduce on a clean 4c236d11 worktree with none of this PR's code.

Scope

Five files: lsms_library/country.py, tests/test_gh323_site4_dfs_merge.py, CLAUDE.md, the sample skill, the ledger. No country config touched, nothing under lsms_library/ beyond country.py (which the PR always owned). No aggregation: key wired up — it appears only in _SCHEME_NON_COLUMN_KEYS, a faithful move of the old _skip set, and as a test fixture asserting it is not read as a column. D1 honoured: the guard reports, it never reduces.

Two things for the merger (neither blocks)

  1. A false claim does still live in two git-tracked files, and it is not this PR's. Ethiopia/_/CONTENTS.org ~L535 and Ethiopia/2013-14/_/data_info.yml L62-63 say left stops the orphans "collapsing into one phantom cluster" and that W2 "gained a spurious 434th EA (030412088800105)". Measured above under the current config: outer yields 433 clusters and 030412088800105 is absent — the orphans carry a null v and are deleted. True of the pre-fix(#323): Ethiopia + Niger geo — 3 of Ethiopia's 5 waves had ZERO coordinates (unblocks #627) #628 config (df_geo keyed v: ea_id2), false since. Correctly left alone here (this PR touches no country config) and already flagged on the thread; it wants a one-line fix in fix(#323): Ethiopia + Niger geo — 3 of Ethiopia's 5 waves had ZERO coordinates (unblocks #627) #628's territory.
  2. The branch is behind current origin/development (4c236d11 vs 212340bafix(#323): assets Value is additive — recovers ₦294,594,970 (25.6% of Nigeria W2, × 2 rounds) #629, fix(#645): to_parquet stops deleting real data via string-matched nulls #655, fix(#648): the documented conftest import does not work — use a marker instead #657 have landed since). Mergeable, but a re-merge before landing would let the cold verification stand against what actually merges. CI unit-tests was still pending when I looked.

Verification by an independent agent; every number above re-derived, none copied from the remediation report.

@ligon
ligon merged commit bcf3d5b into development Jul 22, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant