Skip to content

fix(#323): Togo cluster_features — project to cluster grain in the extraction#632

Open
ligon wants to merge 2 commits into
developmentfrom
fix/323-togo-config
Open

fix(#323): Togo cluster_features — project to cluster grain in the extraction#632
ligon wants to merge 2 commits into
developmentfrom
fix/323-togo-config

Conversation

@ligon

@ligon ligon commented Jul 21, 2026

Copy link
Copy Markdown
Owner

Closes the remaining GH #323 cell for Togo: cluster_features.

Updated 2026-07-22 after adversarial review (FIX_FIRST). Findings 1-3 accepted, none disputed. The hand-rolled projection hook is gone; the wave module now alias-imports the shipped collapse_to_cluster_grain (PR #618). Corrected claims are struck through / replaced in place below rather than deleted -- see "Review round 2" at the end, and .coder/ledger/323-togo.md §7.

Config only, per decision D1 of slurm_logs/DESIGN_grain_collapse_sites_2026-07-13.org (branch origin/docs/323-grain-collapse-sites) — the core does not aggregate. lsms_library/*.py is untouched, no aggregation: key is added (it is dead config and contradicts the policy it was meant to serve), and no policy block is added to lsms_library/data_info.yml.

What was wrong

cluster_features is declared (t, v) — 540 grappes — but its df_main is the EHCVM household cover page 2018/Data1/s00_me_tgo2018.dta: 6,171 households. (df_geo, grappe_gps_tgo2018.dta, is already 540 rows, so every duplicate came from df_main.) The wave frame carried 6,171 rows for a 540-row table and handed it 5,631 EXCESS rows to reduce with groupby().first(). ("5,631 rows on a duplicated (t, v) tuple" was the right number under the wrong name: duplicated(keep=False).sum() is 6,171every row shares its tuple, since grappes hold 3-12 households — while duplicated(keep='first').sum() is 5,631. Corrected everywhere, including the tracked files.)

That is an extraction bug, not an aggregation one — so the fix is to stop manufacturing the duplicates, not to reduce them. 2018/_/mapping.py alias-imports the shipped reducer as its cluster_features df_edit hook —

from lsms_library.transformations import (
    collapse_to_cluster_grain as cluster_features,
)

— exactly as that helper's docstring prescribes. It projects to cluster grain and raises GrainConflict (naming the offending grappes) if any column ever varies within one.

Why fix a value-lossless collapse

No rows of value were destroyed — measured, not assumed: all 540 grappes carry exactly one distinct Region (s00q01) and one distinct Rural (s00q04), 0/540 violations. So .first() landed on the right answer and, post-PR #614, the core is correctly silent about it.

It is still wrong, because "lossless" here was an unchecked accident. .first() on a cluster that stopped being constant returns a silently wrong Region, not a missing one — the Kazakhstan 1996 v=126 failure (178 Urban + 2 Rural households reported as Rural) and the CotedIvoire grappe-648 failure. Both were licensed by the same prose — "Region/Rural/District are invariant within a cluster by construction of the LSMS-ISA sampling design" — and prose is not enforcement. This PR converts the assumption into an enforced check, in the cell where it failing would return wrong data rather than merely lossy data.

The diagnosis follows fix/323-cotedivoire (fc3be203), which projects the identical EHCVM cover page for the identical reason. CIV needed strict-majority conflict resolution because grappe 648 genuinely disagreed; Togo has no disagreeing grappe, so the reducer's default on_conflict='raise' costs nothing and is the stronger contract. The mechanism does not copy CIV: CIV's projection was generalised into the public collapse_to_cluster_grain after CIV landed, and that helper is what this PR uses.

Measured before / after

Cold builds, LSMS_NO_CACHE=1, LSMS_COUNTRIES_ROOT pinned to the worktree (verified in-process, not assumed):

observable before after
Wave.grab_data('cluster_features') rows 6,171 540
EXCESS rows (duplicated(keep='first')) 5,631 0
rows sharing a (t, v) (keep=False) 6,171 0
clusters whose duplicate rows disagree 0 0
Country('Togo').cluster_features() rows 540 540
API frame values identical (assert_frame_equal passes)
GrainCollapseWarnings for this cell 0 0

Rows of value: none lost, none recovered — by design. What changed is the grain being correct by construction.

All 21 Togo tables build cold with zero GrainCollapseWarnings (re-run after the round-2 change: 21/21, no errors, plot_inputs 13,730). The round-2 swap to the shipped helper is assert_frame_equal-identical to the round-1 hand-rolled build, wave frame and API frame — same 540 rows, values, order and dtypes.

plot_inputs — already landed, re-verified here

The other Togo #323 cell was fixed on development in 36170a57. Re-measured empirically by rebuilding the (t, i, input, crop, u) frame from the raw s16b roster under the shipped injective harmonize_seed_crop and under a counterfactual pre-fix map with the shared Autre crop catch-all:

observable old (shared catch-all) current (injective)
wave rows 13,733 13,733
conflicting groups 162 0
rows destroyed by first() 165 0
groups repeating one raw s16bq01 label 0 / 162 n/a

0/162 means every collision was between two distinct reported items pooled by a lossy crop map — the identifier was broken, not a level missing. The issue text's "156" is the same defect counted more conservatively (excluding the 9 groups whose duplicate rows carry identical values, where first() loses nothing); both accountings go to 0.

Tests

tests/test_gh323_togo_cluster_features.py — six tests, 4 of which fail on pristine development (2 of 4), all 6 pass after; 3 of 6 fail against the round-1 hand-rolled tree, so the round-2 change is discriminated too. The two that pass either way are the source-invariant check and the API row count; both are meant to hold before and after, and each now says so in its own docstring, naming the tests that do discriminate.

Round 2 also removed the fixtures' except Exception: pytest.skip(...). It turned a genuinely broken build into a green run — the end-to-end tests were vacuous exactly when they mattered. The module now carries requires_s3 from tests/conftest.py, whose pytest_runtest_makereport hook already converts a missing-credentials failure (and only that) into a skip.

Per the CotedIvoire instrument note, no test asserts API-index uniqueness — the collapse makes it unique BY CONSTRUCTION, so such a test passes with the bug fully present. The observable is the pre-collapse wave frame. There is also a negative test that the reducer raises GrainConflict on a synthetic conflicting frame rather than quietly picking a winner, a test that the hook is collapse_to_cluster_grain (identity, not behaviour — a copy is free to drift, and this one had), and a test that complementary missingness completes rather than raising.

Full runs (all green, no new failures):

  • tests/test_gh323_togo_cluster_features.py tests/test_gh323_benin_togo.py — 10 passed, 2 skipped
  • tests/test_grain_collapse_cluster_features.py tests/test_grain_collapse.py tests/test_normalize_index_j_preserved.py tests/test_schema_consistency.py — 247 passed

Salvage / discard from the rescued WIP

rescue/2026-07-21/323-togo carried the cluster_features hook (kept, docstring corrected) and a ledger arguing for the rejected Design A — a declared aggregation: reducer read by core, a LSMS_CACHE_SCHEMA bump, tests/test_grain_aggregation.py, and edits to country.py / feature.py / local_tools.py / data_info.yml. All discarded. .coder/ledger/323-togo.md records why, so the rejected design does not get re-derived.

The draft also claimed this cell fired a spurious warning that camouflaged the plot_inputs one. That was true of the pre-#614 core, whose guard was not df.index.is_unique and could not distinguish a lossless de-dup from a destructive one. It is not true of development today. Corrected in the docstring and ledger rather than repeated.

Deferred (no core primitive needed — just a wider blast radius)

  • Togo/_/data_scheme.yml's interview_date still carries a dead aggregation: {visit: first} key. It came from 717e32f4 (feat(#438)), which added it to six EHCVM countries at once, so removing it is a six-country change and out of scope here. Flagged in the ledger so it is not mistaken for something this PR endorses.
  • The household-cover-page-as-cluster-source shape is corpus-wide (CIV in all 5 waves; 17 countries declare i: in cluster_features idxvars). Togo's variant — no i at all, so Site 2's audit is structurally blind and only Site 1 sees it — is a third shape. Fixed for Togo only; the sweep is a separate PR.

Review round 2 — what changed

finding verdict what was done
1 (MEDIUM) hook re-implements the merged collapse_to_cluster_grain (PR #618); the ledger's REINVENTION check missed it accepted 40-line copy → one-line alias; re-derived cold as assert_frame_equal-identical. Ledger §2/§5 corrected in place, with the search-tier lesson recorded: the round-1 search walked sibling country fixes and never grepped lsms_library/ — a search over peer call sites cannot find the helper extracted from them.
2 (MEDIUM) the copy's nunique(dropna=False) read NaN as contradiction accepted subsumed by the alias. Re-derived: on Region=['Maritime', NaN] / Latitude=[NaN, 6.1] the copy raised and the build died; the helper returns {'Region': 'Maritime', 'Latitude': 6.1}. Latent, not live — today's 5 GPS-less grappes are wholly GPS-less, so nunique(dropna=False) is still 1. Pinned by a new test.
3 (LOW) weaker diagnostics than reduce_to_agreed accepted subsumed; negative test retargeted to GrainConflict / match='grain conflict'.
disputed: nothing the review did not over-claim. Its two non-findings (the household_roster row-order drift attributed to its own harness; "zero duplicates by splitting real entities" ruled out) both reproduce: households/grappe run 3-12 with 394 of 540 grappes at exactly 12, grappe is nationally unique, (grappe, menage) has 0 duplicates.

Everything above was re-derived independently before being written down: isolated LSMS_DATA_DIR with dvc-cache symlinked to the shared blob store, LSMS_COUNTRIES_ROOT + PYTHONPATH pinned with an in-process assert 'wt632' in lsms_library.__file__, no dvc CLI. Negative control by reverting mapping.py to origin/development (→ 6,171 rows, hook None), not by stashing an already-committed change.

Noted for someone else (not fixed here): tests/conftest.py's docstring advises from conftest import aws_creds_available, which raises ImportErrortests/ is a package and a project-level conftest.py shadows the name. from tests.conftest import ... is the working form.

Refs GH #323.

🤖 Generated with Claude Code

@ligon
ligon force-pushed the fix/323-togo-config branch from dc92615 to a481d1f Compare July 21, 2026 22:07
ligon pushed a commit that referenced this pull request Jul 21, 2026
Four PRs in a single day went red for the same non-reason (#625, #632, #641,
#644): a newly added test module builds real country data, the CI `unit-tests`
job runs deliberately data-free (`LSMS_SKIP_AUTH=1`, no S3 secrets -- only
`data-tests` carries them), and the module lacked the skip guard its neighbours
have.  Each cost a diagnosis cycle to establish the failure had nothing to do
with the change under review.  #644's is the worst of them: it CATCHES
`NoCredentialsError` and re-raises it as a message about the geo file spelling
its coordinate columns differently, which is a confident and completely wrong
diagnosis.

There was no `tests/conftest.py` at all, which is why the same
`_aws_creds_available()` helper had been copy-pasted into three modules already,
each noting that centralising it was wanted "but not in a CI-red hotfix".

This adds it, plus `requires_s3`, plus a `pytest_runtest_makereport` wrapper
that converts a missing-credentials failure into a skip -- covering the `setup`
phase too, since the usual shape is a module-scoped fixture building a country,
where one error takes out the whole module.

## It cannot hide a real failure

The conversion is conditioned on the ENVIRONMENT, not on the exception:

  * credentials absent  -> convert to skip;
  * credentials present -> report the failure, unchanged.

A test silently skipped everywhere is worse than a test red somewhere, so this
was verified in both directions rather than assumed:

  creds absent   NoCredentials -> SKIPPED,  `assert 1 == 2` -> still FAILED
  creds present  NoCredentials -> FAILED,   `assert 1 == 2` -> still FAILED

## The subtlety that made verifying this hard, now documented in the file

`lsms_library/countries/.dvc/s3_creds` is NOT tracked in git -- it is written at
IMPORT TIME by the auto-unlock.  So a fresh `git worktree` looks credential-free
until the first import silently creates it, and the guard then reads True.  Only
`LSMS_SKIP_AUTH=1` reproduces CI's actual state.  My first three attempts to
verify this hook "failed" for exactly that reason.

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

`cluster_features` is declared (t, v) — 540 grappes — but its df_main is the
EHCVM *household* cover page, 2018/Data1/s00_me_tgo2018.dta: 6,171 households.
(df_geo, grappe_gps_tgo2018.dta, is already 540 rows, so every duplicate came
from df_main.)  The wave frame therefore carried 6,171 rows for a 540-row table
and left 5,631 of them on a duplicated (t, v) tuple for the framework's
groupby().first() to reduce.

That is an EXTRACTION bug, not an aggregation one.  Per decision D1 of
slurm_logs/DESIGN_grain_collapse_sites_2026-07-13.org the core does not
aggregate, so the answer to a manufactured duplicate is to stop manufacturing
it — never to declare a reducer.  CONFIG ONLY: lsms_library/*.py is untouched
and no `aggregation:` key is added (it is dead config and contradicts D1).

NO ROWS OF VALUE WERE DESTROYED, and that is not the reason to fix it.
Measured on the source: all 540 grappes carry exactly one distinct Region
(s00q01) and one distinct Rural (s00q04) — 0/540 violations — so the collapse
was value-lossless and, post-PR #614, correctly silent about it.  What was
wrong is that "lossless" was an unchecked accident.  `.first()` on a cluster
that stopped being constant returns a silently WRONG Region, not a missing one
— the Kazakhstan 1996 v=126 failure (178 Urban + 2 Rural households reported as
Rural) and the CotedIvoire grappe-648 failure, both licensed by the same prose:
"invariant within a cluster by construction of the LSMS-ISA sampling design".
Prose is not enforcement.  The new cluster_features(df) hook in
2018/_/mapping.py projects to cluster grain AND raises if any column ever
varies within a grappe.

Follows fix/323-cotedivoire (fc3be20), which projects the identical EHCVM
cover page for the identical reason.  CIV needed strict-majority conflict
resolution because grappe 648 genuinely disagreed; Togo has no disagreeing
grappe, so it raises instead — a stronger contract, available because the data
supports it.

Measured (LSMS_NO_CACHE=1, cold, LSMS_COUNTRIES_ROOT pinned to the worktree):

  Wave.grab_data('cluster_features') rows   6,171 -> 540
  rows on a duplicated (t, v) tuple         5,631 -> 0
  Country('Togo').cluster_features() rows     540 -> 540
  API frame values                          IDENTICAL (assert_frame_equal)
  GrainCollapseWarnings for this cell            0 -> 0

Tests: 2 of 4 new tests FAIL on pristine development, all 4 pass after.  The
two that pass either way are the invariant check and the API row count — both
are meant to hold before and after, and saying so is the point: this buys
correctness by construction, not rows.  Per the CotedIvoire instrument note,
no test asserts API-index uniqueness (the collapse makes it unique BY
CONSTRUCTION, so such a test passes with the bug fully present); the observable
is the pre-collapse wave frame.

Also re-verified the sibling plot_inputs defect that landed in 36170a5, by
rebuilding the (t, i, input, crop, u) frame from the raw s16b roster under both
the shipped injective harmonize_seed_crop and a counterfactual pre-fix map with
the shared 'Autre crop' catch-all:

  wave rows                     13,733    13,733
  conflicting groups               162 ->      0
  rows destroyed by first()        165 ->      0
  groups repeating one raw label  0/162  (i.e. every collision is between two
                                          DISTINCT reported items)

The issue text's "156" is the same defect counted more conservatively
(excluding the 9 groups whose duplicate rows carry identical values, where
first() loses nothing).  Both accountings go to 0.

Diagnosis preserved in Togo/_/CONTENTS.org, a pointer comment on
cluster_features in Togo/_/data_scheme.yml, and .coder/ledger/323-togo.md —
which also records that the rescued WIP draft (rescue/2026-07-21/323-togo)
argued for the REJECTED Design A (a declared `aggregation:` reducer read by
core, a LSMS_CACHE_SCHEMA bump, and tests/test_grain_aggregation.py) and that
none of it is carried forward.  That draft also claimed this cell fired a
spurious warning camouflaging the plot_inputs one; true of the pre-#614 core,
whose guard was `not df.index.is_unique`, but not of development today.
Corrected rather than repeated.

Deferred, needing no core primitive but a separate PR: Togo/_/data_scheme.yml's
`interview_date` still carries a dead `aggregation: {visit: first}` key.  It
came from 717e32f (feat(#438)), which added it to six EHCVM countries at once,
so removing it is a six-country change and out of scope here.

Refs GH #323.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@ligon
ligon force-pushed the fix/323-togo-config branch from a481d1f to 9ab4b1a Compare July 22, 2026 02:52
@ligon

ligon commented Jul 22, 2026

Copy link
Copy Markdown
Owner Author

Adversarial review — PR #632 (Togo cluster_features)

Verdict: FIX_FIRST. Every headline number in this PR reproduces cold, the grain fix is genuinely correct (not a "scored zero by splitting real entities" artefact), D1 is honoured, and scope is clean. But the hook is a hand-rolled re-implementation of lsms_library.transformations.collapse_to_cluster_grain — already merged on development (PR #618, 48f4d08f), whose docstring names this exact call site — and the re-implementation's NaN semantics contradict the repo's own doctrine. One-line fix, verified bit-identical.

What I re-derived myself, cold

Isolated LSMS_DATA_DIR (fresh, dvc-cache symlinked to the shared blob store), LSMS_NO_CACHE=1, PYTHONPATH pinned + assert 'wt…' in lsms_library.__file__ in-process. Two worktrees: origin/development @ 45aee170 and origin/fix/323-togo-config @ 9ab4b1ac. No dvc CLI touched.

claim measured verdict
wave frame 6,171 → 540 6,171 → 540
rows on a duplicated (t, v) 5,631 excess → 0
API 540 → 540, values identical assert_frame_equal passes
0/540 grappes vary in Region/Rural 0 and 0; s00q01/s00q04 have 0 NaN
GrainCollapseWarnings for this cell 0 before, 0 after
all 21 Togo tables, 0 grain warnings 21/21, zero warnings, identical row counts and content hashes before vs after
2 of 4 new tests fail on pristine development exactly 2 (…is_already_at_cluster_grain, …raises_rather_than_picking_a_row)
plot_inputs 13,733 wave rows; 780 / 591 / 370 13,733; 780 / 591 / 370 exactly; API 13,730
warm-cache invalidation via the mapping.py hash warm read after a pre-fix build returns 540, not 6,171

Also: git diff --stat confirms no lsms_library/*.py, no aggregation: key added, no index level added, no reducer in core.

Attacks that failed (i.e. the fix survives them)

  • "Zero duplicates by splitting real entities." Ruled out: households-per-grappe maxes at 12, the EHCVM design cell size — 394/540 grappes are exactly 12, none exceeds it. If grappe were unique only within a region we would see ~24. drop_duplicates(['s00q01','grappe']) = 540 = nunique(grappe), so the code really is nationally unique. (grappe, menage) has 0 duplicates.
  • "Invariance by missingness." Ruled out: s00q01/s00q04 are 0% null. The only NaNs anywhere are 5 grappes with no GPS fix, and they are constant within grappe (56 household-rows → 5 cluster-rows).
  • .first() vs duplicated(keep='first') divergence. Checked: pre_wave.groupby(level=['t','v']).first() is assert_frame_equal-identical to the hook's output. No completion was silently lost today.
  • Order/value drift elsewhere. I saw a household_roster row-order difference between my two all-table runs, chased it, and it is not attributable to this PR: re-run under the config-only lever (same library, LSMS_COUNTRIES_ROOT = dev-config vs PR-config, both cold) the frames are identical including order. Pre-existing build-order nondeterminism in my harness, not a finding here.
  • Test discrimination / mutation. Replacing if varying: with if False: in the hook makes test_projection_hook_raises_rather_than_picking_a_row fail. The guard is really guarded.

Finding 1 (MEDIUM) — the hook reinvents merged machinery; the ledger's REINVENTION check missed it

lsms_library/build_transforms.py:514 (re-exported as lsms_library.transformations.collapse_to_cluster_grain, merged on development in PR #618) is the shipped, tested helper for exactly this defect. Its docstring:

# countries/<C>/<wave>/_/mapping.py
from lsms_library.transformations import (
    collapse_to_cluster_grain as cluster_features,
)

and tests/test_gh323_explicit_reducers.py::test_collapse_to_cluster_grain_works_as_a_bare_df_edit_hook exists specifically to keep it callable as a bare df_edit hook.

I swapped the 40-line hand-rolled hook for that one-line alias and rebuilt Togo cold: wave frame and API frame are assert_frame_equal-identical (540 rows, same values, same order). So the re-implementation buys nothing.

The ledger's §2 machinery table lists only country._collapse_to_cluster_grain (the private core Site-2 helper) and cites CIV fc3be203 (2026-07-12) as "the pattern followed here" — but the pattern was generalised into a public country-facing helper after CIV, in the sweep this PR is part of. §5's "REINVENTION check" then asserts the only two candidates were CIV's projection and Design A. That is the ledger's one factual error, and it is the error the prior-art discipline exists to catch.

Finding 2 (MEDIUM) — the guard treats NaN as contradiction, hard-coded, no opt-out

.nunique(dropna=False).gt(1).any()

dropna=False counts NaN as a distinct value, i.e. na_is_conflict=True with no way to turn it off. Demonstrated on Region=['Maritime', NaN] in one grappe, Latitude=[NaN, 6.1] in the same grappe:

result
Togo hand-rolled hook ValueError: column(s) ['Region', 'Latitude'] vary WITHIN a clusterbuild dies
collapse_to_cluster_grain (shipped) {'Region': 'Maritime', 'Latitude': 6.1}
core _normalize_dataframe_index {'Region': 'Maritime', 'Latitude': 6.1}

That contradicts tests/test_gh323_grain_contract.py::test_p2_complementary_missingness_is_COMPLETION_not_fabrication and tests/test_gh323_explicit_reducers.py::test_nan_is_absence_not_contradictionNaN is absence, not contradiction. The hook even accuses Latitude, a column that is pure absence. Not reachable on today's Togo data (s00q01/s00q04 0% null, GPS NaN constant within grappe), so it is latent — but it is a build-breaking latent that converts a correct completion into a hard failure, in a country whose one geo source already carries 5 null-coordinate clusters.

Note the two findings are entangled, which is the real argument for using the helper: the payload de-dup is df[~df.index.duplicated(keep='first')], i.e. literal-first-row (.first(skipna=False)) semantics. It is lossless only because the dropna=False guard raises first. You cannot relax the guard to the correct NaN reading without also rewriting the de-dup — which is precisely what reduce_to_agreed already does (grouped.first() for the payload, nunique(dropna=not na_is_conflict) for the conflict test).

Suggested fix (verified): replace the hook body with

from lsms_library.transformations import collapse_to_cluster_grain as cluster_features

keeping the current docstring as a module-level comment (it is good documentation and should not be lost), and retarget test_projection_hook_raises_rather_than_picking_a_row from match='vary WITHIN a cluster' to GrainConflict / match='grain conflict'.

Finding 3 (LOW) — weaker diagnostics than the shipped path

The hook raises a bare ValueError naming only the columns. reduce_to_agreed raises GrainConflict (a ValueError subclass, so except ValueError callers are unaffected) and names the offending groups — the thing you actually need to debug a cluster-code collision — plus per-column conflict counts, the on_conflict='na' escape hatch, and LSMS_GRAIN_STRICT interaction. Subsumed by the Finding-1 fix.

Not findings, for the record

  • The interview_date aggregation: {visit: first} deferral is accurate: it comes from 717e32f4 (6 EHCVM countries), core provably never retrieves the key, and 9 data_scheme.yml files carry it. Out of scope is the right call.
  • The PR is explicit that this cell was already silent post-fix(#323): close the grain-collapse CLASS — audit before collapse, persist the signal through the cache #614 and that no rows are recovered. That honesty is the right call and I confirmed it (0 warnings before and after).
  • No duplicate of already-merged work: development still builds this wave frame at 6,171 rows.

…vate copy

Review response to PR #632 (FIX_FIRST).  The diagnosis and every headline
number survived the review; the REUSE claim did not.  Findings 1-3 accepted,
nothing disputed.

**Finding 1 — the hook re-implemented merged machinery.**
`lsms_library.transformations.collapse_to_cluster_grain` (build_transforms.py:514,
PR #618, 48f4d08) is the shipped helper for exactly this defect, and its
docstring prescribes this exact call site.  It was merged on `development`
BEFORE this branch was cut, and the round-1 ledger's REINVENTION check missed
it because that search walked sibling COUNTRY fixes (CIV fc3be20) and never
grepped `lsms_library/` — a search over peer call sites cannot find the helper
extracted from them.  The 40-line hand-rolled hook is replaced by

    from lsms_library.transformations import (
        collapse_to_cluster_grain as cluster_features,
    )

Re-derived cold, isolated LSMS_DATA_DIR (dvc-cache symlinked), PYTHONPATH +
LSMS_COUNTRIES_ROOT pinned with an in-process `assert 'wt632' in
lsms_library.__file__`: the wave frame AND the API frame are
assert_frame_equal-IDENTICAL to the hand-rolled build (540 rows, same values,
same order, same dtypes).  The copy bought nothing and could only diverge.

**Finding 2 — the copy read NaN as contradiction.**  Its guard was
`nunique(dropna=False).gt(1)`, i.e. `na_is_conflict=True` hard-coded with no
opt-out, contradicting the repo's own doctrine (NaN is ABSENCE — see
test_gh323_grain_contract.py::test_p2_complementary_missingness_is_COMPLETION_
not_fabrication).  Re-derived on Region=['Maritime', NaN] / Latitude=[NaN, 6.1]
in one grappe: hand-rolled -> ValueError, BUILD DIES; shipped helper ->
{'Region': 'Maritime', 'Latitude': 6.1}.  LATENT, not live: today's 5 GPS-less
grappes are WHOLLY GPS-less, so nunique(dropna=False) is still 1.  One partial
GPS fix or one blank Region and a correct completion becomes a dead build.

**Finding 3 — weaker diagnostics.**  Bare ValueError naming only columns vs.
GrainConflict naming the offending GROUPS + per-column counts, with the
on_conflict='na' hatch and LSMS_GRAIN_STRICT interaction.  Subsumed.

Tests — made to discriminate, and honest where they do not:
  * retargeted the negative test from match='vary WITHIN a cluster' to
    GrainConflict / match='grain conflict';
  * added test_hook_is_the_shipped_reducer_not_a_private_copy (identity, not
    behaviour: a copy is free to drift and this one had) and
    test_nan_is_absence_not_contradiction;
  * REMOVED the fixtures' `except Exception: pytest.skip(...)`.  It converted a
    broken build into a green run — the end-to-end tests were vacuous exactly
    when they mattered.  Replaced by `requires_s3` from tests/conftest.py, whose
    pytest_runtest_makereport hook already converts a missing-credentials
    failure (and only that) into a skip.  NB the bare `from conftest import ...`
    that conftest's own docstring suggests raises ImportError (tests/ is a
    package and a root conftest.py shadows the name); `from tests.conftest
    import ...` is the working form.
  * every docstring now states whether the test DISCRIMINATES.  Measured:
    4 of 6 fail on pristine `development`; 3 of 6 fail against the round-1
    hand-rolled tree; the 2 non-discriminating ones say so and name the ones
    that do.

Corrected claims (all of them also in git-TRACKED text — ledger §1/§2/§4/§5 +
new §7, CONTENTS.org, data_scheme.yml, mapping.py):
  * "5,631 rows on a duplicated (t, v) tuple" was the right number under the
    wrong name.  duplicated(keep=False).sum() = 6,171 — EVERY row shares its
    tuple, since grappes hold 3-12 households — and duplicated(keep='first')
    = 5,631, the EXCESS.  Both re-derived.
  * "new, but on the CIV pattern" (§5) and the clean REINVENTION check (§5) are
    corrected in place, with the search-tier lesson recorded rather than the
    error quietly erased.
  * "2 of 4 new tests fail on pristine development" -> 4 of 6.

Re-verified, unchanged: 540 grappes, 0/540 varying in Region/Rural/Lat/Long;
API 540 rows; 0 GrainCollapseWarnings before AND after; full cold Togo build,
21/21 tables, zero grain warnings, plot_inputs 13,730.  Negative control done
by REVERTING mapping.py to origin/development (6,171 rows, hook None), not by
stashing a committed change.

Refs GH #323.

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

ligon commented Jul 22, 2026

Copy link
Copy Markdown
Owner Author

Review response — 8e1394b2 (force-pushed to fix/323-togo-config)

Findings 1–3 accepted, nothing disputed. The review did not over-claim; the code was fine and the reuse claim was not. Everything below was re-derived before being written down (isolated LSMS_DATA_DIR, dvc-cache symlinked to the shared blob store, LSMS_COUNTRIES_ROOT + PYTHONPATH pinned with an in-process assert 'wt632' in lsms_library.__file__, no dvc CLI; negative control by reverting mapping.py to origin/development, not by stashing).

Finding 1 — reinvention. Fixed with the alias.

Togo/2018/_/mapping.py now reads

from lsms_library.transformations import (
    collapse_to_cluster_grain as cluster_features,
)

and the 40-line copy is gone. Independently confirmed bit-identical: two cold builds, the second differing only in that file — wave frame and API frame assert_frame_equal-identical, 540 rows, same values, same order, same dtypes. Full cold Togo re-run: 21/21 tables, no errors, zero grain warnings, plot_inputs 13,730.

The long diagnosis docstring is kept as a module comment above the import, as you suggested.

Finding 2 — NaN. Subsumed, and pinned.

Reproduced your demonstration exactly: Region=['Maritime', NaN], Latitude=[NaN, 6.1] in one grappe → hand-rolled ValueError (build dies) vs. helper {'Region': 'Maritime', 'Latitude': 6.1}. Also confirmed it is latent, not live — the 5 GPS-less grappes are wholly GPS-less (nunique(dropna=False) = 1 for Latitude in 0/540 grappes), so the copy never actually fired. The tracked text now says "latent" rather than implying it was live. New test_nan_is_absence_not_contradiction fails against the round-1 tree.

Finding 3 — diagnostics. Subsumed.

test_projection_hook_raises_rather_than_picking_a_row retargeted from match='vary WITHIN a cluster' to GrainConflict / match='grain conflict'.

The ledger error, recorded rather than erased

§2 now lists reduce_to_agreed and collapse_to_cluster_grain as reuse; §5's projection row is corrected from "new, but on the CIV pattern"; §5's REINVENTION check now states plainly that it failed, and why: the round-1 search was a git log/git show walk over sibling country fixes and never grepped lsms_library/. A prior-art search that only looks at peer call sites cannot find a helper that was extracted from them. New §7 carries the full round-2 record.

Two things you did not flag, fixed anyway

  • The fixtures made the end-to-end tests vacuous. togo and wave_frame wrapped everything in except Exception: pytest.skip(...), so a genuinely broken build would have gone green. Removed; the module now carries requires_s3 from tests/conftest.py, whose pytest_runtest_makereport hook already converts a missing-credentials failure (and only that) into a skip.
  • "5,631 rows on a duplicated (t, v) tuple" was the right number under the wrong name. duplicated(keep=False).sum() is 6,171 — every row shares its tuple, since grappes hold 3–12 households, 0 singletons — and duplicated(keep='first').sum() is 5,631, the excess. Your table quietly said "5,631 excess"; mine did not. Corrected in the PR body, the ledger, CONTENTS.org, data_scheme.yml and mapping.py.

Test discrimination, restated honestly

Six tests. 4 fail on pristine development; 3 fail against the round-1 hand-rolled tree. The other two (…api_still_returns_every_cluster, …region_and_rural_are_constant_within_a_grappe) pass both ways by design — they pin an invariant and a source fact — and each docstring now says so and names the tests that do discriminate. The module docstring carries the same summary.

Out of scope, flagged not fixed

  • Togo/_/data_scheme.yml's dead aggregation: {visit: first} on interview_date — still a six-country change (717e32f4).
  • tests/conftest.py's docstring advises from conftest import aws_creds_available, which raises ImportError: tests/ is a package and a project-level conftest.py shadows the name. from tests.conftest import ... is the working form and is what this module uses; the docstring is somebody's one-line fix, not mine to make here.

🤖 Generated with Claude Code

@ligon

ligon commented Jul 22, 2026

Copy link
Copy Markdown
Owner Author

Independent verification of the round-2 remediation (8e1394b2)

Verdict: STILL_BROKEN — one false claim survives in a git-tracked file. Findings 1–3 are all genuinely fixed and I reproduced each one myself; the remaining item is a two-line docstring correction, not a code defect. Nothing about the fix itself is wrong.

Re-derived in my own worktree (vfy632, detached at 8e1394b2), isolated LSMS_DATA_DIR with dvc-cache symlinked to the shared blob store, LSMS_COUNTRIES_ROOT + PYTHONPATH pinned with an in-process assert 'vfy632' in lsms_library.__file__. No dvc CLI. Negative control by reverting mapping.py to origin/development, then separately to 9ab4b1ac — never by stashing.

Findings 1–3: all genuinely fixed

finding verified how verdict
1 hook reinvents collapse_to_cluster_grain formatting_functions['cluster_features'] is lsms_library.transformations.collapse_to_cluster_grainTrue; repr is <function collapse_to_cluster_grain>. The 40-line copy is gone. ✅ fixed
2 nunique(dropna=False) reads NaN as contradiction Subsumed by the alias. Reproduced the divergence directly: on the round-1 tree, test_nan_is_absence_not_contradiction fails with the exact ValueError: column(s) ['Region', 'Latitude'] vary WITHIN a cluster; on 8e1394b2 it passes and completes to Region='Maritime', Latitude=6.1. ✅ fixed + pinned
3 bare ValueError vs GrainConflict test_projection_hook_raises_rather_than_picking_a_row now asserts GrainConflict / match='grain conflict'; passes on head, fails on the round-1 tree. ✅ fixed + pinned

Negative controls — both reproduce exactly as claimed

Reverting lsms_library/countries/Togo/2018/_/mapping.py to origin/development and clearing the cold Togo cache:

  • wave frame 6,171 rows, index not unique, duplicated(keep=False) = 6,171, duplicated(keep='first') = 5,631, API 540, hook None.
  • 4 of 6 tests fail, and they are exactly the four the module docstring names: …is_already_at_cluster_grain, …hook_is_the_shipped_reducer…, …raises_rather_than_picking_a_row, …nan_is_absence_not_contradiction.

Reverting to 9ab4b1ac (the round-1 hand-rolled tree): 3 of 6 fail, as claimed. On 8e1394b2: 6 passed.

So the "5,631 excess / 6,171 on a duplicated tuple" correction (claims_corrected #1) is right, and every tracked instance of 5,631 now carries the word EXCESS. Wider suite — test_gh323_togo_cluster_features + test_gh323_explicit_reducers + test_gh323_grain_contract + test_gh323_benin_togo + test_grain_collapse_cluster_features + test_grain_collapse: 75 passed, 2 skipped.

Other numbers I re-derived rather than took on trust: households/grappe 3–12, 394 of 540 grappes at exactly 12, (grappe, menage) 0 duplicates, drop_duplicates(['s00q01','grappe']) = 540 = nunique(grappe), s00q01/s00q04 0% null, 5 GPS-less grappes covering 56 household rows. All match.

Scope is clean: the 5 files are unchanged from round 1, no lsms_library/*.py outside countries/, and git diff -- '*.yml' | grep '^+\s*aggregation:' returns nothing.


The one thing outstanding — a false claim in tests/test_gh323_togo_cluster_features.py

The module docstring, lines 22–24:

nunique(dropna=False) guard read NaN as CONTRADICTION, which would have turned Togo's 5 GPS-less grappes into a build-breaking "conflict".

That is false, and it is the same sentence claims_corrected #4 says was caught and rewritten — it was fixed in mapping.py, CONTENTS.org and ledger §7, but the test module was not swept, so the claim survives in tracked text.

Proven, not argued. grappe_gps_tgo2018.dta is 540 rows — one per grappe — so Latitude is constant within a grappe by construction; I measured groupby('grappe')[lat].nunique(dropna=False) > 1 on 0 of 540 grappes. And directly: checking out the round-1 mapping.py and building Togo cold,

ROUND-1 hand-rolled hook on REAL Togo data: BUILD SUCCEEDED, rows = 540
  -> the dropna=False guard did NOT fire on the 5 GPS-less grappes
  Latitude nulls in wave frame: 5

The guard never fired. The defect was latent, exactly as mapping.py, CONTENTS.org and ledger §7 correctly say, and as this same test file says 210 lines further down in test_nan_is_absence_not_contradiction ("the latent case is one upstream edit away from live"). The module docstring contradicts its own test module.

This matters more than a typo: it is a claim that a bug was live on real data, in the file whose job is to pin the fix, and it would be read by the next person as evidence the round-1 code was breaking Togo builds — which it demonstrably was not. It also inflates the justification for the change that was already justified on the correct (latent) grounds.

Fix: replace lines 23–24 with the latent framing already used everywhere else, e.g.

nunique(dropna=False) guard read NaN as CONTRADICTION. That did NOT fire on today's Togo data — the 5 GPS-less grappes are wholly GPS-less, so their nunique(dropna=False) is still 1 — but it was LATENT: one partial GPS fix, or one blank Region, would have converted a correct completion into a dead build.

Re-run the six tests after the edit (docstring-only, so they should stay 6/6) and this is READY.

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