fix(#323): add a condition index level to Uganda crop_production#649
Conversation
…d_codes + the Tenure vocabulary shape Records the machinery in force before touching crop_production: the `conditions:` loop already in crop_production_for_wave, the harvest_units org-table pattern, `_enforce_canonical_spellings`'s index-level branch (country.py:3962), and the two constraints that shape the design -- `_normalize_dataframe_index` drops undeclared index levels, and the dedup collapse's groupby(dropna=True) deletes rows with a null index key. Reuse decisions: extend CROP_COLMAPS + a sibling harvest_conditions org table; declare the cross-country vocabulary the `Tenure` way; DEFER the index-level enumeration ENFORCEMENT (needs an S_VALUES-style core constant) and the `index_info` registration (blocked on cross-country plot/plot_id, crop/j name harmonisation across 14 countries). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Su5JKX3wKChyfMAdXrdCTr
UNPS question 6 is compound -- "how much did you harvest, in what unit,
AND IN WHAT CONDITION" -- so a household that takes some of a crop off a
plot fresh and the rest after drying it answers twice. crop_production
was keyed (t, i, plot, j, u, season) with no level for the condition, so
those answers landed on the same index tuple and the de-duplication block
at the end of crop_production_for_wave SUMMED them. Dry weight was added
to fresh weight. Measured example, 2011-12 HH 1033000506 plot -1-6:
240 kg "Dry after additional drying - in pods" + 100 kg "Fresh/raw
harvested - in pods", same month, same unit -> one 340 kg row. 340 is
not a quantity of anything.
The grain is now (t, i, plot, j, u, condition, season).
Both vintages encode the condition with the SAME 20 integer codes:
2009-16 LONG a value in a5aq6b / a5bq6b, multiple source rows.
2018-20 WIDE parallel _1/_2 slots, each with its own 6c column. The
slot number is NOT an ordinal "first/second harvest" --
the 2019-20 Stata labels read "(2018, full harvest,
condition1)" / "(... condition2)".
So one `harvest_conditions` table in categorical_mapping.org serves every
wave; no harmonisation between vintages was needed or invented. Labels
encode the scheme's own 2-D cross of a dryness family (green / fresh /
dry-at-harvest / dried) with a physical form (with shell and stalk / with
shell / in cob / in pods / grain). The canonical cross-country
vocabulary is declared in data_info.yml the way plot_features.Tenure is,
with the "countries may extend rather than force-fit" stance --
_enforce_canonical_spellings applies it to index levels, not just columns
(country.py:3962).
Verified per wave, against the Stata metadata, which column is the unit
and which the condition -- by VALUE-label vocabulary, never the variable
label. The old comment's claim that "the WB .do's A5aq6b/A5aq6c
unit/condition rename is inverted for these actual UNPS files" holds for
exactly one file (2013-14 AGSEC5A); every other wave labels its variables
correctly. Corrected in place. 2009-10/2010-11 carry no value labels at
all and were matched by code distribution.
`condition` is sentinel-filled ('unknown_condition', 6.0% of rows,
concentrated in the two unlabelled waves) rather than left NA: the
collapse's groupby(level=...) defaults to dropna=True and silently
DELETES rows with a null index key.
Measured cold, isolated LSMS_DATA_DIR:
wave dup groups rows Quantity
before -> after before -> after (identical)
2009-10 670 -> 181 24997 -> 25485 310,664,565.8
2010-11 608 -> 81 20442 -> 20970 708,149.5
2011-12 653 -> 55 19552 -> 20152 1,128,560.3
2013-14 589 -> 61 17053 -> 17585 1,098,713.5
2015-16 764 -> 63 18874 -> 19576 1,050,367.4
2018-19 0 -> 0 14194 -> 14194 1,180,031.7
2019-20 370 -> 18 15369 -> 15721 1,175,600.8
total 3654 -> 459 130481 -> 133683 317,005,989.0
Collisions fall 87.4%. Quantity, Quantity_sold and Value_sold totals are
identical per wave before and after -- summing is conservative, so a
correct fix moves rows apart without creating or destroying mass. The
459 residual groups are now all commensurable (same crop, same unit, same
condition), which is exactly what was not true before; characterised in
CONTENTS.org.
Feature('crop_production') is byte-identical before and after: 250 692
rows, the same 8 kept countries, the same 6 excluded. Uganda was ALREADY
modal-excluded (plot/j/u/season vs the modal plot/crop/u), so a 7th level
changes nothing there. crop_production stays out of index_info, per the
standing note that the plot-level ag features need cross-country index
NAME harmonisation first.
Also documents four adjacent defects found while verifying, none of them
touched here because each moves data: 2018-19 AGSEC5B's harvest unit
exists but is unwired; 2019-20 season B's condition2 slot is dropped
entirely (1 306 records); the collapse deletes null-plot rows (431 rows /
7.1M units in 2009-10); and 2009-10 quantities carry an unstripped 99999
sentinel.
Tests fail on the pre-fix tree (6 failed / 3 errors / 1 passed), notably
"expected the dry (240) and fresh (100) coffee records to stay separate,
got quantities [340.0]".
Refs #637, #647.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Su5JKX3wKChyfMAdXrdCTr
Adversarial review — verdict: APPROVE-WITH-NOTES (one comment I'd fix before merge)I set out to break this and could not break the data path. Every headline number reproduces exactly on an independent cold build, and the two failure modes this class of fix is prone to — zero-by-splitting and invariance by missingness — are both absent from the built table. The findings below are three documentation defects and one test-coverage gap. None of them changes a number in the table. Method. Isolated FindingsF1 — MEDIUM — the code-99 justification is wrong by ~100×, and it is the premise for a semantic merge
Measured: 340 rows in the built table carry
Why it matters: code 99's label genuinely differs by vintage — verified from the Stata metadata, F2 — LOW —
|
| code | 2018-19 raw label | org Source Label |
|---|---|---|
| 24 | Fresh/raw harvested - in pods |
Fresh/raw harvested - in pods or shell/husks |
| 32 | Dry at harvest - with shell |
Dry at harvest - with shell/cob without stalk |
| 42 | Dry after additional drying - with shell |
Dry after additional drying - with shell and without stalk |
| 99 | Not Applicable |
Others / Not Applicable |
Harmless at runtime — I confirmed _harmonized_codes reads only Code + Preferred Label, and the 3-column table parses to exactly 20 codes with unique labels — but "verbatim" is the wrong word for a provenance column, and the comment contradicts itself two sentences later where it correctly describes the drift. On code 45 specifically ('Dry – grain' → dried_grain, ~25% of all records): the rename is an inference from grid position, not from the literal label. I think it's the right inference — 35 already occupies "Dry at harvest – grain", so 45 must be the additional-drying counterpart — but it is an assertion the label alone does not make, and the org file should say so as plainly as it says the rest.
F4 — MEDIUM — the suite cannot detect a silently unwired condition column
crop_production_for_wave falls back to the sentinel with no warning when the colmap names a column absent from the source:
if cond.get('condition') and cond['condition'] in df5.columns:I mutated CROP_COLMAPS['2018-19']['A']'s condition to a5aq6b_TYPO, physically cleared the Uganda cache, and reran the suite: all 10 tests passed, while 2018-19 season A went from 20 distinct conditions to unknown_condition on 7,153 / 7,153 rows. The three tests that look like they'd catch it don't:
test_every_wave_sources_a_condition_columnchecks the colmap names a column, not that it resolves;test_condition_values_are_canonicalpasses becauseunknown_conditionis canonical;test_condition_actually_separates_recordsis a panel-wide> 1000threshold that one broken wave-season cannot trip.
This is the "invariance by missingness" mode, and it is live here for a specific reason: 2018-19 legitimately mixes naming conventions ('qty': 's5aq06a_1' next to 'condition': 'a5aq6b' — I checked, both genuinely coexist in that file), so a real typo is indistinguishable from correct config by inspection. Cheap fix: raise or warn in the else branch when a condition column was named but not found, and/or assert a per-wave-season ceiling on the sentinel share.
Related, smaller: test_fresh_and_dry_no_longer_collide carries if sel.empty: pytest.skip(...). It genuinely passes today, but a skip-on-empty escape hatch on the PR's single most important regression test means an id remapping would silently disarm it rather than fail it.
What I tried hard to break and could not
1. The unknown_condition catch-all is not the Autre crop defect. This was my main line of attack. Decisive test: rebuild every wave with an identity condition map (code_<n> for every integer, so no two distinct codes can share a label) and diff row counts against the PR's map. If any two distinct source codes were landing on one tuple, the identity build would carry more rows. Result: 133,683 vs 133,683 — zero rows lost, in every single wave. I validated the harness rather than trusting it (asserted the code_* labels actually appeared; my first attempt silently no-op'd and I caught it). So the sentinel absorbs only genuinely-NA codes plus off-scheme codes that never co-occur on a tuple, and the only residual merging it causes is NA+NA — unresolvable without inventing data. The catch-all is non-injective in principle and injective in fact.
2. The 2009-10 / 2010-11 inference is correct — and I tested it in a way that could have come out backwards. Cosine similarity of each unlabelled column's code-frequency vector against the fully-labelled 2011-12 reference:
| column | vs CONDITION | vs UNIT | verdict |
|---|---|---|---|
2009-10 a5aq6b |
0.997 | 0.133 | condition |
2009-10 a5aq6c |
0.149 | 0.931 | unit |
2010-11 a5aq6b |
0.994 | 0.128 | condition |
2010-11 a5aq6c |
0.158 | 0.729 | unit |
The two reference vectors are near-orthogonal (cos = 0.110), so the test genuinely discriminates rather than returning "similar to everything". Independently, 2009-10's a5aq6c codes behave like physical units — median reported quantity orders exactly as it must: Sack(100kg) 2.0, Sack(120kg) 3.0, Basket(5kg) 13, Bunch(Medium) 30, Kilogram 50, Number of Units 70. The fix is not inverted for those two waves. The same test also confirms 2018-19's unlabelled a5aq6c is a second condition column (0.976 vs 0.127), so preferring the labelled a5aq6b there is right.
3. Conservation is real and is not hiding a deletion. Cold builds of both trees reproduce the PR's table exactly, wave by wave: 130,481 → 133,683 rows; Quantity 317,005,989.0; Quantity_sold 18,134,251.4; Value_sold 9,066,365,499.8 — identical on both sides. I then instrumented the pre-collapse frame and counted null index keys under the 6-level and 7-level keys on the same frame, so the comparison is exact: identical in every wave (2009-10: 431 rows / 7,108,132.2 units, plot only; 2011-12: 43 / 962.5; all others zero). condition contributes exactly zero new null-key deletions. The sentinel fill does what it claims.
One caveat the PR doesn't draw: 2009-10's Quantity total is 99.6–99.8% the unstripped 99999 sentinel (3,169 rows carrying ~316.9M of the 317.0M panel total), so for that wave the conservation check has very little power to detect a real error. The PR flags the sentinel as known issue #4 but doesn't connect it to the weakness of its own headline number.
4. D1 compliance is clean. No reducer added; no aggregation: key anywhere in Uganda's data_scheme.yml. The only groupby/sum tokens in the diff are comments. The pre-existing collapse is unchanged in behaviour — it merely gained a level. This is an identifier fix, which is what D1 asks for.
5. The Feature('crop_production') claim is vacuous — and the PR says so itself. I read all 14 countries' declared index from config: modal is (t, i, plot, crop, u) (8 countries); Uganda's is (t, i, plot, j, u, condition, season), unique. Uganda was already excluded pre-fix at (t,i,plot,j,u,season) — it uses j where the modal uses crop, and carries a season nobody else has. So "byte-identical, 250,692 rows" was never going to be anything else: the cross-country risk was not cleared, only not exercised. Worth stating for the future harmonisation PR: if Uganda were registered in index_info today, condition would be an 8th level the other 13 countries lack, and Feature assembly would have to either drop it — re-summing fresh onto dry in the pooled table, reintroducing this exact bug one layer up — or broadcast NA into it for everyone else. That hazard should be written into the harmonisation issue now, while the reason is fresh. (I did not build Feature across 14 countries cold; this is from config plus the PR's own measurement.)
6. The headline example is real, and the negative control reproduces. Country-level, cold cache:
t i plot j u condition season Quantity harvest_month
2011-12 1033000506 1033000506-1-6 Coffee Kg dried_in_pods A 240.0 6
2011-12 1033000506 1033000506-1-6 Coffee Kg fresh_in_pods A 100.0 6
On the pre-fix tree with the Uganda cache physically removed: 6 failed, 3 errors, 1 passed — exactly as claimed. The single pre-fix pass is test_index_is_unique, a guard rather than a test of the fix. Tests read schema from data_info.yml / data_scheme.yml / categorical_mapping.org; nothing hardcoded, per the house rule. I also confirmed the mechanism the Columns: declaration leans on — _enforce_canonical_spellings does rewrite index levels (country.py:4021-4022, df.rename(index=..., level=col)) — though with every variant list empty it is documentary today, which the PR states plainly.
7. The 2019-20 _1/_2 reading is right, and the 18 residual groups are fully explained. The Stata variable labels read literally 6c. Condition/state of crop harvested (2018, full harvest, condition1) / (condition2) — condition slots, not ordinal harvests. Of 1,382 season-A rows with both slots populated, 133 report the same condition code, and 18 of those also share the unit — which is exactly the 18 residual duplicate groups reported for that wave.
8. The scope calls check out. 2018-19 AGSEC5B's unwired unit is real (7,041 non-null, 40 labels; all 7,041 built rows carry u='Unknown') but cannot interact with this fix — 2018-19 has 0 duplicate groups before and after, so an extra unit level can only keep it at 0. 2019-20 season B's dropped _2 slot is real (1,306 non-null conditions, labelled "(2019, condition2)"); it leaves that wave incomplete, so its residual-collision count will move when wired, but it invalidates nothing here because both sides of the before/after read the same extraction.
Summary
The identifier is right, the decode is right, the inference on the two unlabelled waves is right and independently confirmed, mass is conserved, and no new nulls are introduced. My residual-group counts differ slightly from the PR's (426 vs 459 post-fix; 3,611 vs 3,654 pre-fix) purely because I excluded null-plot rows before grouping — same direction, same ~88% reduction, and my harvest_month sub-count lands on 50 exactly as reported.
I'd fix F1's comment before merge (it is the stated basis for a merge on an index level, and it is false), and I'd take F4 as a follow-up since it is cheap and guards the exact failure mode this fix is most exposed to. F2 and F3 are comment corrections.
No cells blessed, correctly — nobody has read per-condition numbers in analysis yet.
|
Addendum — regression suites confirmed green under the same cold isolated setup.
(Slightly smaller counts than the PR's 454/60 because I omitted This does not change the verdict above — APPROVE-WITH-NOTES, with F1 (the code-99 comment) worth correcting before merge and F4 (the silent-sentinel test gap) worth a follow-up. |
…ilent-colmap test gap Adversarial review of PR #649 returned APPROVE-WITH-NOTES: the data path survived every attack; three comments were inaccurate and one test gap was real. Every number below was re-measured independently on a cold build (isolated LSMS_DATA_DIR with only dvc-cache symlinked in, LSMS_COUNTRIES_ROOT pinned to the worktree and asserted, Uganda cache physically removed, no dvc CLI), not copied from the review. F1 (MEDIUM) — the code-99 justification was false by ~100x. categorical_mapping.org claimed 99 was "only 3 rows in the whole panel and none after 2011-12". Measured: 381 raw source rows carry code 99 with a reported measure and 340 built rows carry `other_condition` (1 / 0 / 120 / 32 / 32 / 32 / 123 by wave), of which 219 are AFTER 2011-12. Code 99 also means three different things — 'Others' (2011-12), 'Not Applicable' (2018-20), and nothing at all in 2013-14 / 2015-16, whose 19-code label sets omit it though 32 rows per wave use it. BEHAVIOUR UNCHANGED: the merge is kept and the real justification is now stated (t is an index level so no wave-crossing tuple exists and nothing is summed; 99 is the scheme's own residual slot in every vintage; splitting would need a third, invented value for 2013-14/2015-16). The split alternative is recorded as a follow-up that moves 340 rows. F2 (LOW) — data_scheme.yml said the sentinel was "~1%". Measured 7 994 / 133 683 = 6.0% panel-wide and 23.2% in 2009-10. "~1%" is the OFF-SCHEME rate in the two unlabelled waves only (1.1% / 0.9%, 492 rows panel-wide). CONTENTS.org already had the right figures; the summary now agrees with it. F3 (LOW) — `Source Label` is not verbatim. Of the 216 (code, wave, season) label pairs, 60 match the org column literally and 156 do not. It is the 2018-20 wording with whitespace collapsed, except codes 24, 32, 42 and 99, which take the longer 2009-16 phrasing; 99's text appears in no wave at all. Code 45's PREFERRED label `dried_grain` is likewise an inference from grid position, covering 30% of rows — now stated rather than implied. F4 (MEDIUM) — a mis-wired colmap now fails loudly. Two independent guards: * uganda._require raises CropColmapError when a colmap NAMES a column the source lacks. `None` stays the declared way to say "no such column". Measured no-op: exactly one named column failed to resolve panel-wide (2010-11's intercrop flag 'a4aq3', absent from that file — set to None, itself a no-op), and a cold rebuild is byte-identical afterwards: 133 683 rows, Quantity 317,005,989.0, Quantity_sold 18,134,251.4, Value_sold 9,066,365,499.8, intercropped non-null 75 625, frames .equals(). * Three tests with measured thresholds — the colmap must RESOLVE against the raw source (S3-guarded), no (wave, season) may exceed 40% unknown_condition (worst honest cell 25.7%), and every cell must decode >= 10 distinct non-sentinel conditions (measured minimum 18). Mutation-proved. `condition: 'a5aq6b_TYPO'` (2018-19 A): was 10 passed, now 1 failed + 8 errors. `condition: 's5aq06f_1'` (an existing but wrong integer column): was 10 passed, now 1 failed + 12 passed — and informatively, the sentinel share is only 33.7%, UNDER the ceiling, so the variety floor is what catches it. Neither invariant alone suffices; both are kept. Also hardened: test_fresh_and_dry_no_longer_collide's skip-on-empty is now an assert, and the crop_production fixture re-raises a build failure when S3 credentials are present (still skips without them, so the data-free CI job is unaffected). Found while doing F4, NOT raised by the review: crop_production.intercropped is wired to the SEED-USE question in every wave that populates it (a4aq3 / a4aq16 / s4aq16, all "did you use any seed/seedlings?", {1: Yes, 2: No}), not to the cropping-system question (a4aq7 "Cropping system", or a4aq8 / s4aq08 "What type of crop stand was on the plot?"). Agreement with the true crop-stand answer is 48.5-52.5% — a coin flip. Rewiring moves data, so it is filed as a Known Issue in Uganda/_/CONTENTS.org, not fixed here. Verification: tests/test_uganda_crop_condition.py 13 passed from a physically cleared cache (was 10). Full sweep with test_schema_consistency, test_canonical_spellings, test_table_structure, test_uganda_tables, test_uganda_invariance, test_uganda_v_grain_invariants, test_quantity_kg: 627 passed, 60 skipped, exit 0. No `aggregation:` key added; no edit under lsms_library/*.py outside countries/Uganda/. Refs #637, #647. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Su5JKX3wKChyfMAdXrdCTr
All four findings applied —
|
| wave | 2009-10 | 2010-11 | 2011-12 | 2013-14 | 2015-16 | 2018-19 | 2019-20 | total |
|---|---|---|---|---|---|---|---|---|
other_condition |
1 | 0 | 120 | 32 | 32 | 32 | 123 | 340 |
219 after 2011-12. Raw source rows carrying code 99 with a reported measure: 381 (I count 381 rather than your 365 — I kept a row if any of qty / qty_sold / value_sold was present, which is the filter the build itself uses; on the narrower "qty only" reading we'd converge). Vintage semantics confirmed from the Stata metadata: Others in 2011-12, Not Applicable in 2018-19/2019-20, and absent from the label set in 2013-14/2015-16 — both ship 19 codes, no 99, while 32 rows per wave use it.
I did not change behaviour. The merge is kept, and the corrected numbers do not undermine it — but the stated reason had to be replaced, because the old one was false:
tis an index level, so a 2011-12 "Others" tuple and a 2019-20 "Not Applicable" tuple can never coincide. Nothing is summed, aggregated or de-duplicated across the vintage boundary; the merge is a naming choice and the vintage stays recoverable fromt.- 99 is the scheme's own residual slot in every vintage — the code the instrument uses when the dryness × form grid does not apply.
other_conditionnames the slot without asserting which residual reading applies. - Splitting would need a third value for 2013-14/2015-16, where no wave says what 99 means — i.e. inventing a label the source does not carry, which is what
unknown_conditionexists to avoid.
The split you offered as the alternative (other_condition for 2011-12 / not_applicable_condition for 2018-20, sentinel for the rest) is recorded as a follow-up: it moves 340 rows between index values, so it needs its own before/after.
One more inaccuracy in the same paragraph, which you flagged in passing but which the comment still asserted: "The CODES are bit-identical in every wave that carries value labels." They are not — 2013-14 and 2015-16 ship 19, not 20. Corrected.
F2 — corrected
Sentinel share 7,994 / 133,683 = 6.0% panel-wide, 23.2% in 2009-10 (then 4.4 / 3.5 / 2.3 / 0.1 / 0.1 / 0.1%). The "~1%" was the off-scheme rate and only in the two unlabelled waves: 297 = 1.1% of 2009-10, 192 = 0.9% of 2010-11, 492 rows panel-wide (your 489 — same measurement, mine taken pre-collapse). data_scheme.yml now carries the true figures and says explicitly which rate is which.
F3 — corrected, and the four codes are confirmed
Of the 216 (code, wave, season) label pairs across the five labelled waves, 60 match the org column literally and 156 do not. It is the 2018-20 wording with whitespace runs collapsed, except exactly the four you named — 24, 32, 42, 99 — which keep the longer 2009-16 phrasing. Code 99's text (Others / Not Applicable) is a composite that appears in no wave at all.
Your point on code 45 is taken and written down: every labelled wave writes exactly Dry - grain, and dried_grain is an inference from grid position (35 already occupies "Dry at harvest - grain"). Measured coverage 40,411 / 133,683 = 30% of all records, so it is load-bearing and now says so.
F4 — closed, with a mutation proof in both directions
Two guards, because they catch different mistakes.
1. The silent fallback now raises. uganda._require → CropColmapError when a colmap names a column the source lacks. None stays the declared, auditable way to say "this wave records no such column" (2018-19 season A genuinely has no harvest unit). Applied uniformly to every per-slot and id column, not just condition — a mis-typed qty drops a whole slot, which is worse.
Measured before changing it, per your standard: across all 7 waves × 2 seasons × every slot, exactly one named column failed to resolve — and it was a live bug (see below). With that fixed, a cold rebuild is byte-identical: 133,683 rows, Quantity 317,005,989.0, Quantity_sold 18,134,251.4, Value_sold 9,066,365,499.8, intercropped non-null 75,625, sorted frames .equals() → True.
2. Three tests, thresholds set from measurement and with their provenance in the source:
test_crop_colmap_columns_resolve_in_source— a named column must exist (reads raw Stata, S3-guarded via the house_aws_creds_available()pattern; PR test: one creds guard, and a net for the modules that forget it #648'sconftest.pyis not ondevelopmentyet).test_sentinel_share_bounded_per_wave_season— ceiling 40%; worst honest cell is 2009-10/A at 25.7%.test_condition_varies_within_every_wave_season— floor 10 distinct non-sentinel values; measured minimum 18.
Mutation proof (Uganda cache physically cleared before each run):
| mutation | before | after |
|---|---|---|
| baseline | 10 passed | 13 passed |
condition: 'a5aq6b_TYPO' (your mutation) |
10 passed | 1 failed, 4 passed, 8 errors |
condition: 's5aq06f_1' — existing but wrong |
10 passed | 1 failed, 12 passed |
The second is the one worth reading. Pointing 2018-19/A's condition at the harvest-month column (a plausible 6f/6b fat-finger) builds cleanly, passes the resolve test, and leaves the sentinel share at 33.7% — under the 40% ceiling. The variety floor is what catches it, at 2 distinct conditions. So a sentinel ceiling alone is not sufficient, and I deliberately did not tune the threshold down to pretend otherwise; that counter-example is recorded next to the constant.
I also took your smaller note: test_fresh_and_dry_no_longer_collide's skip-on-empty is now an assert. And the crop_production fixture re-raises a build failure when S3 credentials are present — without them it still skips, so the data-free CI job is unaffected. Otherwise your own mutation would have produced eight quiet skips, which is the same silent-failure mode the tests exist to close.
What the review missed: intercropped is wired to the seed-use question
Found while measuring the blast radius of the strict check — I read the Stata variable labels of every column CROP_COLMAPS[...]['intercrop'] names, not just whether they resolve.
intercropped is built from intercrop.flag with 2 -> True. In every wave that populates the column, that flag points at the seed-use question:
| wave | wired flag | its variable label | value labels |
|---|---|---|---|
| 2011-12 | a4aq3 |
Did you use any seed or for this crop? | 1 Yes, 2 No |
| 2013-14 | a4aq16 |
A4AQ16: Did you use any seed or seedlings...? | 1 Yes, 2 No |
| 2015-16 | a4aq16 |
A4aq16: did you use any seed or seedlings...? | 1 Yes, 2 No |
| 2018-19 | s4aq16 |
16. Did you use any seed/seedlings? | 1 Yes, 2 No |
| 2019-20 | s4aq16 |
16. Did you use any seed/seedlings? | 1 Yes, 2 No |
The actual cropping-system question is a different column in the same file, present in every wave: a4aq7 "Cropping system" {1: Pure Stand, 2: Inter cropped} in 2009-10/2010-11, a4aq8 / s4aq08 "What type of crop stand was on the plot?" {1: Pure Stand, 2: Mixed Stand} from 2011-12 on.
The two are near-independent — (wired == 2) agrees with (crop-stand == 2) on 48.5 / 48.8 / 52.5 / 51.8 / 52.5% of rows for 2011-12 … 2019-20. A coin flip. Anything using crop_production.intercropped today is using the seed-purchase question.
And the 2010-11 case is exactly the F4 failure mode, live: flag: 'a4aq3' names a column that does not exist in that wave's AGSEC4A (it ships HHID, prcid, pltid, cropID, a4aq7..a4aq14), so the old in df4a.columns guard swallowed it and intercropped was NaN on all 20,970 rows with nothing said. Set to None here — a provable no-op that makes the config state what the build already did, and lets the strict check apply uniformly. Rewiring to the real question adds and moves data (it would newly populate 2009-10 and 2010-11), so it is filed as a Known Issue rather than folded into a comment-and-tests commit.
Your F4 mutation only touched condition; this one was sitting in the same dict.
Verification
tests/test_uganda_crop_condition.py: 13 passed from a physically cleared cache (was 10). Full sweep — test_schema_consistency, test_canonical_spellings, test_table_structure, test_uganda_tables, test_uganda_invariance, test_uganda_v_grain_invariants, test_quantity_kg: 627 passed, 60 skipped, exit 0 (10m33s, cold). The only warnings are the pre-existing Uganda cluster_features GrainCollapseWarnings, unrelated to this table.
Constraints held: no aggregation: key; no edit under lsms_library/*.py outside countries/Uganda/; data_info.yml untouched this round; no dvc CLI. Still no cells blessed — nobody has read per-condition numbers in analysis.
Full write-ups in Uganda/_/CONTENTS.org ("Code 99 is merged across vintages", "Source Label is a normalized representative", "A mis-wired colmap must fail loudly", and two new Known Issues) and .coder/ledger/323-uganda-crop-condition.md §Phase 4.
…ions did not follow Coordinator correction: I applied F1-F3 without reading Uganda/_/CONTENTS.org end to end — only the sections PR #649 itself had added. Re-read in full. No contradiction was found (nothing pre-existing asserts anything about code 99's incidence, the sentinel rate, or the unlabelled 2009-10/2010-11 codes, and the file's only LOGBOOK entry is on 2019-20 panel weights), but one piece of genuine prior art was missed, and it reframes F3. CONTENTS.org "Unit handling for food_acquired" — which pre-dates the condition work and was untouched by b28b502 — already records the house convention for cross-wave label drift: the `u` table carries ONE COLUMN PER WAVE (2005-06 ... 2019-20) holding that wave's raw questionnaire string, "unused at runtime but preserv[ing] the cross-wave provenance". So categorical_mapping.org holds three shapes for one job: `u` (per-wave columns, drift representable), `harvest_units` (Code + Preferred Label, drift not recorded), and `harvest_conditions` (Code + Preferred Label + a single `Source Label`, drift LOSSY). That single column is not merely where the "verbatim" claim went wrong — it is why it could go wrong: one cell cannot hold two vintages' wordings, so codes 24/32/42 silently took the 2009-16 text and 99 became a hand-made composite. The `u` table has no such failure mode; it preserves each wave's string separately, typos and all ("Small cup wuth handle(Akendo)" survives verbatim in six waves). Documented in Uganda/_/CONTENTS.org (a new subsection under the Source Label correction) and pointed at from the categorical_mapping.org comment, with a concrete recommended follow-up: give harvest_conditions the `u` shape. It is runtime-inert — get_categorical_mapping reads only Code + Preferred Label — and the 216 measured label pairs already recorded are exactly the data needed to populate it. NOT done here: the review scoped F3 to a prose correction and a table reshape is the maintainer's call, not a side effect of one. Docs only; no behaviour change. tests/test_uganda_crop_condition.py: 13 passed from a fresh isolated LSMS_DATA_DIR (cold rebuild confirmed). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Su5JKX3wKChyfMAdXrdCTr
Addendum —
|
| table | shape | per-wave drift |
|---|---|---|
u |
Code, Preferred Label, one col per wave | representable |
harvest_units |
Code, Preferred Label | not recorded |
harvest_conditions |
Code, Preferred Label, Source Label |
lossy |
The single Source Label column is not merely where the "verbatim" claim went wrong — it is why it could. One cell cannot hold two vintages' wordings, so codes 24 / 32 / 42 silently took the 2009-16 text and 99 became a hand-made composite that appears in no wave. The u table has no such failure mode: it keeps each wave's string in its own column, typos and all — Small cup wuth handle(Akendo) survives verbatim across six waves. That is what "verbatim" is supposed to mean, and it is what a single column structurally cannot deliver.
Recommended follow-up (documented, not done)
Give harvest_conditions the u table's shape: replace Source Label with one column per labelled wave (2011-12, 2013-14, 2015-16, 2018-19, 2019-20; 2009-10 and 2010-11 stay blank, since they ship no value labels — exactly as the u table leaves blanks). It is runtime-inert (get_categorical_mapping reads only Code + Preferred Label), and the 216 measured label pairs already recorded in this PR are precisely the data needed to populate it.
I did not do it here: you scoped F3 to a prose correction, and a table reshape is your call rather than a side effect of one. Say the word and it's a small, mechanical change.
Written up in Uganda/_/CONTENTS.org (new subsection under the Source Label correction), pointed at from the categorical_mapping.org comment so the next reader hits it at the table rather than having to find this thread, and recorded in .coder/ledger/323-uganda-crop-condition.md.
Docs only, no behaviour change. tests/test_uganda_crop_condition.py: 13 passed from a fresh isolated LSMS_DATA_DIR (cold rebuild confirmed — no reuse of the earlier run's cache).
The defect
The UNPS post-harvest module asks question 6 as one compound question — how much did you harvest, in what unit, and in what condition/state. A household that takes some of a crop off a plot fresh and the rest after drying it answers twice.
crop_productionwas keyed(t, i, plot, j, u, season)with no level for the condition, so those answers landed on the same index tuple and the de-duplication block at the end ofuganda.crop_production_for_wavesummed them. Dry weight was added to fresh weight.340 is not a quantity of anything. 3 654 index groups were affected. The quietest instances were 2009-10 and 2010-11: those waves have no harvest-month column, so every one of their collisions looked like an exact duplicate of an identical record.
The fix
Grain is now
(t, i, plot, j, u, condition, season).Both vintages use the same 20 integer codes.
a5aq6b/a5bq6b_1/_2slots2019-20's
_1/_2are not an ordinal "first/second harvest": the Stata variable labels read literally6c. Condition/state of crop harvested (2018, full harvest, condition1)/(… condition2). So oneharvest_conditionstable incategorical_mapping.orgserves every wave — no harmonisation between vintages was needed or invented, only cosmetic label drift (en-dash vs hyphen, mojibake in 2015-16 AGSEC5B, code 99 reading "Others" in 2011-12 and "Not Applicable" in 2018-20).Labels encode the scheme's own 2-D cross: a dryness family (
green/fresh/dry_at_harvest/dried) × a physical form (_with_shell_and_stalk/_with_shell/_in_cob/_in_pods/_grain).The canonical cross-country vocabulary is declared in
lsms_library/data_info.ymlthe wayplot_features.Tenureis —type+ prosenote+spellingswith empty variant lists, carrying the house "countries may extend this list rather than force-fit a local category" stance._enforce_canonical_spellingsapplies spelling maps to index levels, not just columns (country.py:3962), which is what makes aColumns:declaration legitimate for an index level.conditionis sentinel-filled (unknown_condition) rather than left NA, because the collapse'sgroupby(level=...)defaults todropna=Trueand silently deletes rows with a null index key.Verification of the
a5aq6b/a5aq6cclaimChecked per wave against the Stata metadata, deciding by the value-label vocabulary, never the variable label. The old comment's claim that "the WB .do's A5aq6b/A5aq6c unit/condition rename is inverted for these actual UNPS files" is true of exactly one file — 2013-14 AGSEC5A. Every other wave labels its variables correctly. The wiring (
6c= unit) was right in all cases; only the explanation was overstated. Corrected in place; the full per-wave table is inUganda/_/CONTENTS.org.2009-10 and 2010-11 carry no value labels at all; their mapping was confirmed from code distributions (6b's modes are 20/45/24 = the condition scheme's modes; 6c's are 22/1/10 = Plastic Basin / Kg / Sack 100 kg).
Measured (cold, isolated
LSMS_DATA_DIR)Collisions fall 87.4%.
Quantity,Quantity_sold(18,134,251.4) andValue_sold(9,066,365,499.8) totals are identical per wave before and after — summing is conservative, so a correct fix moves rows apart without creating or destroying mass.Residual 459 groups are now all commensurable (same crop, same unit, same condition), so summing them is arithmetically valid — precisely what was not true before. 50 groups' rows differ in
harvest_month; ~320 differ only inQuantity; 49 are byte-identical source rows; 37 carry theunknown_conditionsentinel and so may still hide a real difference.Feature('crop_production')Byte-identical before and after: 250 692 rows, the same 8 kept countries, the same modal index
['country','i','t','v','plot','crop','u','currency'], the same 6 excluded frames[Ethiopia, EthiopiaRHS, Mali, Nigeria, Tanzania, Uganda].Uganda was already modal-excluded — its shape
['i','t','v','plot','j','u','season']diverges from the modal['i','t','v','plot','crop','u']— so a 7th level changes nothing.crop_productionstays out ofindex_info, per the standing note there that the plot-level ag features need cross-country index NAME harmonisation (plotvsplot_id,cropvsj, across 14 countries) before registration. That is a separate PR.Tests
tests/test_uganda_crop_condition.py, 10 tests, schema rules read fromdata_info.yml/data_scheme.yml/categorical_mapping.org, never hardcoded.Negative control — on the pre-fix tree (stashed working copy, Uganda cache physically cleared): 6 failed, 3 errors, 1 passed, including
Post-fix, from a cleared cache: 10 passed. Related suites (
test_schema_consistency,test_canonical_spellings,test_table_structure,test_uganda_tables,test_uganda_invariance,test_uganda_v_grain_invariants,test_quantity_kg): 454 passed, 60 skipped.Deliberately NOT in this PR
Four adjacent defects found while verifying — each moves data, so each needs its own before/after. All four are written up in
Uganda/_/CONTENTS.org"Known Issues":a5bq6bhas the full 40-labelharvest_unitsvocabulary and 7 041 non-null values, but the colmap setsunit: None, so every 2018-19 season-B row carriesu='Unknown'. The long-standing note "2018-19's harvest side carries no unit label" is correct for 5A and wrong for 5B.s5bq06a_2/b_2/c_2, labelled "(2019, condition2)", 1 306 non-null conditions, absent from the table. 2018-19's_2slot is labelled "second season of 2017" and may be a season slot — needs the questionnaire (question: Uganda 2019-20 crop_production silently drops the PART-harvest columns (s5aq06*_11/_21) #647).plotid. Pre-existing;conditionis sentinel-filled precisely so as not to add to it.99999sentinel — that wave's total is ~300× every other wave.Also out of scope per the issue: the part-harvest columns
s5aq06a_11/_21(#647), anyaggregation:YAML key (D1: dead config), and any reducer (D1: a new index level or nothing).Follow-up the maintainer should weigh
spellings:declares the vocabulary and rewrites known variants, but nothing rejects a value outside it — the precedent for an enforced index-level enumeration is a code constant (transformations.S_VALUESfors), which is core and out of scope for a config-only PR. A follow-up should either add aCONDITION_VALUESconstant alongsideS_VALUES, or — better — drive an index-level enumeration check fromdata_info.ymlsosstops being special-cased. Recorded in.coder/ledger/323-uganda-crop-condition.md§6.No cells blessed: no human has read the per-condition numbers in real analysis.
Refs #637, #647.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Su5JKX3wKChyfMAdXrdCTr