fix(#323): Mali 2021-22 cluster_features — delete the 4.3M-phantom-row cartesian (88% of #627's census)#641
Conversation
8e4cf98 to
b796b9c
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>
…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>
b796b9c to
2014b7a
Compare
Adversarial review — APPROVE_WITH_NOTES (one MEDIUM: a test claims a guard it does not have)I tried to break this from the data up, cold, and the substantive claims hold. Everything below was re-derived independently, not read off the PR body. SetupTwo isolated My first "before" run was wrong (I pointed at the Headline numbers — reproduced
The The source-equivalence, checked myself
So no "invariance by missingness" (nothing is null), no entity-splitting (no new index level was introduced at all), and no catch-all bucket. Not applicable failure modes, checked rather than assumed. Other waves — the m:1 claim is exactly right
Negative control — verified, not taken on trustRan the PR's test file against the pre-fix config in a fresh data root: Warm-cache upgrade — checked, worksThe pre-fix L2-wave parquet really did persist all 4,718,148 rows to disk ( Scope / policyConfig + docs + tests only; nothing under MEDIUM — the test docstring claims a guard the test does not implement
It does not. Mutation-tested. I edited the branch's YAML to Nothing caught it. The grain audit does not catch it either — Given this repo's own standard ("an unevidenced these are the same column claim is exactly the kind that rots silently"), a claim that the guard exists is worse than not making it. Cheap fix — read the names out of the config instead of hard-coding them: myvars = mali['2021-22'].resources['cluster_features']['myvars']
region_col, rural_col = myvars['Region'], myvars['Rural']…then use those in the groupby. That makes the docstring true and turns the mutation above into a failure. (Or, if you prefer to keep it a pure data-invariant test, delete the sentence.) LOW-1 —
|
1 participant
Clears 88% of PR #627's phantom-row census — 4,324,668 of the 4,907,774 phantom rows in the whole 40-country corpus, from one cell that had no owner.
CONFIG-ONLY. Nothing under
lsms_library/*.py. Noaggregation:key. The cartesian is cured at the merge (#323 decision D1); nothing is reduced afterwards.Root cause — established from the data, not assumed
Mali/2021-22/_/data_info.ymldeclaredcluster_featuresas a two-sub-framedfs:merge on the cluster keyv:df_mainehcvm_conso_mli2021.dtadf_geos00_me_mli2021.dta513 grappes, and both sides repeat every key value — so the join is many-to-many by construction: every consumption line-item pairs with every household of its grappe.
_normalize_dataframe_indexthen collapsed 4.7M rows to 513 withgroupby().first(), and the L2-country parquet is written post-collapse. The table looked clean at every point anyone ever looked; the phantoms lived only in the wave-level frame. (CLAUDE.md, "Grain Collapse": the bug hid behind the cache the bug poisoned.)The fix: delete the merge
The brief's hypothesis was that Region/Rural are on the cover page too. They are — but not under the names it guessed.
s00_me_mli2021.dtahas no literalregion/milieucolumn; it carries the EHCVM cover-page geography codes. Checked against the data:Regions00q01regionRurals00q04milieuLatitude/LongitudeGPS__Latitude/GPS__LongitudeBoth are constant within grappe in both files, and the two files cover the identical 6,143
(grappe, menage)households. Prior art in this same repo:sample.df_coverin this very file already readsRuralfroms00q04, and 2017-18'scluster_featuresalready readsRegion: s0q01/Rural: s0q04from its cover page. So the merge was never needed — the block is now a single-file extraction, per CLAUDE.md's "collapse to a single-file extraction" / "grandfathered but should be collapsed when touched".Also verified rather than assumed: the 2021-22 GPS fix is one distinct coordinate pair per grappe, 513/513 — the survey's displaced cluster fix stamped on each household (GH #161), so the
(t, v)collapse cannot pick one household's coordinates over another's.Before → after (cold, isolated
LSMS_DATA_DIR, #627's detector onPYTHONPATH)Final country-level
cluster_features: 3,006 rows, of which 513 for 2021-22 — exactly Mali's grappe count (513 distinctgrappein both source files).This changes no value. At all.
The full country-level frame — 3,006 rows × 4 columns, all four waves — is bit-for-bit identical before and after:
DataFrame.equals() is True, index equal, dtypes equal. The 2021-22 slice alone is likewise identical over all 513 grappes. This PR removes 4.3M invented rows and alters nothing a user has ever been served.Other Mali waves — checked, and they are fine
Their
df_geois one row per grappe, so those merges are m:1, not m:n:df_geoeaci_geovariables_2014.dtaeaci_geovariables_2017.dtagrappe_gps_mli2018.dtaConsistent with #627's census, which flagged only 2021-22. Deferred (waste, not a defect): 2018-19 still reads a 366,639-row consumption file to produce 551 cluster rows. It manufactures nothing, and unlike 2021-22 it cannot be collapsed to one file — its GPS genuinely lives in a separate
grappe_gps_mli2018.dta. Slimmingdf_mainto the cover page would be pure performance; left alone so this PR alters no value anywhere.Tests, with the negative control actually run
tests/test_gh323_mali_cartesian.py— 11 tests. They assert at the wave level on purpose, and the docstring says why: the country-level frame is 513 rows with or without the bug, and its values are identical, so a country-level assertion passes with the bug fully present.Negative control (pre-fix YAML restored, fresh isolated data root):
…and the four country-level tests passed right through it — which is the point. After the fix: 11 passed.
The equivalence that licenses the whole change (
s00q01 == region,s00q04 == milieu, per grappe) is itself a test, not a YAML comment — an unevidenced "these are the same column" claim is exactly the kind that rots silently.Targeted regression set (
test_grain_collapse,test_grain_collapse_cluster_features,test_declared_spellings,test_coverage_matrix,test_schema_consistency, + the new file): 313 passed.Full suite: 3,762 passed, 167 skipped, 9 xfailed, 1 xpassed — zero failures, zero errors (41m43s).
Not reused
origin/fix/323-mali(a8fe61cc) patchescountry.py+local_tools.py— rejected D1 core changes, and aboutpid/EACI visits, a different defect. Ignored, as directed.Known cosmetic inconsistency, deliberately unchanged
cluster_features.Ruralships the raw FrenchUrbainwhilesample.Ruralmaps it toUrban. Pre-existing and visible in 2014-15 too; fixing it would change returned values, which this PR does not do. Adjacent to GH #602.Ledger:
.coder/ledger/323-mali-cartesian.md. Diagnosis:Mali/_/CONTENTS.org. Refs #323, #627.🤖 Generated with Claude Code