fix(#637): Uganda plot_inputs grain needs season; key-soundness review of six sites

[ligon]

Key-soundness review of the six GH #637 groupby().first() sites outside the EHCVM people_last7days cluster.

Per the corrected reading on #637: the per-column NA-skip is a correct completion, and the composite is wrong only when the key merges different real entities. So this reviews the six keys, not .first(). No .first(skipna=False), no aggregation: keys, no core changes.

All measurements are cold builds (LSMS_NO_CACHE=1 + isolated LSMS_DATA_DIR, dvc-cache symlinked; no dvc CLI invoked).

Verdicts

site	duplicate groups	exact	complementary	conflicting	verdict
Burkina_Faso/2014/_/food_coping.py	0	0	0	0	sound
Burkina_Faso/2014/_/months_food_inadequate.py	0	0	0	0	sound
EthiopiaRHS/_/ethiopiarhs.py:443	0	0	0	0	sound
Uganda/_/uganda.py:1470 (plot_inputs)	1,342	1,300	0	42	broken — fixed here
Niger/_/niger.py:1064 (people_last7days)	210	34	6	170	broken — wider than the site
Uganda/_/uganda.py:1088 (crop_production)	3,654	2,709	112	833	broken — wider than the site

Totals over all waves; Niger's are entirely from 2014-15.

Read the exact column carefully. For the two broken Uganda keys, "exact" means only that the payload columns happened to agree — the rows still came from different entities (a season-A vs a season-B application; a fresh vs a dry harvest record), and their Quantity was still summed. Conflict counts are therefore a lower bound on the entity merge, not a measure of it. crop_production shows this starkly: 2009-10 and 2010-11 have no month column at all, so 100% of their groups classify "exact" while fresh and dry quantities are being added together.

Sound (3 of 6)

Burkina Faso 2014, both tables. i = unpadded zd + 3-padded menage, which would be ambiguous if menage reached 1000 (zd=1,menage=1001 and zd=11,menage=001 both give '11001'). It ranges 1..17 over 900 zd. 10,230 source rows → 10,230 distinct (zd, menage) → 10,230 distinct i, zero values mapping to more than one household. food_coping 51,099 rows / months_food_inadequate 10,230 rows, zero duplicate index tuples. Both guards are dead code today.

EthiopiaRHS crop_production. Unique by construction: the area_output_* files are wide one-row-per-household frames melted to at most one row per (i, crop stub) with a constant u='Kg'. 6 waves, 17,523 rows (3,404 / 1,967 / 3,350 / 3,021 / 2,998 / 2,783), zero duplicates in every wave. The "defensive de-dupe" comment is accurate.

Each of these three sites gets a comment recording what was checked and the counts.

Broken and fixed: Uganda plot_inputs

AGSEC3A and AGSEC3B are two distinct plot-SEASON questionnaires, and the same plot id appears in both — but the key (t, i, plot, input, j) had no season level. 89% of the collisions were season-A/season-B pairs — 1,196 of 1,342 (2009-10: 230 of 242 index tuples present in both seasons; 2013-14: 112 of 112, i.e. every collision in that wave). Corrected 2026-07-21: the first draft said "~95%" and "112 of 122"; neither reproduces.

33 groups across the seven waves reported the same input in different units, so kilograms were summed with litres — per wave 5 / 6 / 0 / 5 / 4 / 3 / 10 over 2009-10 … 2019-20. Corrected: the first draft said "6 per wave"; it is not constant, and 2011-12 has none at all.

HH 1063000910, plot 1063000910-1-1, Insecticide
  season A:  70.0  Litre
  season B:   1.0  Kg
  old key ->  ONE row, Quantity 71.0, u 'Litre'

71 exists nowhere in the survey, and the unit is wrong for part of it.

The repo had already decided this key. The sibling plot_labor builder reads the same two files and carries season, with the reason in Uganda/_/data_scheme.yml:

season — 'A' (first season) / 'B' (second season). Carried in the grain (as in crop_production) because the same plot id appears in BOTH AGSEC3A and AGSEC3B for ~64% of plots … collapsing the two seasons into one (plot, source) row would require summing across seasons (a transformation).

crop_production carries it too. plot_inputs was the only one of the three UNPS plot-level features that did not. Fix is to the identifier, per #323 D1.

Grain is now (t, i, plot, input, j, season):

wave	before	after	recovered
2009-10	13,763	13,993	+230
2010-11	13,375	13,554	+179
2011-12	12,196	12,353	+157
2013-14	11,721	11,834	+113
2015-16	11,889	11,981	+92
2018-19	10,195	10,367	+172
2019-20	9,880	10,134	+254
total	83,019	84,216	+1,197

Corrected 2026-07-21 — what the first draft got wrong here

The first draft of this section claimed "the index is now unique in every wave, so the collapse no longer fires at all" and "Quantity totals are unchanged". Both are false. Re-derived cold (isolated LSMS_DATA_DIR, dvc-cache symlinked, frame captured immediately before the de-dup block; no dvc CLI):

wave	built	returned	discarded	NaN-plot deleted	merged away
2009-10	14,124	13,993	131	130	1
2010-11	13,561	13,554	7	0	7
2011-12	12,367	12,353	14	5	9
2013-14	11,834	11,834	0	0	0
2015-16	11,981	11,981	0	0	0
2018-19	10,485	10,367	118	0	118
2019-20	10,137	10,134	3	0	3
total	84,489	84,216	273	135	138

The index is unique in 2 of 7 waves. The collapse still runs in the other five and still discards 273 rows: 135 deleted outright by groupby(dropna=True) because plot is NaN, and 138 merged away across 135 duplicate groups. The residual is pre-existing and much smaller than before (pre-fix the same 84,489 rows returned 83,019, losing 1,470) — but it is not zero, and 84,216 is a post-collapse count, not "every reported line-item".

Quantity totals: identical in six waves; 2013-14 gains +3.0 (335,392.005 → 335,395.005). Cause: i=2113000606, plot=NaN, input=Seed, j=Cassava, Quantity=3.0, u='Sack (100 kgs)'. Pre-fix the collapse fired in that wave and dropna=True deleted the row; post-fix 2013-14's index is unique, the collapse does not run, and the row survives. So one of the "+113 recovered" rows in 2013-14 is not a season split, and whether a NaN-plot row survives depends on whether some unrelated tuple happened to collide. Resurrecting it is the better outcome; the inconsistency is now stated in the docstring and pinned by test_2013_14_nan_plot_seed_row_survives rather than left silent.

And the residual is not "genuinely duplicate". All 135 residual groups are input='Seed' — two AGSEC4A plot-crop rows for the same (plot, crop), i.e. the same missing-identifier defect season just fixed, one level down. 119 carry no reported Quantity at all and 8 carry exactly one (nothing is summed), but 8 groups, all in 2011-12, sum two or three reported quantities and 4 of those sum different values:

2011-12  HH 2083000802  plot -1-2  Seed/Ground Nuts   9.0 + 8.0        -> 17.0 Kg
2011-12  HH 3073002502  plot -1-1  Seed/Beans        20.0 + 4.0        -> 24.0
2011-12  HH 3183000307  plot -2-2  Seed/Other Crop    0.5 + 0.25       -> 0.75
2011-12  HH 3183000310  plot -3-5  Seed/Other Crop   32.0 + 0.25 + 0.25 -> 32.5

No residual group has a unit conflict. Tracked on #637, deliberately not fixed here (naming the missing AGSEC4A row identifier is its own grain decision; per #323 D1 the answer is an index level, never a reducer). The measured residual now replaces the false "what remains is same-season repetition, which is genuinely a duplicate" sentence in uganda.py.

Conservation still holds where it was claimed: 2009-10 is 735,805.08 on both sides, and summing is conservative, so the fix moves rows rather than mass — in six of seven waves.

Seed rows carry season='A': they come from AGSEC4A, the first-season plot-crop roster. AGSEC4B exists in the source but is not read, so there are no season-B seed rows — a pre-existing coverage gap the level now makes visible instead of folding into the season-A rows.

Broken, but the fix is demonstrably wider than the site

Both are recorded as comments at the site with their measurements, rather than half-fixed.

Niger people_last7days — 2014-15 only

2011-12 / 2018-19 / 2021-22 are clean: 25,125 / 35,406 / 44,320 rows, zero duplicates.

ECVMA2 keys households on (GRAPPE, MENAGE, EXTENSION); niger.i builds i from (GRAPPE, MENAGE) only. ECVMA2_MS01P1.dta holds 3,617 distinct (GRAPPE, MENAGE, EXTENSION) households but only 3,558 distinct (GRAPPE, MENAGE) — 59 households have no id of their own, and their members' line numbers collide. 420 rows in 210 colliding (t,i,pid) groups, with EXTENSION differing in 210 of 210 (100%).

These are different real people:

GRAPPE 2, MENAGE 1, line 1  ->  EXTENSION 0: age 56, industry 316
                                EXTENSION 2: age 29, industry 321
GRAPPE 2, MENAGE 1, line 2  ->  EXTENSION 0: age 11
                                EXTENSION 2: age 22

.first() merges each pair into one person who was never enumerated — exactly the failure mode #637 names.

Why not fixed here: dropping EXTENSION is a wave-wide convention. 2014-15's sample, household_roster (same source file, same 210 collisions), individual_education, livestock, crop_production, plot_inputs and plot_labor all key on (GRAPPE, MENAGE). Re-keying only people_last7days would break its join to sample() / household_roster and the panel_ids chain — a regression, not a fix. The correct fix adds EXTENSION to i across the whole 2014-15 wave and re-derives panel_ids.

Uganda crop_production

AGSEC5A/5B record multiple distinct harvest records per plot-crop-season; the key has no level for which record a row is.

2009-16 — records split by harvest condition (a5aq6b), season A:

wave	colliding groups	condition differs
2009-10	257	222 (86%)
2010-11	328	277 (84%)
2011-12	363	339 (93%)
2013-14	254	223 (88%)
2015-16	397	370 (93%)

Adding the condition to the key drops 2011-12 from 363 colliding groups to 24. The rows are not "repeated identical source rows":

2011-12  HH 1033000506  plot -1-6  Coffee  Kilogram (KG)
   240 kg  "Dry after additional drying - in pods or shell/husks"  month 6
   100 kg  "Fresh/raw harvested - in pods or shell/husks"          month 6
   -> summed to 340 kg   (dry weight added to fresh weight)

2019-20 — records split by instance (the _1/_2 column groups: quantity, unit and month of a first and a second harvest). 1,382 of 8,429 source rows report both; 369 share a unit and collide. 370 colliding groups, 211 conflicting on harvest_month.

The conflict counts understate the merge. harvest_month is the only conflict-capable column, and 2009-10/2010-11 have no month column at all — so their condition-merges are invisible to a conflict test while still summing fresh and dry quantities.

Why not fixed here: the missing level means two different things in two questionnaire vintages (a condition vocabulary in 2009-16, an instance ordinal in 2019-20). Naming it is a grain decision for a published table, not a bug fix, and per #323 D1 the answer must be a new index level — never a reducer. Worth its own PR with a maintainer decision on the level's name and vocabulary.

Test

tests/test_gh637_uganda_plot_inputs_season.py, 7 tests: season is an index level; both seasons survive; total and per-wave row counts; the exact 70 L / 1 kg insecticide pair stays separate; the 2013-14 NaN-plot seed row survives (the +3.0); and every 5-key duplicate differs by season.

Changed 2026-07-21 in response to the review:

• The fixture no longer swallows exceptions into pytest.skip. try: ... except Exception: pytest.skip(...) made the whole module vacuous — a raising build is the likeliest way this fix regresses, and a blanket skip reports that as green. It now uses pytestmark = requires_s3 plus tests/conftest.py's pytest_runtest_makereport hook, which converts a NoCredentialsError and only that, and only when credentials really are absent into a skip. Verified: 7 passed with credentials, 7 skipped under LSMS_SKIP_AUTH=1 with s3_creds removed.
• test_keeps_every_reported_line_item → test_total_row_count. 84,216 is the post-collapse row count, not "every reported line-item" (273 are still discarded — see above). The docstring now says so.
• test_no_row_merges_two_seasons_of_the_same_input keeps its assertion but its docstring now says plainly that it is near-tautological (the builder collapses on the 6-key, so any surviving 5-key duplicate must differ on season by construction) and names the tests that actually discriminate. It does still catch one real regression: both files being tagged the same season, via the both.empty → pytest.fail branch.
• The insecticide test pytest.fails instead of skipping when the row is absent — its disappearance is the regression, not a missing fixture.

Negative control — done properly: the change is committed, so git stash would be a no-op. The eight source files were reverted with git checkout origin/development -- …, the isolated LSMS_DATA_DIR was deleted, and the run was LSMS_NO_CACHE=1. All 7 fail:

FAILED test_season_is_an_index_level         - index is ['i','t','v','plot','input','j'],
                                               no `season` level
FAILED test_both_seasons_are_present         - KeyError: 'season'
FAILED test_total_row_count                  - Uganda plot_inputs has 83019 rows,
                                               expected 84216
FAILED test_per_wave_row_counts              - 13763/13375/12196/11721/11889/10195/9880
FAILED test_kilograms_are_not_summed_with_litres
FAILED test_2013_14_nan_plot_seed_row_survives - expected exactly one 2013-14 NaN-`plot`
                                               Seed/Cassava row; got 0
FAILED test_no_row_merges_two_seasons_of_the_same_input

Restoring the branch files and clearing the cache again: 7 passed cold.

Follows the #323 instrument note: asserts row counts and the specific merged rows, never post-collapse index uniqueness (which holds by construction and would pass with the bug present).

Scope

Config/script only — no lsms_library/*.py touched. No dvc CLI invoked (lock-free get_dataframe() only). Tanzania and Malawi untouched.

tests/ (this module + gh323_grain_contract + gh323_explicit_reducers + schema_consistency + uganda_tables + uganda_v_grain_invariants + id_format_id_canonical), re-run 2026-07-21: 283 passed, 13 skipped, 0 failures. The GrainCollapseWarnings in the run are pre-existing #323 Uganda/cluster_features findings, unrelated to this change.

Refs #637.

🤖 Generated with Claude Code

[ligon]

Adversarial review — PR #646

Independently re-measured cold (isolated LSMS_DATA_DIR with dvc-cache symlinked, LSMS_COUNTRIES_ROOT pointed at a worktree of this branch and separately at origin/development; no dvc CLI invoked). Verdict: FIX_FIRST — the fix itself is right and I could not break it, but three stated measurements do not reproduce, and one of them is now baked into a permanent source comment that tells the next reader the site is clean when it is not.

What reproduced exactly

claim	measured
pre-fix 83,019 → post-fix 84,216 rows	✅ exact, cold, both trees
per-wave deltas 230/179/157/113/92/172/254	✅ exact
2009-10 collisions "230 of 242"	✅ exact (242 dup groups on the 5-key, 230 cross-season)
all-wave totals "1,342 duplicate groups"	✅ exact (242+186+165+112+92+288+257)
the 70 L / 1 kg insecticide pair	✅ verified in the raw: AGSEC3A HHID 1063000910 parcel 1 plot 1 → a3aq28a=2, a3aq28b=70; AGSEC3B same plot → a3bq28a=1, a3bq28b=1. Genuinely two records in two different files.
Burkina Faso both sites sound	✅ exact: 10,230 rows, menage 1..17 over 900 zd, 10,230 distinct i, 0 many-to-one, 51,099 melted rows / 51,099 distinct tuples
EthiopiaRHS crop_production sound	✅ exact: 3,404 / 1,967 / 3,350 / 3,021 / 2,998 / 2,783 = 17,523, index.duplicated().sum() == 0 in every wave (instrumented pre-collapse)
Niger 2014-15 broken, wider than the site	✅ exact: 3,617 (G,M,E) vs 3,558 (G,M); 210 groups / 420 rows; EXTENSION differs 210/210; GRAPPE 2 MENAGE 1 line 1 → age 56/ind 316 vs age 29/ind 321, line 2 → age 11 vs 22
negative control (6/6 fail pre-fix)	✅ reproduced against origin/development
AGSEC4B present but unread	✅ AGSEC4B.dta.dvc exists in 2009-10 / 2011-12 / 2019-20

Conservation, checked properly. I re-collapsed the post-fix frame on the old 5-key (Quantity/Quantity_purchased → sum(min_count=1), flags → max, u → first) and compared cell-by-cell to the pre-fix build: 83,019 vs 83,019 rows, index sets identical (the only "differences" are 723 NaN-v tuples that never compare equal), 0 mismatches on all five columns. The fix provably only splits rows. This is the strongest evidence in the PR and it holds.

I also checked the failure modes the house rules name. Not splitting real entities: the two files are separate modules with separate variable stems (a3aq* / a3bq*), 9,898 vs 9,789 rows, and only 230/13,993 tuples collide — not wholesale file duplication. Not invariance-by-missingness: season is 100% non-null, A=80,081 / B=4,135. Not a Feature() regression: Uganda's plot_inputs index shape was already unique among the 13 declaring countries (the modal shape is EHCVM (t,i,input,crop,u)), so it was already modal-excluded before this change. D1 respected — identifier, not reducer; no aggregation: key; no lsms_library/*.py outside countries/.

FIX_FIRST — 1. "the collapse no longer fires at all" is false

"The index is now unique in every wave, so the collapse no longer fires at all."

I dumped the frame immediately before set_index in every wave (patched copy, then reverted):

wave	pre-collapse rows	returned	lost	NaN-plot rows	same-season dup groups
2009-10	14,124	13,993	131	130	12
2010-11	13,561	13,554	7	0	7
2011-12	12,367	12,353	14	5	8
2013-14	11,834	11,834	0	1	0
2015-16	11,981	11,981	0	0	0
2018-19	10,485	10,367	118	0	116
2019-20	10,137	10,134	3	0	3
total	84,489	84,216	273	136	146

The index is unique in 2 of 7 waves. The collapse still fires in five, and still discards 273 reported line-items — 130 of them in 2009-10 alone, dropped outright because groupby(level=…) defaults to dropna=True and their plot is NaN. So test_keeps_every_reported_line_item and "84,216 reported line-items" are both measuring the post-collapse number, not the reported number.

The residual is pre-existing (pre-fix lost 1,470 of the same 84,489; this PR strictly improves it), so this is not a defect the PR introduces. What must change is the claim. The line now sitting permanently in uganda.py —

"What remains is same-season repetition, which is genuinely a duplicate."

— is unevidenced and, on the measurements above, partly wrong. 2011-12 has 8 same-season groups of which 4 sum different quantities (e.g. HH 2083000802 plot 1-2 Seed/Ground Nuts: 9.0 Kg + 8.0 Kg → 17.0 Kg), and 2018-19 has 116 groups (Seed/Coffee, up to 3 rows per tuple). That is structurally the same defect this PR just fixed — a missing identifier for which AGSEC4A crop-row — and it deserves the same treatment the PR gave Niger and crop_production: a measured comment saying "still broken, here are the counts, tracked on #637", not "genuinely a duplicate". Given how carefully this PR argues that "exact duplicates" is not reassurance, that sentence is exactly the claim it warns others against.

(The 2018-19 unit conflicts I checked are benign — Quantity=NaN, u='Unknown' against a real value, i.e. absence, not contradiction. Worth noting only that the 'Unknown' sentinel is non-null, so first() cannot skip it; it happens to lose the coin-flip nowhere here because the valued row sorts first.)

FIX_FIRST — 2. "Quantity totals are unchanged" is false in one wave

Per-wave Quantity totals, pre vs post: identical in six waves, 2013-14 gains +3.0 (335,392.005 → 335,395.005). Cause: one row, t=2013-14, i=2113000606, plot=NaN, input=Seed, j=Cassava, Quantity=3.0. Pre-fix the collapse fired in that wave and dropna=True deleted it; post-fix 2013-14's index is unique, the collapse does not run, and the row survives. So one of the "+113 recovered" rows in 2013-14 is not a season split, and the API now returns a row with a NaN index level — in the only wave where that happens, because whether a NaN-plot row survives depends on whether some unrelated tuple happened to collide. Resurrecting the row is arguably the better outcome; the inconsistency and the falsified conservation claim should be stated rather than left as a silent side effect.

Notes (no change required, but they are wrong as written)

• "6 groups per wave reported the same pesticide in different units" (PR body, data_scheme.yml, and uganda.py). Cross-season groups with a genuine unit conflict (both quantities present, Unknown excluded), per wave: 5 / 6 / 0 / 5 / 4 / 3 / 10. It is zero in 2011-12. "6 per wave" is now baked into two permanent comments.
• "2013-14: 112 of 122" → measured 112 of 112. Every 5-key collision in that wave is cross-season.
• "~95% of the collisions were season-A/season-B pairs" → 1,196 / 1,342 = 89%.
• test_no_row_merges_two_seasons_of_the_same_input is close to tautological: the builder guarantees a unique 6-key, so any 5-key duplicate must differ on season. It does catch the "both files tagged season A" regression (via the both.empty → pytest.fail branch), so it is not useless — but it is not testing what its docstring says it tests. The row-count tests are the ones doing the work.

What I tried and could not break

Cold-rebuilt both trees end to end; reconstructed the pre-fix output exactly from the post-fix one (0 mismatches / 83,019 rows); checked the raw .dta for the headline insecticide pair and for the season-A/B file structure; checked for split-real-entities and invariance-by-missingness; verified all three "sound" verdicts (Burkina ×2, EthiopiaRHS) and the Niger "broken but wider" verdict against source, every number exact; verified the negative control fails 6/6 on origin/development and passes 6/6 here; confirmed Feature('plot_inputs') behaviour is unchanged (Uganda was already modal-excluded); confirmed no reducer, no aggregation: key, no core-code change.

The season level is right and should land. Please correct the "collapse no longer fires" and "totals unchanged" claims, replace the "genuinely a duplicate" sentence in uganda.py with the measured residual (273 rows / 146 same-season groups / 136 NaN-plot rows), and fix the "6 per wave" figure in data_scheme.yml and uganda.py.

[ligon]

FIX_FIRST addressed — cd1ce0f1

All three findings confirmed and fixed. I re-derived every number independently before writing it down (cold, isolated LSMS_DATA_DIR with dvc-cache symlinked, LSMS_COUNTRIES_ROOT pointed at a worktree, the pre-collapse frame captured by instrumenting a copy of uganda.py immediately after set_index; the pre-fix side built from origin/development:uganda.py in the same process). No dvc CLI. The review is exact on every figure it reported — I found nothing to dispute.

1. "the collapse no longer fires at all" — confirmed false

wave	built	returned	discarded	NaN-plot deleted	merged away
2009-10	14,124	13,993	131	130	1
2010-11	13,561	13,554	7	0	7
2011-12	12,367	12,353	14	5	9
2013-14	11,834	11,834	0	0	0
2015-16	11,981	11,981	0	0	0
2018-19	10,485	10,367	118	0	118
2019-20	10,137	10,134	3	0	3
total	84,489	84,216	273	135	138

Index unique in 2 of 7; 273 rows still discarded — matching your table row for row.

One refinement, offered as a sharpening rather than a correction: your "146 same-season dup groups" counts duplicate groups including NaN-plot rows, which groupby(dropna=True) deletes before any merge can happen — so those rows are counted once as dropped and again as duplicates. Disjointly: 135 rows deleted for NaN plot (130 in 2009-10, 5 in 2011-12; plot is the only NaN level) + 138 rows merged away across 135 duplicate groups = 273, which reconciles exactly. That is the decomposition I wrote into the source.

2. "genuinely a duplicate" — confirmed false, and worse than "not proven"

All 135 residual groups are input='Seed' — two AGSEC4A plot-crop rows for the same (plot, crop). That is precisely the defect season just fixed, one level down: a missing "which AGSEC4A row" identifier. Character of the 135: 119 carry no reported Quantity at all, 8 carry exactly one, and 8 (all 2011-12) sum two or three reported quantities — 4 of them summing different values:

2011-12  HH 2083000802  plot -1-2  Seed/Ground Nuts   9.0 + 8.0         -> 17.0 Kg
2011-12  HH 3073002502  plot -1-1  Seed/Beans        20.0 + 4.0         -> 24.0
2011-12  HH 3183000307  plot -2-2  Seed/Other Crop    0.5 + 0.25        -> 0.75
2011-12  HH 3183000310  plot -3-5  Seed/Other Crop   32.0 + 0.25 + 0.25 -> 32.5

Your 9.0 + 8.0 -> 17.0 example is exact. No residual group has a unit conflict among rows carrying a real (non-'Unknown') unit — your note that the 2018-19 conflicts are benign absence, not contradiction, holds: 108 of that wave's 116 groups are Quantity=NaN, u='Unknown' throughout. The sentence you flagged is gone; the measured residual replaces it, tracked on #637 and deliberately not fixed here.

3. "Quantity totals are unchanged" — confirmed false in one wave

+3.0 in 2013-14 only (335,392.005 → 335,395.005). The row is exactly the one you named: at the API level i=2113000606 (wave-level H16306-04-01), plot=NaN, input=Seed, j=Cassava, Quantity=3.0, u='Sack (100 kgs)'. Now stated in the docstring and pinned by a new test (test_2013_14_nan_plot_seed_row_survives), which fails pre-fix with "got 0" — so the side effect can no longer flip silently.

Notes you flagged as wrong-as-written — all confirmed

• "6 groups per wave in different units" → 5 / 6 / 0 / 5 / 4 / 3 / 10 (33 total); zero in 2011-12. Corrected in the PR body, data_scheme.yml and uganda.py.
• "2013-14: 112 of 122" → 112 of 112.
• "~95%" → 1,196 / 1,342 = 89%.

Tests

You were right that test_no_row_merges_two_seasons_of_the_same_input is near-tautological. I kept the assertion (it still catches "both files tagged season A" via the both.empty → pytest.fail branch) but the docstring now says so plainly and names the tests that do discriminate.

Two more test problems I found while there, both self-inflicted by the original draft:

• The fixture converted every exception into pytest.skip. That made the whole module vacuous — a raising build is the likeliest regression and would have been reported green. Removed; the module now uses pytestmark = requires_s3 plus conftest.py's NoCredentialsError-only hook. Verified both directions: 7 passed with credentials, 7 skipped under LSMS_SKIP_AUTH=1 with s3_creds moved aside.
• test_keeps_every_reported_line_item was misnamed: 84,216 is the post-collapse count, and 273 reported line-items do not survive. Renamed test_total_row_count, docstring corrected. The insecticide test now pytest.fails instead of skipping when the row is missing.

Negative control, done properly. The change is committed, so git stash is a no-op on it — I reverted the eight source files with git checkout origin/development -- …, deleted the isolated LSMS_DATA_DIR, and ran under LSMS_NO_CACHE=1: all 7 fail, including the new one ("expected exactly one 2013-14 NaN-plot Seed/Cassava row; got 0"). Restored and re-cleared: 7 passed cold.

Wider run: gh323_grain_contract, gh323_explicit_reducers, schema_consistency, uganda_tables, uganda_v_grain_invariants, id_format_id_canonical + this module — 283 passed, 13 skipped, 0 failures. The GrainCollapseWarnings are the pre-existing Uganda/cluster_features #323 findings.

Nothing disputed

I could not find anything the review over-claimed. The three "sound" verdicts (Burkina ×2, EthiopiaRHS) and the Niger "broken but wider" verdict were left untouched — you verified them against source and I had no basis to re-open them. The crop_production comment is likewise unchanged; it was not among the findings.

No code behaviour changed in this commit: the diff is comments, docstrings, the test module, and the PR body.

[ligon]

Independent verification of the FIX_FIRST remediation — cd1ce0f1

Verified READY. I re-derived every number now written into the source myself rather than reading the remediation reply — separate worktree at cd1ce0f1, separate isolated LSMS_DATA_DIR (only contents: a dvc-cache symlink), LSMS_COUNTRIES_ROOT + PYTHONPATH pointed at it with assert 'verify646' in lsms_library.__file__ checked in-process, LSMS_NO_CACHE=1, no dvc CLI. The pre-collapse frame was captured by exec'ing my own instrumented copy of uganda.py (one PRECOLLAPSE.append(df.copy()) after set_index); the worktree's uganda.py was never edited.

Finding-by-finding

#	review finding	status
1	"the collapse no longer fires at all" is false	fixed — replaced by a measured per-wave table
1b	"what remains is same-season repetition, which is genuinely a duplicate"	fixed — replaced by the measured residual
2	"Quantity totals are unchanged" is false	fixed — stated in the docstring and pinned by a new test
3a	"6 groups per wave in different units"	fixed — 5/6/0/5/4/3/10 in both uganda.py and data_scheme.yml
3b	"2013-14: 112 of 122"	fixed — 112 of 112
3c	"~95% cross-season"	fixed — 89%
3d	test_no_row_merges_two_seasons_of_the_same_input near-tautological	fixed — docstring now says so and names the discriminating tests

Every number in the new comment reproduces exactly

My instrumented build, independent of the remediation's:

  2009-10 built=14,124 returned=13,993 disc=131 unique=False nan_plot=130 dupgroups=  1 merged=  1
  2010-11 built=13,561 returned=13,554 disc=  7 unique=False nan_plot=  0 dupgroups=  7 merged=  7
  2011-12 built=12,367 returned=12,353 disc= 14 unique=False nan_plot=  5 dupgroups=  8 merged=  9
  2013-14 built=11,834 returned=11,834 disc=  0 unique=True  nan_plot=  1 dupgroups=  0 merged=  0
  2015-16 built=11,981 returned=11,981 disc=  0 unique=True  nan_plot=  0 dupgroups=  0 merged=  0
  2018-19 built=10,485 returned=10,367 disc=118 unique=False nan_plot=  0 dupgroups=116 merged=118
  2019-20 built=10,137 returned=10,134 disc=  3 unique=False nan_plot=  0 dupgroups=  3 merged=  3
    TOTAL built=84,489 returned=84,216 disc=273  nan_deleted=135  merged_away=138

Row for row identical to the table now in uganda.py. Index unique in exactly 2 of 7 (2013-14, 2015-16). plot is the only NaN index level.

Residual characterisation, also mine: 135 duplicate groups, 135 of them input='Seed' (100%); by number of reported quantities 119 / 8 / 7 / 1 for 0/1/2/3 → 8 groups summing ≥2, of which 4 sum different values — and they are exactly the four in the comment (9.0+8.0→17.0 Kg, 20.0+4.0→24.0, 0.5+0.25→0.75, 32.0+0.25+0.25→32.5, all 2011-12). 0 residual groups with a non-'Unknown' unit conflict.

Old-5-key analysis: 242/186/165/112/92/288/257 = 1,342 duplicate groups, 1,196 cross-season = 89.1%; 2009-10 230 of 242; 2013-14 112 of 112. Unit conflicts 5/6/0/5/4/3/10 = 33, none in 2011-12.

Finding 3's row, at wave level: 2013-14, i='H16306-04-01', plot=NaN, input='Seed', j='Cassava', Quantity=3.0, u='Sack (100 kgs)', and 2013-14's post-fix Quantity total is 335,395.005 — both exactly as documented.

The one place the remediation departed from the review is correct

The reply replaced the review's "146 same-season dup groups / 136 NaN-plot rows" with 135 deleted + 138 merged across 135 groups. I checked which decomposition is right, since it is the version now baked into source:

• 136 is the count of NaN-index rows across all seven waves, but one of them is the 2013-14 row — and 2013-14's index is unique, so the collapse never runs and that row is not deleted. Deleted = 135.
• 146 = 12+7+8+0+0+116+3, i.e. 2009-10 counted with dropna=False, which groups the 130 NaN-plot rows into ~11 extra "duplicate groups". Those rows are deleted before any merge can occur, so they are counted twice.
• Only the remediation's pair reconciles: 135 + 138 = 273. The review's does not.

So this is an evidenced sharpening, not a dodge. (The "1,342" figure elsewhere in the same comment does use dropna=False; both conventions are individually correct, but the comment now mixes them a few lines apart — worth a word if you touch it again.)

Negative control, run by me, not read from the report

Reverted with git checkout origin/development -- lsms_library/countries/Uganda/ (confirmed index: (t, i, plot, input, j) was back), deleted the isolated data dir entirely, ran LSMS_NO_CACHE=1 against the branch's test file. 7 failed, 0 passed:

test_season_is_an_index_level     index is ['i','t','v','plot','input','j'], no `season`
test_both_seasons_are_present     KeyError: 'season'
test_total_row_count              83019 rows, expected 84216
test_per_wave_row_counts          13763/13375/12196/11721/11889/10195/9880
test_kilograms_are_not_summed_with_litres   KeyError: 'season'
test_2013_14_nan_plot_seed_row_survives     got 0
test_no_row_merges_two_seasons_of_the_same_input

Restored, wiped the cache again, cold: 7 passed. The new test_2013_14_nan_plot_seed_row_survives genuinely discriminates.

No false claim survives in a git-tracked file

~95%, 112 of 122, 6 groups per wave, no longer fires at all, genuinely a duplicate, totals are unchanged, keeps_every_reported_line_item — the only surviving hits are inside uganda.py / data_scheme.yml / the test docstring where the old wording is quoted and labelled false. No .coder/ledger/ entry and no CONTENTS.org carries a claim about this table.

Standing-rule check done independently: Uganda/_/CONTENTS.org:20 — "The UNPS uses two visits per year (six months apart) to capture both agricultural seasons". The season split is an instrument design feature, which is the substantive case for the level. The file's two open TODOs (2019-20 missing panel weight, 2005-06 strata vs sub-regions) are unrelated.

Scope

Clean. Every changed file is under lsms_library/countries/ or tests/; no lsms_library/*.py outside countries/; no aggregation: YAML key (the only matches are prose rejecting it). Adjacent tests, cold, my run: gh323_grain_contract, gh323_explicit_reducers, uganda_tables, uganda_v_grain_invariants, schema_consistency + this module = 275 passed, 9 skipped, 0 failures. The GrainCollapseWarnings are the pre-existing Uganda/cluster_features #323 findings.

One non-blocking note

.coder/coverage/latest.csv still carries the pre-fix Uganda plot_inputs row counts (13763 / 13375 / 12196 / 11721 / 11889 / 10195 / 9880). That is a generated snapshot and correctly not hand-edited here, but it will read stale until the next make matrix.

Verdict: READY.