Skip to content

docs(#637): EHCVM people_last7days (t, i, pid) key review -- no key is broken#638

Merged
ligon merged 1 commit into
developmentfrom
docs/637-ehcvm-people-last7days-key-review
Jul 22, 2026
Merged

docs(#637): EHCVM people_last7days (t, i, pid) key review -- no key is broken#638
ligon merged 1 commit into
developmentfrom
docs/637-ehcvm-people-last7days-key-review

Conversation

@ligon

@ligon ligon commented Jul 21, 2026

Copy link
Copy Markdown
Owner

Key-soundness review of the six copy-pasted EHCVM people_last7days sites named in #637.

Verdict: no key is broken. No fix warranted.

The .groupby(['t', 'i', 'pid'], dropna=False, as_index=False).first() line is a strict no-op in all six countries — there are zero duplicate (t, i, pid) groups, so .first()'s per-column skipna=True can form no composite row at all. Not "the composites it forms are benign": it forms none.

This PR therefore adds only the evidence — a comment at each site recording that the key was reviewed, what was checked, and the counts. No behaviour change.

How it was measured

Directly on the source, not by instrumenting the build. These tables are materialize: make, so they run out-of-process via subprocess.run(make_cmd, …); the earlier in-process DataFrameGroupBy.first monkeypatch never entered those subprocesses, which is why it reported a meaningless 0 for these sites (per the #637 correction thread, all 38 country-script sites were unmeasured).

For each country: load s04_me_* (7-day module) and s01_me_* (roster) with get_dataframe(), reproduce the merge and the i()/pid construction exactly as the site does, then count duplicate groups on the built key and classify each.

Per-country verdict

country s04 rows s01 rows dup groups on (grappe, menage, s01q00a) in s04 / s01 merge fan-out groups on (t,i,pid) dup groups exact complementary conflicting
Benin 2018-19 35,042 42,343 0 / 0 0 35,042 0 0 0 0
Burkina Faso 2018-19 45,612 45,612 0 / 0 0 45,612 0 0 0 0
CotedIvoire 2018-19 61,116 61,116 0 / 0 0 61,116 0 0 0 0
Guinea-Bissau 2018-19 42,839 42,839 0 / 0 0 42,839 0 0 0 0
Senegal 2018-19 66,120 66,119 0 / 0 0 66,120 0 0 0 0
Togo 2018 25,282 27,480 0 / 0 0 25,282 0 0 0 0

groups == rows in every row of that table. There is no conflicting group to quote evidence for, because there is no duplicate group of any kind.

Why the key cannot merge two people

Three independent facts, each checked:

  1. s04 is already one row per person. Zero duplicate (grappe, menage, s01q00a) groups in the 7-day module itself.
  2. The roster merge does not fan out. s01 also has zero duplicate person-key groups, so the how='left' merge adds zero rows in all six countries (merged == len(s04) exactly).
  3. Every household-id formatter is injective, over the full roster — not just the households that reach s04:
country formatter distinct (grappe, menage) pairs distinct i collisions
Benin grappe + zeropad3(menage) 8,012 8,012 0
Burkina Faso ehcvm_i: grappe + '0' + zeropad2(menage) 7,010 7,010 0
CotedIvoire grappe + zeropad3(menage) 12,992 12,992 0
Guinea-Bissau grappe + '0' + zeropad2(menage) 5,351 5,351 0
Senegal grappe + '0' + zeropad2(menage) 7,156 7,156 0
Togo grappe + '0' + zeropad2(menage) 6,171 6,171 0

Two of these are worth naming because they are the ones that could have collided:

  • Burkina Faso menage runs to 527 — the three-digit case that broke the older i() (the GAP-4 livestock i-key bug documented in ehcvm_i's docstring). ehcvm_i's '0' separator survives it with zero collisions.
  • CotedIvoire concatenates with no separator at all. It is injective only because menage tops out at 30, so zeropadding=3 always yields exactly three characters and the split point is unambiguous. A future wave with menage > 999 would break that. Recorded in the comment at the site.
  • Guinea-Bissau's grappe is a wide code (max 27,711,109), not a 3-digit cluster number as elsewhere; the separated form still collides on none of it.

End-to-end confirmation

Cold rebuilds under an isolated LSMS_DATA_DIR (dvc-cache symlinked; no dvc CLI invoked anywhere in this work). The wave parquets carry exactly the pre-groupby row counts, with zero duplicate index entries:

Togo    wave-parquet rows 25282  dup idx 0
Benin   wave-parquet rows 35042  dup idx 0
Senegal wave-parquet rows 66120  dup idx 0

Deliberately NOT changed

Three side observations, recorded so they are not rediscovered as defects

  1. dropna=False is inert at all six sites. It governs NA group keys only. Rows with NA i/pid are already filtered out on the line immediately above, and t is a literal. If the author believed it preserved NAs in the values, that belief was mistaken — and harmless here.
  2. Senegal has one orphan person. Exactly one s04 person-key — (grappe 481, menage 9, line 17) — has no s01 roster row, so that person's Age (hence working_age) is NA. A one-row roster gap, not a key defect.
  3. Senegal's API row count is 65,723, not the parquet's 66,120. I chased this 397-row gap because a silent row loss is exactly what a broken key looks like. It is neither the key nor the v-join: all 7,156 wave households are present in sample() at t='2018-19' with zero NA v, _join_v_from_sample drops nothing, and id_walk drops nothing (Senegal's updated_ids['2018-19'] is empty). It is the universal hollow-row safety net df.dropna(how='all') at country.py:2243, removing exactly 397 persons whose every declared column is NA (unknown age and non-1/2 codes on all three dummies). Benin and Togo have 0 such rows. Working as designed; unrelated to bug: groupby().first() is not first_row() — skipna=True fabricates composite rows (49 sites) #637.

Refs #637.

🤖 Generated with Claude Code

…_last7days sites

Key-soundness review of the six copy-pasted EHCVM `people_last7days`
`.groupby(['t','i','pid'], dropna=False).first()` sites (Benin, Burkina
Faso, CotedIvoire, Guinea-Bissau, Senegal, Togo).

Verdict: no key is broken.  The collapse is a strict NO-OP in all six
countries -- there are zero duplicate (t, i, pid) groups, so `.first()`'s
per-column skipna=True can form no composite row at all.  This commit
adds only the evidence: a comment at each site recording that the key was
reviewed, what was checked, and the counts.  No behaviour change.

Measured on the SOURCE directly rather than by instrumenting the build.
These tables are `materialize: make`, so they run out-of-process via
subprocess; the earlier in-process `DataFrameGroupBy.first` monkeypatch
probe never observed them, which is why it reported a meaningless "0"
(see the #637 correction thread).

  country        s04 rows  s01 rows  dup key groups  merge fan-out
  Benin            35,042    42,343       0 / 0             0
  Burkina Faso     45,612    45,612       0 / 0             0
  CotedIvoire      61,116    61,116       0 / 0             0
  Guinea-Bissau    42,839    42,839       0 / 0             0
  Senegal          66,120    66,119       0 / 0             0
  Togo             25,282    27,480       0 / 0             0

Every country's household-id formatter is injective over the full
roster (8,012 / 7,010 / 12,992 / 5,351 / 7,156 / 6,171 distinct
(grappe, menage) pairs -> the same number of distinct ids), so no two
households -- and hence no two persons -- can be merged onto one key.
Cold rebuilds confirm end to end: the wave parquets carry exactly the
pre-groupby row counts (Togo 25,282; Benin 35,042; Senegal 66,120) with
zero duplicate index entries.

Deliberately NOT changed:

* `.first()` is left as-is.  `.first(skipna=False)` was withdrawn on
  #637 as a regression -- NaN is absence, not contradiction, and it
  would return <NA> for values the survey recorded.
* No `aggregation:` YAML key (GH #323 D1 forbids it).

Also recorded at the sites, so they are not rediscovered as defects:

* `dropna=False` governs NA *group keys* only.  Rows with NA i/pid are
  already filtered out immediately above and `t` is a literal, so the
  kwarg is inert at all six sites.
* Senegal: exactly one s04 person-key (grappe 481, menage 9, line 17)
  has no s01 roster row, so that person's Age/working_age is NA.  A
  one-row roster gap, not a key defect.
* Senegal's API-level row count is 65,723, not the parquet's 66,120.
  The 397-row difference is NOT the v-join and NOT id_walk (both were
  checked and drop nothing): it is the universal hollow-row safety net
  `df.dropna(how='all')` at `country.py:2243` removing 397 persons whose
  every declared column is NA.  Unrelated to the key.

Refs #637.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@ligon
ligon force-pushed the docs/637-ehcvm-people-last7days-key-review branch from c4773f1 to c7dee21 Compare July 22, 2026 02:56
@ligon

ligon commented Jul 22, 2026

Copy link
Copy Markdown
Owner Author

Adversarial review — APPROVE_WITH_NOTES. I re-derived every headline number independently and could not break the claim.

I did not take a single figure on trust. I rebuilt the measurement from the .dta sources with get_dataframe() (no dvc CLI anywhere), importing the actual site formatters (benin.i, burkina_faso.ehcvm_i, and the inlined pad3 / '0'-separated forms), and reproducing the merge and key construction line-for-line. Every number in the six comments and in the PR body is exact.

country s04 / s01 rows s04 dup person-key s01 dup person-key merge fan-out rows→groups on (t,i,pid) dup roster pairs → distinct i menage max
Benin 35,042 / 42,343 0 0 0 35,042 → 35,042 0 8,012 → 8,012 562
Burkina Faso 45,612 / 45,612 0 0 0 45,612 → 45,612 0 7,010 → 7,010 527
CotedIvoire 61,116 / 61,116 0 0 0 61,116 → 61,116 0 12,992 → 12,992 30
Guinea-Bissau 42,839 / 42,839 0 0 0 42,839 → 42,839 0 5,351 → 5,351 18 (grappe max 27,711,109)
Senegal 66,120 / 66,119 0 0 0 66,120 → 66,120 0 7,156 → 7,156 12
Togo 25,282 / 27,480 0 0 0 25,282 → 25,282 0 6,171 → 6,171 16

Matches the PR table cell for cell, including menage 562 / 527 / 30 and Guinea-Bissau's grappe max 27,711,109.

Attacks I ran and what they found

1. "Duplicate counts falling to zero is not evidence." Agreed as a general rule, but it does not bite here: this is not a proposed new key scored against duplicates, it is a demonstration that the existing key is already unique. The two ways a zero can be an artefact were both excluded:

  • Invariance by missingness — if i or pid were partly NULL the df[df['i'].notna() & df['pid'].notna()] filter above would have silently shrunk the frame. It does not: pre-filter rows == post-filter rows == s04 rows in all six. Nothing is dropped, so the zero is not bought with dropped rows.
  • Splitting real entities — impossible in the other direction here (nothing was added to the key), but I checked the mirror-image risk: is (grappe, menage) actually a household, or is there a hidden id level being collapsed? This is the Niger EXTENSION failure mode from fix(#637): Uganda plot_inputs grain needs season; key-soundness review of six sites #646, i.e. two real households merged onto one i and their line numbers colliding. vague is the only candidate extra id column in the EHCVM s01/s04, and it is strictly 1:1 with (grappe, menage) in all six (8,012 / 7,010 / 12,992 / 5,351 / 7,156 / 6,171 groups either way). This is the check that would have caught a Niger-style defect, the PR does not report it, and it comes out clean — so the conclusion is if anything better supported than the PR argues.

2. "No key is broken" under the wider reading — stranded households. The PR proves injectivity of the i formatter. It does not check the other flavour of broken key: an i that is internally injective but does not reconcile with sample(), which is exactly the GAP-4 livestock bug that burkina_faso.ehcvm_i's own docstring documents — and Benin and CotedIvoire use precisely the no-separator grappe + pad3(menage) form that docstring calls the bug. So I checked it: wave household-id set ∩ sample().i, per country — 8,012 / 7,010 / 12,992 / 5,351 / 7,156 / 6,171, i.e. 100% of wave households present in sample() in all six. No strandings. The headline survives the wider reading too.

3. End-to-end, cold. Isolated LSMS_DATA_DIR with dvc-cache symlinked (not LSMS_NO_CACHE=1, which is soft for script-path L2-wave parquets):

Togo    API rows 25282  dup idx 0   | wave parquet 25282  dup idx 0
Senegal API rows 65723  dup idx 0   | wave parquet 66120  dup idx 0

Reproduces the PR exactly, including the 397-row Senegal gap. I re-derived the 397 straight from source rather than accepting the attribution: rows where the age is unknown and all three dummies are outside {1,2} number exactly 397 in Senegal, and 66,120 − 397 = 65,723. Confirmed independently, so the dropna(how='all') explanation is not just arithmetic that happens to close.

4. Methodology claim. people_last7days really is materialize: make in all six data_scheme.yml, so the "the in-process DataFrameGroupBy.first patch never entered these subprocesses, the earlier 0 was not a measurement" correction is right, and this PR's source-level measurement is the right replacement.

5. Scope / house rules. Diff is six files, all under countries/, comment lines onlygrep -E '^\+' | grep -vE '^\+\s*#' returns nothing. No aggregation: key. No reducer smuggled in. .first() untouched, and each site now says why skipna=False is a regression, which is correct per the correction thread. No duplicate of merged work: git log origin/development on these paths shows only the original feature-add 760d4204.

Notes (none blocking)

  1. Togo has 2 orphan s04 person-keys, and they are not recorded. (grappe 70, menage 9, line 11) and (70, 9, 12) are in s04 but not s01 (household (70,9) is in the roster, so it is a two-person roster gap, same class as Senegal's). The PR calls out Senegal's single orphan as a side observation precisely so it is "not rediscovered as a defect" — but the orphan sweep evidently only ran for Senegal, and the identical fact in Togo is undocumented. Nothing stated is false; the record is just incomplete where it claims to be complete. Same for the hollow-row sweep: the PR says "Benin and Togo have 0 such rows" (true — I get 0 for both), but Burkina Faso 280, CotedIvoire 499 and Guinea-Bissau 270 rows are dropped by the same dropna(how='all') net and go unmentioned.

  2. The headroom warning is on the wrong site. CotedIvoire's comment flags that _i() concatenates with no separator and "a future wave with menage > 999 would break that". Benin's benin.i() is the same no-separator grappe + pad3(menage) form, and Benin's menage reaches 562 against CotedIvoire's 30 — Benin is ~1.8× from the cliff where CotedIvoire is ~33× from it. Yet Benin's comment presents 562 as reassurance ("the zero-padded-3 form is exercised; no collision") and carries no headroom warning at all. If the prospective risk is worth recording anywhere, it is worth recording at Benin first. (Concrete: grappe=67, menage=1005'671005' collides with grappe=671, menage=5'671005'.)

  3. The invariant is now documented but still undefended. The PR establishes that the collapse is dead code, and the comment will be true right up until the day it isn't — a future wave that broke uniqueness would silently merge two persons, and nothing would fire. A one-line assert not df.duplicated(['t','i','pid']).any() alongside (or in place of) the dead .first() would turn the review's conclusion into an enforced invariant. That is an identifier-side guard, not a reducer, so it is D1-compatible. Deferrable, but it is the difference between evidence that rots and evidence that holds.

What I tried and could not break

Re-derived all 6×9 source counts cold and independently; excluded invariance-by-missingness (pre-filter == post-filter rows); excluded a hidden vague/extension household level (1:1 in all six, the Niger-EXTENSION failure mode); excluded stranded households against sample() (100% in all six); re-derived Senegal's 397 hollow rows from source rather than trusting the attribution; cold-rebuilt Togo and Senegal in an isolated LSMS_DATA_DIR and got 25,282 / 66,120 / 65,723 exactly. Verified the materialize: make premise behind the methodology correction. Found no false claim, no aggregation: key, no reducer, no core file touched, and no overlap with already-merged work.

@ligon
ligon merged commit 0659da2 into development Jul 22, 2026
10 checks passed
@ligon
ligon deleted the docs/637-ehcvm-people-last7days-key-review branch July 22, 2026 03:53
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