Skip to content

fix(#602): enforce declared spellings — 6 countries were shipping non-canonical Rural (Uganda was the least severe)#605

Merged
ligon merged 4 commits into
developmentfrom
fix/602-spellings
Jul 20, 2026
Merged

fix(#602): enforce declared spellings — 6 countries were shipping non-canonical Rural (Uganda was the least severe)#605
ligon merged 4 commits into
developmentfrom
fix/602-spellings

Conversation

@ligon

@ligon ligon commented Jul 12, 2026

Copy link
Copy Markdown
Owner

Closes #602.

spellings blocks in lsms_library/data_info.yml were mapped but never enforced. This PR adds the enforcement check, sweeps all 34 buildable countries with it, and fixes the 6 countries it caught shipping non-canonical Rural values to users.

Uganda — the case the issue was filed on — turned out to be the least severe of the six.


THE ISSUE'S DIAGNOSIS WAS WRONG IN THREE WAYS. READ THIS BEFORE THE DIFF.

The issue is confirmed in substance — the constraint really is unenforced, Uganda really does ship '0' — but every one of its three actionable claims is wrong, and the third is the one that matters.

1. Uganda's root cause is NOT "the extraction emits '0'". It is a YAML key type mismatch, and a previous fix introduced it.

The mapping is declared (Uganda/2005-06/_/data_info.yml:62 on development). It simply never fires:

Rural:
    - urban
    - # ...String keys required (see CLAUDE.md...).  GH #163 item 3.
        '0': Rural        # <-- string key
        urban: Urban

The raw urban column is object-dtype but mixed-typed: rural households carry the python int 0 (2,263 rows), urban ones the str 'urban' (860). The key '0' never matches 0, the value passes through untouched, and the literal string '0' reaches the user.

Commit 9959b9f3 (2026-04-14, "close GH #163 item 3") introduced exactly that string key, citing CLAUDE.md's "YAML mapping keys must be string keys when the raw labels are strings" — advice that is false for this column. It has been silently dead for three months.

The fix is one character: '0'0. Anyone who "re-fixes" this by adding a mapping will re-fail.

2. There is a second, independent gap the issue never mentions: sample was not in Columns: at all.

_enforce_canonical_spellings is a total no-op on sample() — it did not merely fail to reject unknowns, it never mapped the known ones either. That is the entire Tajikistan bug: 9,020 rows of plain lowercase 'rural'/'urban' that the existing spellings block would have fixed for free. Fixing only the rejection check, without declaring sample:, leaves Tajikistan broken and unflagged.

3. "If your check finds only Uganda, be suspicious." It found 10 violations across 6 countries — and two are strictly worse than Uganda.

  • Malawi cluster_features.Rural — the worst. Not merely non-canonical: sign-flipped between waves. The mapping: stanzas destroyed good labels into 0/1 and got the direction wrong (the Cross_Sectional and Panel files disagree on case, so 2016-17 mapped rural:0 and RURAL:1 simultaneously). Agreement with the correct sample.Rural was 0.0% in every single wave. A user pooling Malawi across waves gets a rural indicator with the wrong sign for part of the panel. Uganda at least fails visibly (you get a literal '0'); Malawi hands you a plausible-looking 0/1 that is wrong.
  • India sample.Rural — 100% fabricated. Derived from stratum via a float-keyed map against a string column (India/1997-98/_/data_info.yml:20-26), so it never fired and the raw stratum label reached users as the value of Rural: 'UP-other' (675), ' B-other' (674), 'UP-qual' (542), ' B-qual' (360). All 2,251 rows garbage. strata was affected by the same bug.

One thing I got wrong and corrected mid-flight, recorded so the next person doesn't repeat it: my first (parquet-only) pass flagged Malawi/sample as a violation. It is not — Malawi self-heals via an auto-discovered #+name: Rural table in Malawi/_/categorical_mapping.org:420, which only runs at API time. Reading cached parquets is not equivalent to calling the API. Do not "fix" Malawi/sample.


Evidence it was broken (live API, isolated LSMS_DATA_DIR)

Country / table Before After
Uganda sample.Rural 2005-06 {'0': 2263, 'Urban': 860}(Rural=='Rural').sum() = 0 of 3123 {'Rural': 2263, 'Urban': 860}2263 of 3123
Malawi cluster_features.Rural {'0.0': 1647, '1.0': 1134}; agreement w/ correct sample.Rural = 0.0% / 0.0% / 0.0% / 0.0% / 0.0% {'Rural','Urban'}; agreement 87.2% / 100.0% / 90.0% / 97.5% / 96.1% (2004-05 / 2010-11 / 2013-14 / 2016-17 / 2019-20)
India sample.Rural 2251/2251 stratum labels: 'UP-other' 675, ' B-other' 674, 'UP-qual' 542, ' B-qual' 360 column removed (see below); strata = {UP-qual, UP-other, B-qual, B-other}, leading spaces stripped
Tajikistan sample.Rural {'rural': 5790, 'urban': 3230, 'Rural': 2419, 'Urban': 1084}=='Rural' returned 2419 of 8209 true rural HH fully canonical
Guinea-Bissau cluster_features.Rural {'Rural': 281, 'Urbano': 169}=='Urban' returned 0 rows canonical
Mali cluster_features.Rural French strata (Urbain, Autre urbain, Urbain (District de Bamako)) verbatim — =='Urban' returned 0 rows canonical
GhanaLSS cluster_features.Rural 2016-17 'Urban 'trailing space; looks right in a value_counts, compares false canonical
GhanaLSS sample.Rural 1991-92 'Semi-urban' × 1,578 HH (+ 100 clusters) Urban

Sanity of the "sane" grade, pre-fix: is_this_feature_sane(Country('Uganda').sample(), 'Uganda', 'sample').ok was True — because nothing was checked.

India: why the column was deleted rather than repaired. stratum is a state×phase stratum, perfectly collinear with state (UP vs Bihar). It carries no urban/rural information. There is nothing to map it to. Deleting is class-2 (silently missing) and beats class-1 (silently wrong). Documented in India/_/CONTENTS.org.

GhanaLSS: why loc2=1 is provably Urban, not Semi-urban. It nests exactly: loc2=1 == loc3 ∈ {1,2} == loc5 ∈ {1,2} == Accra (459) + Other Urban (1119) = 1578; loc2=2 == the three rural strata = 2945. GLSS4 declares 1|Urban for the same loc2 (GhanaLSS/1998-99/_/categorical_mapping.org:19-22).


Root causes (file:line, on development)

lsms_library/data_info.yml sample absent from Columns:_enforce_canonical_spellings a no-op on sample()
lsms_library/countries/Uganda/2005-06/_/data_info.yml:62 '0': Rural — string key against an int 0. Introduced by 9959b9f3.
lsms_library/countries/India/1997-98/_/data_info.yml:13-26 float keys (1.0:) against a string column — both strata and Rural
lsms_library/countries/Malawi/{2004-05,2010-11,2013-14,2016-17,2019-20}/_/data_info.yml mapping:/mappings: stanzas destroying canonical labels into 0/1, inconsistently across waves
lsms_library/countries/GhanaLSS/1991-92/_/categorical_mapping.org:32-36 #+name: rural table: `1
lsms_library/countries/Mali/2017-18/_/data_info.yml, GhanaLSS/2016-17/_/data_info.yml raw French / trailing-space labels passed through

The fix

1 — the check. diagnostics._check_declared_spellings (lsms_library/diagnostics.py:447), status fail, naming offending values + row counts. Wired into both is_this_feature_sane (:742) and validate_feature (:969). Three design constraints, each pinned by a test:

  • Keyed on (table, column), never column name. housing.Tenure is a legitimately different vocabulary (dwelling tenure) in ~12 countries and housing declares none — a name-keyed check emits ~12 false failures and discredits itself. I only found this because I validated the instrument first.
  • Membership tested after applying the variant map, so the check is correct on pre-_finalize_result cached parquets (which legitimately hold variants like Malawi's rural/RURAL) as well as on API frames.
  • Reads Columns itself rather than reusing country._load_canonical_spellings(), whose if variant_map: guard drops Affinity/Tenure/TenureSystem (empty variant lists) — they would have gone unchecked.

2 — the sweep. 34 countries × every spellings-constrained (table, column), live API. BEFORE: pass=109 fail=10AFTER: pass=114 fail=5 (all 5 remaining = the enumerated plot_features xfails below).

3 — the data fixes. 9 country configs (Uganda, Malawi ×5, India, Mali, GhanaLSS ×2) + sample: declared under Columns:.

Deliberately NOT fixed

5 plot_features Tenure/TenureSystem pairs ship raw unharmonized labels. The check correctly fails them. They are enumerated in KNOWN_UNHARMONIZED and xfailed — not downgraded to warn. Mapping them needs the questionnaire codebooks (Cambodia's "GIVEN BY THE GOVERNMENT"; Timor-Leste's "Part owner" = 81% of plots) and guessing would replace a visibly wrong value with an invisibly wrong one.


Proof nothing else moved

Instrument validated FIRST — 12 planted cases: fires on violations in columns and index levels; silent on clean frames, on known variants, on all-NaN, and on the housing.Tenure trap.

Byte-identical regression sweep (the #594 bar): 123 (country, table) pairs across all 4 constrained tables, live API, base vs fix, isolated data roots.

  • 112 byte-identical.
  • 8 changed — every one an intended fix; row counts unchanged everywhere:
rows note
Uganda / sample 24362 → 24362 cols same
Malawi / cluster_features 2781 → 2781 cols same
Mali / cluster_features 3006 → 3006 cols same
Guinea-Bissau / cluster_features 450 → 450 cols same
GhanaLSS / cluster_features 3145 → 3145 cols same
GhanaLSS / sample unchanged 1,578 HH: Semi-urbanUrban — missed by my sweep; see the retraction below
Tajikistan / sample 12523 → 12523 cols same
India / sample 2251 → 2251 Rural column REMOVED (intended)
  • 4 errors identical on both branches (Armenia, Nepal — documented no-microdata countries).
  • South Africa did NOT change: 'Informal' was already the shipped value; this PR only admits it to the vocabulary.

All numbers come from an ISOLATED LSMS_DATA_DIR. The shared cache was being overwritten mid-run by concurrent agents rebuilding sample/cluster_features with their configs, producing fresh-mtime/stale-content parquets. It very nearly fooled me (see the retracted GhanaLSS grading, below).

Full suite (isolated data root, both branches, same harness):

FIX : 3 failed, 3239 passed, 127 skipped, 9 xfailed
BASE: 3 failed, 3224 passed, 128 skipped, 4 xfailed

The same 3 failures on both, all pre-existing and untouched: test_currency.py::test_feature_ghana_per_wave (GH #589 — confirmed failing identically on pristine development); test_table_structure::{test_declared_columns_present,test_feature_is_sane}[CotedIvoire/cluster_features]. Zero new failures. xfailed 4 → 9 = exactly the 5 KNOWN_UNHARMONIZED pairs.

Proof the new tests fail on pre-fix code — executed, not asserted:

  1. Against the fully pristine tree the module cannot even import (ImportError: cannot import name '_check_declared_spellings') — the check is genuinely new.
  2. Against this PR's package + pristine country configs (isolating the data fixes): 5 failed, 13 passedtest_uganda_2005_06_rural_is_canonical, test_malawi_cluster_features_rural_not_sign_flipped, test_india_sample_has_no_fabricated_rural, test_ghanalss_2016_17_cluster_rural_has_no_trailing_space, test_ehcvm_cluster_rural_is_canonical[Mali].
  3. test_ghanalss_1991_92_rural_code_map_says_urban verified by direct execution (pristine yields {1: 'Semi-urban'}; this PR yields {1: 'Urban'}). It did not fail via LSMS_COUNTRIES_ROOT because GhanaLSS/1991-92/_/mapping.py:8 hardcodes files('lsms_library')/'countries', ignoring the override — itself a CLAUDE.md anti-pattern; follow-up filed below.

Feature audit harness (bench/feature_audit/scan.py; sample + cluster_features + household_roster + plot_features, both branches): 115 of 122 records identical. Regressions (pass → not-pass): NONE. The 5 warn → fail are exactly the enumerated plot_features pairs — the check converting silently-wrong into loudly-broken, by design. The one anomaly (7 new cluster_features dup-(t,v) warnings) was chased to ground rather than waved off: per-country duplicate count is 0 on both branches, and Feature('cluster_features') is identical on both (41376 rows / 32 countries / 0 dup warnings). The delta was a cache-coverage artifact of the two isolated roots, not a code difference.

Blast radius checked: Rural is declared under sample: without required: true (feature.py:316 reads that flag; many samples have no Rural, and Feature('sample').columns is unchanged). Only Rural was declared — weight/panel_weight/strata deliberately left undeclared so _enforce_canonical_dtypes re-types nothing. Every country already declares sample.Rural as str in its data_scheme.yml, so the new type: str re-types nothing either. The Uganda baseline fixture diff is exactly one line (content_hash); shape / columns / dtypes / index names unchanged.


RETRACTION (do not let this get lost)

An earlier version of this fix graded the GhanaLSS 1991-92 Semi-urban fix as latent / doc-only, on the theory that the Semi-urban values I first saw came from a stale shared-cache parquet and a clean build yields Urban.

That retraction was itself wrong, in the conservative direction. Two red-teamers, independently, built pristine development clean from source in isolated data roots and both got Semi-urban shipped to users: 1,578 rows in sample(), 100 clusters in cluster_features(). GhanaLSS has no country-level Rural Preferred-Label table to rescue it.

The GhanaLSS fix is LIVE class-1, not latent. The changed-table list above is therefore 8, not 7 — my own regression sweep had a blind spot (mapping.py:8's hardcoded path silently ignored the LSMS_COUNTRIES_ROOT override, so base and fix read the same config and came out byte-identical). The change itself is correct — GLSS4 declares 1|Urban for the same loc2 — so this was undisclosed right data, not wrong data. The false "LATENT" docstring has been corrected on the branch (commit 3fd235c3).


Red-team verdicts (3 lenses, all pass; concerns disclosed, not laundered)

1. Correctness — PASS (severity: nit)

The fix does what it claims. I reproduced the original defect on a pristine development tree (git archive development, isolated LSMS_DATA_DIR, import asserted from the export), reproduced the corrected behaviour on fix/602-spellings, validated the new check against pre-fix data, and independently re-ran the cross-country sweep on BOTH branches. Every load-bearing claim held up, and two of the boldest (Malawi sign-flip, GhanaLSS 1=Urban) I confirmed from an angle the author did not use.

Where I disagree with the author: their "self-correction" about GhanaLSS is itself wrong, in the conservative direction. My clean, from-source, isolated build of pristine development ships Semi-urban to users: 1,578 rows in sample(), 100 clusters in cluster_features(). The GhanaLSS fix is a LIVE class-1 fix. The docstring saying "LATENT defect (not currently reaching sample() in a clean build)" is a false statement in a file that will be read as documentation and should be corrected before merge. That is my only requested change; it does not affect any shipped value.

→ Fixed on-branch in 3fd235c3 (docstring only, no runtime surface).

Non-blocking residuals, all disclosed or pre-existing:

  1. Enforcement is diagnostic-only, and test_feature_is_sane uses _read_or_skip on cached parquets — so on a cold cache the new check runs on nothing. The guarantee holds only where someone has built the table.
  2. Informal is now admitted globally to the Rural vocabulary (for South Africa's 83 HH / 2 clusters), so any other country emitting Informal will pass silently.
  3. Malawi's cluster-level Rural rests on ea_id being a clean cluster key, and it is not (2013-14: 131 of 204 EAs contain both urban and rural HHs). Post-fix the cluster label equals the HH-majority label, which is defensible; the pre-fix code carried the same aggregation with meaningless values, so no regression.
  4. India keeps employment.Rural (from v02a05b), outside the enforcement surface and in mild tension with the new CONTENTS.org line "India simply has no Rural indicator" — verified to be a workplace attribute, not residence, so the deletion decision is right.

2. Regression — PASS (severity: concern)

No regression. Nothing that was working breaks: I ran the branch's own _check_declared_spellings over all 302 cached parquets (the exact population test_feature_is_sane parametrizes over, and a SUPERSET of the author's isolated root) and got 11 failures — the 5 plot_features pairs (exactly KNOWN_UNHARMONIZED, no over- or under-enumeration) plus the 6 real defects the branch fixes. Zero household_roster Sex/Affinity false failures, which was the largest false-positive surface. Dtype blast radius is nil. Feature('sample').columns unchanged. The author's Malawi/sample "no change at the API" claim is TRUE and I verified it (Malawi/_/categorical_mapping.org:420-427 is a Rural Preferred-Label table canonicalizing 42,660 variant rows pre-spellings) — the normalize-then-test design holds up.

CONCERN (must fix the PR body, not the code): the regression sweep has a proven blind spot and its changed-table list is incomplete by one. GhanaLSS/1991-92/_/mapping.py:8 hardcodes files('lsms_library')/'countries', bypassing LSMS_COUNTRIES_ROOT — the bug the author found but did not propagate to their own verification. The branch flips rural_dict from {1:'Semi-urban'} to {1:'Urban'}, and GhanaLSS/var/sample.parquet on development holds exactly 1,578 'Semi-urban' rows, which reach users. So the branch changes GhanaLSS sample.Rural for 1,578 households — a live, user-facing change. The summary asserts the opposite and omits GhanaLSS/sample from the enumerated changed tables; the AFTER sweep and the regression sweep are mutually inconsistent and cannot both hold. The change itself is CORRECT — GLSS4's own rural table declares 1|Urban for the same loc2 — so this is undisclosed RIGHT data, not wrong data. Not a blocker, but the blast radius must be restated as 8 changed tables and the "latent/doc fix" claim retracted.

NIT: the Malawi 2004-05 diff comment ("it was even spelled mappings: rather than the mapping: the grabber honours") is factually false — country.py:749-751 honours BOTH keys. That block did fire, which is what makes the author's own 0.0%-agreement figure coherent. The comment would mislead a future maintainer into thinking mappings: is inert.

→ Both addressed. Blast radius restated as 8 changed tables above; the "latent" grading retracted in its own section; the false Malawi comment corrected on-branch in 3fd235c3.

3. Completeness — PASS (severity: concern)

The fix is sound and every claim survived independent verification from source in an isolated data root. It closes the declared-vocabulary class completely — I swept all 4 constrained tables across all 34 buildable countries and found zero remaining violations beyond the 5 enumerated plot_features xfails, whose registry I confirmed is exactly complete (not one country more, not one fewer). It should land.

CONCERN — the gap is one level up. The fix's own root-cause analysis identified the real mechanism: a mapping: stanza whose YAML keys are the wrong TYPE, silently dead, raw labels leaking to users, so df[col == 'X'] returns a wrong subset. _check_declared_spellings is keyed on 6 (table, column) pairs, so it is structurally blind to that mechanism wherever the target column has no declared vocabulary. It is alive today: Albania household_roster.Marital ships 13 labels for 5 concepts, and df[df.Marital == 'Married'] silently drops 11,012 married people across two entire waves. The country's own mapping proves the intended value ('Married'), so unlike the plot_features Tenure cases this needs no codebook — it is unambiguously a bug.

That is not a defect IN the fix and must not block it, but it is exactly the failure the brief warns about: closing #602 with a green check makes the next instance of the same bug harder to find. Land, and file the follow-up (a mapping whose keys match 0% of its source column's raw values is always a bug, and is a ~30-line lint).

→ Follow-ups to file (not in this PR):

  1. The dead-mapping lint. A mapping: stanza whose keys match 0% of its source column's raw values is always a bug. ~30 lines. This is the generator of Declared 'spellings' are mapped but never ENFORCED — Uganda 2005-06 Rural is 2263 rows of the string '0', and it grades sane #602, of Uganda's 9959b9f3, and of India's float keys — a lint catches the class, where _check_declared_spellings only catches the 6 columns that happen to have a declared vocabulary.
  2. Albania household_roster.Marital — 13 labels for 5 concepts; == 'Married' drops 11,012 people. Class-1, unambiguous, needs no codebook.
  3. GhanaLSS/1991-92/_/mapping.py:8 hardcodes files('lsms_library')/'countries' and so ignores LSMS_COUNTRIES_ROOT. A CLAUDE.md anti-pattern, and it blinded a verification run in this very PR. Worth a grep across all mapping.py.

Files

lsms_library/diagnostics.py · lsms_library/data_info.yml · tests/test_declared_spellings.py (18 tests) · tests/test_table_structure.py (KNOWN_UNHARMONIZED xfail registry) · .coder/ledger/602-spellings.md · 9 country configs (Uganda, Malawi ×5, India, Mali, GhanaLSS ×2).

Severity class 1 (silently wrong) by slurm_logs/PLAN_backlog_2026-07-12.org.

🤖 Generated with Claude Code

ligon and others added 4 commits July 12, 2026 13:31
…-canonical values (#602)

`data_info.yml` declares accepted values via `spellings` blocks, but nothing
enforced them.  `_enforce_canonical_spellings()` maps known variants and never
rejects unknown ones, and no sanity check read the declarations at all.  Uganda's
`sample.Rural` was the literal string '0' for 2,263 households (72% of the
2005-06 wave), so `df[df.Rural == 'Rural']` silently returned ZERO rows -- and
`is_this_feature_sane(...).ok` returned True.

The sweep is the deliverable; the check is how you do it.  Sweeping all 34
countries x every spellings-constrained (table, column) through the live API
found 10 violations, of which Uganda was the LEAST severe.

The check
---------
* `diagnostics._check_declared_spellings` -- status `fail`, naming offending
  values + row counts.  Wired into both `is_this_feature_sane` and
  `validate_feature`.
* Keyed on **(table, column)**, never column name: `housing.Tenure` is a
  legitimately different vocabulary (dwelling tenure) in ~12 countries and
  `housing` declares none; a name-keyed check emits ~12 false failures.
* Membership is tested AFTER applying the variant map, so the check is correct on
  both pre-`_finalize_result` cached parquets (which legitimately hold known
  variants such as Malawi's `rural`/`RURAL`) and post-finalize API frames.
* Reads `Columns` itself rather than reusing `country._load_canonical_spellings()`,
  whose `if variant_map:` guard DROPS columns whose variant lists are all empty --
  `Affinity`, `Tenure`, `TenureSystem` would otherwise go unchecked.

Schema
------
* Declare `sample:` under `Columns:`.  It was absent entirely, which made
  `_enforce_canonical_spellings` a TOTAL no-op on `sample()` -- not merely failing
  to reject unknowns, but never mapping the known variants.  That is the
  Tajikistan bug: 9,020 rows of lowercase 'rural'/'urban' next to canonical
  values, so `== 'Rural'` returned 2,419 of 8,209 true rural households.
  `Rural` is declared WITHOUT `required: true` (many samples have no such column).
* Extend the `Rural` vocabulary with the French (EHCVM) / Portuguese labels
  ('Urbain', 'Urbano', ...), and with `Informal` -- South Africa 1993's third
  settlement stratum, kept VISIBLE rather than guessed into Urban (the source's
  own `metro` variable groups it with the non-metro areas).

Data fixes (each a distinct root cause)
---------------------------------------
* Uganda 2005-06: YAML key TYPE mismatch.  Raw `urban` is object-dtype but MIXED
  -- python int 0 (2263) + str 'urban' (860).  The declared key was the STRING
  '0', which never matches.  Commit 9959b9f ("close GH #163 item 3") introduced
  exactly that string key and sat silently dead for three months.  Fix: `0`.
* Malawi cluster_features: the `mapping:` stanzas converted good labels into 0/1
  and got the DIRECTION wrong -- the Cross_Sectional and Panel files disagree on
  case, so 2016-17/2019-20 mapped rural households onto BOTH codes, SIGN-FLIPPING
  the indicator between waves.  Every raw value is already canonical or a declared
  variant, so the mappings are deleted and the label passed through.  Agreement
  with the (correct) sample.Rural goes 0.0% -> 87-100% in every wave.
* India 1997-98: `Rural` was derived from `stratum` via a float-keyed map against
  a string column, so it never fired and the raw stratum label ('UP-other',
  ' B-qual', ...) reached the user as `Rural` for 100% of households.  `stratum`
  is a state x phase stratum (collinear with `state`) carrying NO urban/rural
  information, so `Rural` is DELETED, not invented.
* Mali 2017-18 / GhanaLSS 2016-17: French strata labels, and a TRAILING SPACE
  ('Urban ') that looks right in a value_counts but compares false.
* GhanaLSS 1991-92: the `rural` code->label table claimed 1 = 'Semi-urban'.
  loc2=1 nests exactly onto loc3 in {1,2} (Accra 459 + Other Urban 1119 = 1578),
  so code 1 is the union of the two URBAN strata.  Latent (not currently reaching
  sample() in a clean build) but provably wrong; fixed in the org + CONTENTS.

Remaining, deliberately not force-fitted
----------------------------------------
Five plot_features Tenure/TenureSystem pairs (Albania, Cambodia, Kosovo,
Timor-Leste, Tajikistan) ship raw unharmonized survey labels.  The check correctly
FAILS them; they are enumerated in `tests.test_declared_spellings.KNOWN_UNHARMONIZED`
and xfailed rather than downgraded to `warn`.  Mapping them needs the questionnaire
codebooks, and guessing would replace a visibly wrong value with an invisibly wrong
one -- the exact class-1 failure this issue is about.

BEFORE -> AFTER (isolated LSMS_DATA_DIR, race-free):
  violations                     10 -> 5 (all 5 enumerated + xfailed)
  Uganda  (Rural=='Rural')        0 -> 2263  of 3123
  Malawi  cf vs sample agreement  0.0% -> 87.2/100/90/97.5/96.1% by wave

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…rebase Uganda sample baseline

Follow-ups from the full-suite run, each a consequence of the #602 fixes:

* India `_/data_scheme.yml` still declared `Rural: str` under `sample` after the
  column was removed from the wave YAML, so `_check_declared_columns` (and hence
  `is_this_feature_sane`) failed India/sample on a column that intentionally no
  longer exists.  Removed the declaration; documented in India/_/CONTENTS.org.

* `test_india_sample_has_no_fabricated_rural` asserted the wrong thing: 'UP-other'
  is a LEGITIMATE stratum label (only the Bihar pair carried a leading space).
  It now asserts Rural is absent and `strata` == the four true labels, whitespace
  normalised.

* `tests/fixtures/uganda_baseline.json`: `var/sample.parquet` content_hash rebased.
  Uganda 2005-06's 2,263 '0' values are now 'Rural', so the parquet's contents
  legitimately change.  Shape [24362, 6], columns, dtypes and index_names are all
  UNCHANGED -- the diff is exactly one line (the hash).

Full suite, isolated LSMS_DATA_DIR: 3 failed / 3239 passed / 9 xfailed.
Pristine development, same harness:  3 failed / 3224 passed / 4 xfailed.
The 3 failures are IDENTICAL and pre-existing (test_feature_ghana_per_wave = GH
#589; CotedIvoire/cluster_features x2).  Zero new failures.  The +5 xfails are the
enumerated KNOWN_UNHARMONIZED plot_features pairs.

Regression net (123 country/table pairs, live API, both branches):
  112 BYTE-IDENTICAL; 7 changed, every one an intended fix; row counts unchanged
  everywhere (only India loses the fabricated Rural column).
  Feature('cluster_features'): 41376 rows / 32 countries / 0 dup-warnings on BOTH.

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

Both changes are comments/docstrings; no runtime surface, no value moves.

1. tests/test_declared_spellings.py::test_ghanalss_1991_92_rural_code_map_says_urban
   The docstring graded the GhanaLSS 1991-92 rural code map as a LATENT
   defect ("not currently reaching sample() in a clean build").  That is
   FALSE, and two independent red-team builds from source in isolated data
   roots showed pristine `development` shipping 'Semi-urban' to users for
   1,578 households in sample() and 100 clusters in cluster_features().
   The original mis-grading came from a verification run whose
   LSMS_COUNTRIES_ROOT override was silently ignored, because
   GhanaLSS/1991-92/_/mapping.py:8 hardcodes files('lsms_library')/'countries'.
   The docstring now says LIVE, cites GLSS4's own 1|Urban declaration, and
   records the override-bypass trap so nobody re-downgrades it.

2. Malawi/2004-05/_/data_info.yml
   The new comment claimed the deleted block "was even spelled `mappings:`
   rather than the `mapping:` the grabber honours", implying it was inert.
   country.py:749-751 honours BOTH spellings -- the block DID fire, which is
   precisely why agreement with the correct sample.Rural was 0.0%.  Left
   uncorrected the comment would teach a future maintainer that `mappings:`
   is a no-op.

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

CI `unit-tests` went red on PR #605 with 6 x `NoCredentialsError`. All six are
the new `TestCountryRegressions` cases, which BUILD REAL COUNTRY DATA (DVC ->
S3).  The `unit-tests` job deliberately runs data-free (`LSMS_SKIP_AUTH=1`, no
AWS secrets); only `data-tests` carries the credentials.

The failures are environmental, not defects: the same six pass locally with data
access, and `development` is green.

Applies the house pattern the repo already documents.  From the equivalent guard
in `tests/test_canonical_shape_via_cache_miss.py`:

    "The CI `unit-tests` job intentionally sets LSMS_SKIP_AUTH=1 to keep PR
     validation fast and data-free; this function returns False in that
     environment, and the tests below silent-skip."

The new tests simply did not do that.  They do now.

Verified in BOTH directions, because a skip guard that is wrong in either one is
worse than no guard:
  - CI conditions (no env creds; `.dvc/s3_creds` is NOT git-tracked, and
    LSMS_SKIP_AUTH=1 suppresses the auto-unlock that would create it):
    `_aws_creds_available()` -> False  => the class SKIPS rather than fails.
  - Locally, with data access: all 18 tests RUN and pass -- the regressions are
    not quietly neutered into always-skip, which would have been the lazy fix
    and would have silently disarmed the very checks this PR exists to add.

The pure-unit tests (the check itself, the `sample` vocabulary) are unaffected
and still run everywhere -- they are where the logic lives.

NOTE: the guard is duplicated from test_canonical_shape_via_cache_miss.py rather
than shared.  That duplication is the pre-existing house pattern; centralising it
into `conftest.py` is worth doing, but not inside a CI-red hotfix.

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

ligon commented Jul 12, 2026

Copy link
Copy Markdown
Owner Author

CI fixed (2a775f34) — the failures were environmental, not defects

unit-tests went red with 6 × NoCredentialsError. All six are the new TestCountryRegressions cases, which build real country data (DVC → S3). The unit-tests job deliberately runs data-free (LSMS_SKIP_AUTH=1, no AWS secrets); only data-tests carries the credentials.

The same six pass locally with data access, and development is green — so this is the job, not the code.

The fix is the house pattern this repo already documents

From the equivalent guard in tests/test_canonical_shape_via_cache_miss.py:

"The CI unit-tests job intentionally sets LSMS_SKIP_AUTH=1 to keep PR validation fast and data-free; this function returns False in that environment, and the tests below silent-skip."

The new tests simply didn't do that. Now they do.

Verified in BOTH directions

A skip guard that is wrong in either direction is worse than no guard — so I checked both:

condition result
CI (no env creds; .dvc/s3_creds is not git-tracked, and LSMS_SKIP_AUTH=1 suppresses the auto-unlock that would create it) _aws_creds_available()False ⇒ the class skips, does not fail
Locally, with data access all 18 tests run and pass

That second row is the one that matters. The lazy fix here is a guard that makes the tests always skip — which would have silently disarmed the very regressions this PR exists to add, and CI would have gone green while testing nothing.

The pure-unit tests (the check itself; the sample vocabulary) are untouched and still run everywhere — that's where the logic lives.

Known follow-up: the guard is duplicated from test_canonical_shape_via_cache_miss.py rather than shared. That duplication is the pre-existing house pattern; centralising it into conftest.py is worth doing, but not inside a CI-red hotfix.

@ligon
ligon merged commit 6278474 into development Jul 20, 2026
10 checks passed
@ligon
ligon deleted the fix/602-spellings branch July 20, 2026 04:05
ligon pushed a commit that referenced this pull request Jul 22, 2026
… claim

Adversarial review found that this PR's four tests all pass on plain
`development` WITHOUT the config change, so they pinned an invariant rather
than testing the fix.

Cause: commit `c8c25f68` (GH #602/#605) had already added `Urbano, urbano,
URBANO` to `Columns.cluster_features.Rural.spellings`, and it is an ancestor of
this branch.  `_enforce_canonical_spellings` therefore repairs the value at API
time, so `Country(...).cluster_features()` is canonical with or without this
diff.  Every API-level assertion is blind to the thing being fixed.

The config change is still worth keeping: it fixes the EXTRACTION, so the stored
L2-wave parquet is canonical rather than relying on a read-time net.  That
matters because the parquet is a consumer-visible artifact (`docs/guide/
caching.md`), and CLAUDE.md notes cached parquets hold PRE-transformation data
-- so anything the net repairs is invisible in storage.

Adds `test_wave_parquet_stores_canonical_rural`, which reads the wave parquet
directly and so bypasses the net.  Measured cold in an isolated `LSMS_DATA_DIR`:

    with the fix     stored Rural = {'Rural': 3396, 'Urban':  2014}
    without the fix  stored Rural = {'Rural': 3396, 'Urbano': 2014}

Note on method: my first negative control was INVALID -- I used `git stash`,
which found nothing because the config change is committed on this branch, so I
rebuilt with the fix still in place and got a false "no difference".  Reverting
the file to `origin/development` is what actually removes it.  Recording this
because a negative control that silently tests nothing is the exact failure this
commit exists to fix.

The module docstring now states plainly that the API-level assertions do NOT
discriminate, and which single test does.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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