Skip to content

fix(#323): the D1 guard had its polarity inverted — guard choosing, not naming#640

Merged
ligon merged 1 commit into
developmentfrom
fix/323-guard-polarity
Jul 21, 2026
Merged

fix(#323): the D1 guard had its polarity inverted — guard choosing, not naming#640
ligon merged 1 commit into
developmentfrom
fix/323-guard-polarity

Conversation

@ligon

@ligon ligon commented Jul 21, 2026

Copy link
Copy Markdown
Owner

Follow-up to #635, which fixed the instrument but kept the wrong target.

The inversion

#635 bans reduce_to_agreed, collapse_to_cluster_grain, add_visit_level from core. But look at what those three do: they raise on disagreement, or blank the cell to <NA> and warn. None of them ever picks a winner. Core calling one cannot violate D1.

The construct that does choose — a bare groupby(...).first(), silently keeping one of two disagreeing observed values — was not guarded at all.

The guard forbade the safe tools and permitted the dangerous one.

This is not theoretical: implementing Option B (core using reduce_to_agreed in place of .first()) would have tripped #635 as a false positive — the same species of error as the original substring grep, one level up.

What replaces it

test_core_never_reduces_grain_without_auditing_it — a core function may use a raw pandas reducer only if it audits that reduction in the same function, while the pre-collapse frame still exists. One line later the evidence is gone; that is the whole lesson of #323.

All three current sites satisfy it:

site reduces audits
country.py::_collapse_to_cluster_grain 4536 4530
country.py::_normalize_dataframe_index 4680 4664
feature.py::_collapse_duplicate_index 142 127

That third site was not in #635s field of view at all.

Note the gradient this creates on purpose: rewrite a site to use a fail-loud helper and the raw reducer disappears, so no audit is required — nothing can be silently destroyed. Use the raw primitive and you must show your work.

test_core_does_not_read_an_aggregation_key — the actual Design-A wiring the name-ban was groping for. Twelve sweep branches proposed teaching core to read an aggregation: key; D1 rejected it. Core may mention the string (it sits in the _skip set so the key is ignored rather than mistaken for a column) but must never retrieve it.

test_the_reducers_are_not_yaml_dispatchable is kept, with its rationale corrected — it is not a D1 requirement. It is that these helpers are invoked explicitly from a countrys mapping.py, where the call is visible in review, rather than via a YAML string that is easy to add without one.

Verified in four directions

Tests only; no lsms_library/*.py touched. Unblocks Option B.

…not naming

PR #635 replaced a substring grep with an AST check, which fixed the false
positive but kept the wrong TARGET.  It banned `reduce_to_agreed`,
`collapse_to_cluster_grain` and `add_visit_level` from core.  Look at what those
three actually do: they raise on disagreement, or blank the cell to <NA> and
warn.  **None of them ever picks a winner.**  Core calling one cannot violate
D1.

Meanwhile the construct that DOES choose -- a bare `groupby(...).first()`, which
silently keeps one of two disagreeing observed values -- was not guarded at all.
The guard forbade the safe tools and permitted the dangerous one.

This surfaced concretely: implementing Option B (core uses `reduce_to_agreed`
instead of `.first()`) would have tripped #635's guard as a false positive --
the same species of error as the substring grep, one level up.

## The replacement

`test_core_never_reduces_grain_without_auditing_it` -- a core function may use a
raw pandas reducer (`groupby(...).first/last/nth`, `agg('first')`) only if it
AUDITS that reduction in the same function, while the pre-collapse frame still
exists.  All three current sites satisfy it:

    country.py::_collapse_to_cluster_grain   reduce 4536, audit 4530
    country.py::_normalize_dataframe_index   reduce 4680, audit 4664
    feature.py::_collapse_duplicate_index    reduce  142, audit  127

(the feature.py site was not in #635's field of view at all.)

Note the gradient this creates deliberately: rewrite a site to use a fail-loud
helper and the raw reducer disappears, so no audit is required -- nothing can be
silently destroyed.  Use the raw primitive and you must show your work.

`test_core_does_not_read_an_aggregation_key` -- the actual Design-A wiring the
old name-ban was groping for.  Twelve sweep branches proposed teaching core to
read an `aggregation:` key from `data_scheme.yml`; D1 rejected it.  Core may
MENTION the string (it is in the `_skip` set, so the key is ignored rather than
mistaken for a column) but must never RETRIEVE it.

`test_the_reducers_are_not_yaml_dispatchable` is kept, with its rationale
corrected: it is not a D1 requirement, it is that these helpers are invoked
explicitly from a country's `mapping.py` where the call is visible in review,
rather than through a YAML string that is easy to add without one.

## Verified in three directions

- green on `development` as it stands (22 passed);
- an unaudited `groupby().first()` injected into `feature.py` is REPORTED;
- `core.get("aggregation")` injected into `country.py` is REPORTED;
- and the Option-B shape -- core calling `reduce_to_agreed`, raw reducer gone --
  now PASSES, where under #635 it would have failed.

Tests only; no `lsms_library/*.py` touched.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@ligon
ligon merged commit d7d57ce into development Jul 21, 2026
10 checks passed
@ligon
ligon deleted the fix/323-guard-polarity branch July 21, 2026 20:52
ligon pushed a commit that referenced this pull request Jul 22, 2026
…raw two false claims

Adversarial review of PR #634 (FIX_FIRST).  One real defect, plus tracked text
that asserted things which do not reproduce.

1. The composite `v` over-split three real 2019-20 parishes
--------------------------------------------------------------

The only evidence the branch offered for the DISTRICT/PARISH key was "clusters
spanning more than one district: 20 -> 0, 23 -> 0".  That number is ZERO BY
CONSTRUCTION under a key that contains the district: it checks that the key was
applied, and structurally cannot see the opposite error.  The branch ran the
spelling-artefact audit on the subcounty component it REJECTED but not on the
district component it ADOPTED.

Audit that can see it: group GSEC1 by its own (county, subcounty, parish) triple
and count distinct v.  2018-19: 0.  2019-20: 3 --

  BUSIKI/NAMUTUMBA/NAWANSAGWA   NAMTUMBA (1 hh)  / NAMUTUMBA (10)
  KASSANDA/KITUMBI/KIJUNA       MUBENDE  (1 hh)  / KASSANDA  (9)
  OMORO/BOBI/PALWO              GULU     (2 hh)  / OMORO     (8)

NAMTUMBA is a misspelling occurring once in 3,098 households; Kassanda District
was carved out of Mubende in 2019 and Omoro out of Gulu in 2016, and a straggler
household still codes the parent.  All three pairs also agree on `region`.  Four
households were being put in a 1-2 household phantom cluster instead of the
10-11 household cluster they belong to -- the same "new bug traded for the old
one" the file cites when declining to key on subcounty.

Fixed with a three-entry `_V_ALIASES` table in 2019-20/_/mapping.py, keyed on
the (district, parish) PAIR: Mubende and Gulu are still real, populated
districts in this wave (29 / 37 hh), so a bare district rename would be wrong.
`District` is now declared row-valued in that wave's cluster_features and
resolved through the same table -- otherwise reduce_to_agreed would blank
District for exactly the three clusters just merged, trading three over-splits
for three <NA>s whose value we have just argued we know.

  2019-20 clusters 827 -> 824; total 4,808 -> 4,805 (parquet 4,812 -> 4,809).
  cluster_features District <NA> in 2019-20: 0.  grain_reports('Uganda'): [].
  cf (t,v) unknown to sample: 0; orphaned sample clusters: 4 (2010-11, as
  documented).  Cold build, isolated LSMS_DATA_DIR.

Two further one-household district misspellings (PALISA, RUKINGIRI; also PALISA
in 2018-19) are deliberately NOT aliased: the correct spelling holds no
household in the affected parish, so they split nothing.  Written down rather
than silently skipped.

2. Tests that discriminate, and fixtures that cannot go vacuous
--------------------------------------------------------------

* test_v_does_not_over_split_one_real_parish[2018-19, 2019-20] -- the audit
  above, against GSEC1.  Negative control: emptying `_V_ALIASES` in place (git
  stash does nothing to a committed change) fails the 2019-20 parametrisation.
  The 2018-19 one passes either way; its docstring says so.
* test_the_2019_20_alias_table_is_the_thing_being_tested -- data-free companion,
  so the fix is pinned in the credential-free CI job too.  Also asserts an
  unaliased pair passes through untouched.
* The module no longer converts a build failure into a skip.  `_uganda_or_skip`
  and the `built` fixture wrapped everything in `except Exception:
  pytest.skip(...)`, which would make the end-to-end half of the file vacuous
  the moment the build broke.  Replaced by tests/conftest.py's
  aws_creds_available() guard on the `uga` fixture only -- so the data-free job
  runs 8 tests and skips 19, instead of skipping on any error.

3. False claims withdrawn (CONTENTS.org, .coder/ledger/323-uganda.md, PR body)
--------------------------------------------------------------

* "Pre-existing failure ... test_gh323_explicit_reducers.py::
  test_core_does_not_dispatch_the_reducers fails on pristine
  origin/development."  It does not: 22 passed at 45aee17 (this branch's base)
  AND at 4c236d1 (current development).  PRs #635/#640 replaced the substring
  guard with an AST one.  Withdrawn.
* The ledger said the baseline fixture was refreshed for "exactly two entries".
  It was three (cluster_features, sample, people_last7days).  Corrected, with
  the reason only two of 53 entries can carry `v` (cached parquets are written
  before _join_v_from_sample).
* "65-126 clusters per wave for Rural" -> re-derived per wave: 70 / 65 / 122 /
  122 for 2013-14 / 2015-16 / 2018-19 / 2019-20.
* "23 of 2019-20's 794 parish names" -> 793, and "as coded".

4. Recorded, not fixed (scope)
--------------------------------------------------------------

* Consumer-visible <NA> is a third of a column in the two `comm` panel waves,
  not a handful of cells: District 27.8% (2010-11) / 33.0% (2011-12),
  Latitude/Longitude 41.2% / 49.8%.  Now in CONTENTS.org and the ledger.
* 32 of 2019-20's 824 clusters span more than one county/subcounty SPELLING.
  Almost all are the data-entry artefacts the key deliberately ignores, but
  KAKUMIRO/KIKWAYA (8 hh in BUGANGAIZI/KIKWAYA, 1 in BUYANJA/KAKINDO) is a
  genuine second parish of the same name inside one district.  Untouched, and
  unquantified for the other waves -- noted so the silence is not read as a
  clean bill.

Verified: tests/test_uganda_323_grain.py 27 passed; the 323 guard suite 76
passed; test_uganda_invariance's three refreshed entries plus household_roster
(untouched) pass.

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