verify(#323): Ethiopia is GREEN under #627 (already fixed by #628) — unblocks #627; + one real individual_education index bug#644
Conversation
be5913f to
9dc821a
Compare
Four PRs in a single day went red for the same non-reason (#625, #632, #641, #644): a newly added test module builds real country data, the CI `unit-tests` job runs deliberately data-free (`LSMS_SKIP_AUTH=1`, no S3 secrets -- only `data-tests` carries them), and the module lacked the skip guard its neighbours have. Each cost a diagnosis cycle to establish the failure had nothing to do with the change under review. #644's is the worst of them: it CATCHES `NoCredentialsError` and re-raises it as a message about the geo file spelling its coordinate columns differently, which is a confident and completely wrong diagnosis. There was no `tests/conftest.py` at all, which is why the same `_aws_creds_available()` helper had been copy-pasted into three modules already, each noting that centralising it was wanted "but not in a CI-red hotfix". This adds it, plus `requires_s3`, plus a `pytest_runtest_makereport` wrapper that converts a missing-credentials failure into a skip -- covering the `setup` phase too, since the usual shape is a module-scoped fixture building a country, where one error takes out the whole module. ## It cannot hide a real failure The conversion is conditioned on the ENVIRONMENT, not on the exception: * credentials absent -> convert to skip; * credentials present -> report the failure, unchanged. A test silently skipped everywhere is worse than a test red somewhere, so this was verified in both directions rather than assumed: creds absent NoCredentials -> SKIPPED, `assert 1 == 2` -> still FAILED creds present NoCredentials -> FAILED, `assert 1 == 2` -> still FAILED ## The subtlety that made verifying this hard, now documented in the file `lsms_library/countries/.dvc/s3_creds` is NOT tracked in git -- it is written at IMPORT TIME by the auto-unlock. So a fresh `git worktree` looks credential-free until the first import silently creates it, and the guard then reads True. Only `LSMS_SKIP_AUTH=1` reproduces CI's actual state. My first three attempts to verify this hook "failed" for exactly that reason. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…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>
9dc821a to
24712f6
Compare
Adversarial review — verdict: APPROVE_WITH_NOTESI tried to break this and could not break the fix. Everything load-bearing reproduced Method. Two git worktrees (
Re-derived, cold, and confirmedRaw identifiers (read straight from the
The W3 "two real, distinct people" claim is the one I most wanted to falsify — the house Different age, different relationship, in one case different sex. The split-off's roster Conservation. Post-fix the country-level row count per wave equals exactly the raw count
W3 gains exactly 2 rows — precisely the two collisions where both rows were non-null and Roster join — 0 / 8,505 and 0 / 12,633 before, 12,583 / 12,583 and 12,635 / 12,635 Negative control reproduces exactly. Reverting only the five Ethiopia Mutation test of the D1 guard. Injected "GREEN under #627" independently confirmed. On my own merge of Also verified: GPS disagreement table (333/0, 433/38, 432/72, 516/0, 432/72 — exact); EA Findings (all LOW — documentation only, none blocking)L1 — the order-dependence caveat does not reproduce. The body, L2 — the headline row totals don't reproduce. "59,092 → 63,181 (+4,089) under identical L3 — informational, pre-existing, not caused by this PR. The 42-row gap between my None of L1–L3 touches the correctness of the four changed YAML lines, which are right. |
…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
Ethiopia is GREEN under #627. Verified, not assumed — and this PR deliberately carries no
cluster_featuresconfig change, because the fix is already ondevelopment.#627's blocker table is stale
#627's body lists Ethiopia — 3 cells — "config fix written but not yet filed". That body was written 2026-07-13. Commit
3488b791(PR #628) landed the fix and is an ancestor oforigin/development. Nobody had confirmed it against #627's actual core — the sibling agent who cleared Niger flagged exactly that. This PR is that confirmation.Method:
developmentmerged withfix/323-site4-dfs-merge(git merge-tree→59b34cc2) onPYTHONPATH, asserted at runtime vialsms_library.__file__; config read viaLSMS_COUNTRIES_ROOT; isolatedLSMS_DATA_DIRwhose only warm tier is a symlinkeddvc-cache.Negative control — pre-
3488b791config, same coreReproduces #627's census exactly:
df_geolackslat_dd_mod(file hasLAT_DD_MOD)lat_mod, notlat_dd_modea_idat allCurrent
developmentconfig, same coreCountry('Ethiopia').cluster_features()→ 2,168 rows on a unique(t, v), 2,146 with Latitude (333 / 433 / 432 / 535 / 435 clusters). All 25 declared Ethiopia tables build: 0 raised, 0 cartesian, 0 dropped sub-dfs. #627's own 16 tests pass against this config tree.The one real gap found:
individual_education(2013-14, 2015-16)Not a #627 blocker. A #323 site-1 true positive, unfixed until now, found while verifying the above.
Both waves indexed education on the W1-baseline
household_id/individual_id, whilehousehold_rosterandshocksfor the same waves correctly used the wave-nativehousehold_id2/individual_id2.('2013-14', '', ''). The framework's own audit said so:010501020100105031and010501088800105031, the8888marking the split-off, both carry baseline01050100105031), so 16 rows in 8 pairs collided andfirst()merged two real, distinct people: one recorded10th Grade Complete, the other9th Grade Complete.It also made education unjoinable to the roster: 0 of the built
(i, pid)pairs matched ahousehold_rosterrow in W2/W3, against 100% in W1/W4/W5.Fixed by correcting the identifier, per the standing rule — never by declaring a reducer.
(t, i, pid)(t, i, pid)GrainCollapseWarnings(i, pid)joinable to roster(i, pid)joinable to rosterid_walk+ the existingpanel_idschain still maphousehold_id2back to the W1 baseline, so panel linkage is preserved — this is exactly whathousehold_rosteralready relies on.Deliberately NOT salvaged from the abandoned
fix/323-ethiopiaaggregation:keys — dead config, contradicts D1. A test now pins that Ethiopia declares none, so a future salvage cannot revive them.interview_datemapping.pyhooks — measured redundant:_enforce_canonical_dtypesalready honours the declaredInt_t: datetime, and the table returnsdatetime64[us], 8,236 rows, 0 nulls ondevelopment. Adding them would be inert code that reads as a fix.final_index: [t,v]+drop: [i]shape — wrong:Wave.cluster_features' GH Uganda cluster_features: wrong grain, stringified-float District, missing GPS wiring #161 collapse only fires wheniis an index level.3488b791gets this right.A
CONTENTS.orgclaim that had gone false#628 documented the collapse as taking
mean()for Latitude/Longitude ("the cluster centroid") andfirst()for the rest. The GPS.mean()has since been retired — core aggregates nothing anywhere. Corrected, and do not assume the flip is a no-op here. Measured:W5 EA
020103010100203spans 1.79° of longitude (~200 km). Points that far apart are not one enumeration area — this is a cluster key that is not unique nationally (the Malawi 2013-14 pattern), not GPS jitter. Recorded, unfixed, GH #323 site 2.Tests
21 new, asserted at the wave level — post-collapse index uniqueness holds by construction and passes with the bug fully present (the instrument note inherited from the Benin/Togo tests).
Negative control run: 21/21 pass against the current config; 17/21 FAIL against the pre-
3488b791tree on the same core. Of the 4 that still passed, 3 were data-backedcluster_featurescases whose raise was shadowed by a stale L2-wave parquet — #627's "the green is a lie" warning, live. The config-level assertions failed 5/5 in every wave and cannot be shadowed.Recorded, not fixed
individual_education's country-level row count is order-dependent.Country('Ethiopia').individual_education()→ 63,139; the same call after touchingc.panel_ids→ 62,939. The delta lands on waves this PR does not touch and reproduces in the pre-fix config. Cause: the lazyself.updated_idsprobe in_aggregate_wave_data, whoseexcept (...): passrecords a failed first attempt as "attempted" and then skipsid_walkfor the rest of the process. The same call returns two different row counts depending on what the process touched first — a correctness problem. Worth its own issue.panel_ids:block still points atsect_cover_ls_w2.dta— the livestock cover, holder-grain (verified: 3,812 rows / 3,670 households, vs the household cover's 5,262 unique).data_scheme.ymldeclarespanel_idsasmaterialize: makeso the country script wins, but "appears unused" is not evidence and deleting config on that basis is the move the repo forbids.Ledger:
.coder/ledger/323-ethiopia-config.md. Refs #323, #515. Unblocks #627.🤖 Generated with Claude Code