From 8e526dbdf9b616639965f83a7d2e645520fad009 Mon Sep 17 00:00:00 2001 From: Polichinl Date: Tue, 4 Aug 2026 13:46:45 +0200 Subject: [PATCH] =?UTF-8?q?refactor:=20C-75=20=E2=80=94=20retire=20GaulLoo?= =?UTF-8?q?kupEnricher;=20epic=20#85's=20pandas=20claim=20becomes=20litera?= =?UTF-8?q?l?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The decision was recorded in #90 as its Owner field required. This executes it. What went - views_postprocessing/contract/enrichment.py (317 lines) - tests/test_enrichment.py (39 tests) - docs/CICs/GaulLookupEnricher.md Why: zero production callers, its "build/verification path" justification spent once #90 made that path arrow-native, and its _gather duplicated contract/historical.py's shipping gather -- which four test files cover independently. C-45 is the precedent and was resolved the same way. No coverage of shipping code was lost The two tests elsewhere that imported the class both asserted only that its lookup_version agreed with gaul_lookup.version() -- two readers of one fact, checked against each other. They now read it through the declared reader the delivery uses, which is the half that was ever load-bearing. One guard deleted rather than kept: test_gaul_lookup_access asserted "GaulLookupEnricher" not in the manager source. With the class gone that cannot fail, and a test that cannot fail is decoration (ADR-014 s2). What it protected -- one lookup read per delivery -- is the first assertion in the same function and still bites. Fifteen files, and the sweep is the point The module, its tests, its CIC, the CIC index, two importing test files, the machinery list, the pandas-importer assertion, README's dependency table and package tree, role_and_seams' tree and contract list, ADR-012's ontology row and pandas claim, and three module docstrings. Every one was a live claim about a class that no longer exists, and NONE of the ADR or CIC references would have been caught by any guard -- which is C-80's whole argument. Two stale claims fixed while in the neighbourhood, both actively false rather than merely dated: frame_extraction.py described itself as the sibling of extraction.py (a module deleted in #151) and said "the pandas readers stay for the still-pandas path"; gaul_lookup.py described the enricher in the present tense. What it makes true `grep -rn "^import pandas\|^from pandas" views_postprocessing/ scripts/` returns nothing -- not a runtime import, not a type-only one. Epic #85's claim is literal now rather than nearly-true, and test_doc_accuracy's assertion changed from "exactly one type-only importer" to "none". 333 passed / 40 xfailed / 0 failed (-40: the enricher's 39, plus one retargeted). ruff clean. Register 83/14/69. Closes #85. Closes #93. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 5 +- docs/ADRs/012_revised_ontology.md | 6 +- docs/CICs/GaulLookupEnricher.md | 193 -------- docs/CICs/README.md | 1 - docs/architecture/role_and_seams.md | 5 +- reports/technical_risk_register.md | 84 ++-- tests/test_clone_readiness.py | 1 - tests/test_doc_accuracy.py | 10 +- tests/test_enrichment.py | 419 ------------------ tests/test_gaul_lookup_access.py | 30 +- tests/test_gaul_lookup_fidelity.py | 12 +- tests/test_input_integrity_design_contract.py | 13 +- views_postprocessing/contract/enrichment.py | 317 ------------- .../contract/frame_extraction.py | 31 +- views_postprocessing/contract/gaul_lookup.py | 9 +- views_postprocessing/contract/gaul_schema.py | 2 +- views_postprocessing/unfao/managers/README.md | 2 +- 17 files changed, 117 insertions(+), 1023 deletions(-) delete mode 100644 docs/CICs/GaulLookupEnricher.md delete mode 100644 tests/test_enrichment.py delete mode 100644 views_postprocessing/contract/enrichment.py diff --git a/README.md b/README.md index 4f4cec9..361fbd4 100644 --- a/README.md +++ b/README.md @@ -50,7 +50,7 @@ Requires **Python 3.11–3.14**. | Package | Version | Why | |---------|---------|-----| | `views-pipeline-core` | `>=3.0.0,<4.0.0` (with the `appwrite` extra) | The framework: lifecycle base classes, data loader, dataset container, Appwrite/datastore tools | -| `views-frames` | `>=1.10.2,<2` | The frame data contract — **the live delivery representation** since #126. pandas survives only in `contract/enrichment.py` (the build/verification path) | +| `views-frames` | `>=1.10.2,<2` | The frame data contract — **the live delivery representation** since #126. There is no pandas in this package at all since #90 | | `pyarrow` | `>=16.1.0,<17.0.0` | The wire's serialisation. **Pinned deliberately** — the CVE fix past 17 changes delivered bytes (register C-72) | | *dev group* | `pytest`, `ruff` | Not installed by `pip install views-postprocessing`; `poetry install` includes them | @@ -142,7 +142,6 @@ views-postprocessing/ │ ├── historical.py # the historical artifact, built pandas-free │ ├── gaul_lookup.py # the GAUL asset: path, version, one read │ ├── gaul_schema.py # the 9-column contract, declared as data - │ ├── enrichment.py # GaulLookupEnricher (build/verification path) │ ├── source_metadata.py # producer (datafactory) facts │ ├── store_metadata.py # prediction-store facts │ └── launch_config.py # the delivery mode the launcher must declare @@ -220,7 +219,7 @@ only `.py`; it now scans markdown too.)* |-----|----------------| | [`docs/architecture/role_and_seams.md`](docs/architecture/role_and_seams.md) | **Start here** — role vs the sibling repos + internal seams | | [`docs/ADRs/`](docs/ADRs/) | Architecture decisions (esp. ADR-011 mapper→lookup; ADR-012 ontology) | -| [`docs/CICs/`](docs/CICs/) | Class intent contracts (`UNFAOPostProcessorManager`, `GaulLookupEnricher`) | +| [`docs/CICs/`](docs/CICs/) | Class intent contracts (`UNFAOPostProcessorManager`) | | `reports/technical_risk_register.md` | Tracked risks — C-40 (the remaining pipeline-core inheritance), C-30/C-15 (delivery guards), C-43 (enrichment value verification) | --- diff --git a/docs/ADRs/012_revised_ontology.md b/docs/ADRs/012_revised_ontology.md index 433e70a..9bc11d4 100644 --- a/docs/ADRs/012_revised_ontology.md +++ b/docs/ADRs/012_revised_ontology.md @@ -14,7 +14,7 @@ a **runtime spatial-mapping engine** and **bundled shapefiles** as the authorita stable core. That architecture no longer exists: - [ADR-011](011_replace_runtime_mapper_with_precomputed_lookup.md) replaced the runtime - geopandas mapper with a **precomputed GAUL lookup table** (`GaulLookupEnricher`). + geopandas mapper with a **precomputed GAUL lookup table**. - The mapper, shapefiles, and caching machinery were deleted (C-39 / PR #42). - Input-integrity invariants and structured delivery were added (epic #51). @@ -60,7 +60,7 @@ described the cut-a-repo case; it now says which is which. | **Delivery Invariants** | Representation-free rules over primitives that a delivery must satisfy: coverage, no-collapse, gid parity, observed-range, provenance. Live in `delivery/` — **nothing there imports pandas or views_frames**. *Forecast identity was one of these until 2026-07-31 — see the amendment below.* | Authoritative — they define what a valid delivery is | Stable — changes are governance decisions | | **Representation Seam** | `contract/frame_extraction.py` — turns a `views_frames` frame into the primitives the invariants consume. **One seam.** Its pandas sibling `unfao/extraction.py` was deleted in #151 once the pandas delivery was retired; the two ran as deliberate WET siblings through the migration. | Derived — isolates the representation so invariants stay representation-free | Evolving | | **Wire Mechanism** | `contract/wire/` — the ADR-013 contract: header, shard, sidecar, run manifest, sink, source selection. Partner-neutral: it takes its consumer name and collapse floor as **arguments** (#153). | Authoritative — the contract with the consumer | Stable — changes are contract amendments | -| **Enrichment Asset** | The precomputed GAUL lookup (`data/gaul_lookup.parquet`), its identity in `contract/gaul_lookup.py`, its schema in `contract/gaul_schema.py`, and the keyed gather that joins it (`contract/enrichment.py`, the build/verification path — numpy/pyarrow since #89; see register **C-75** on whether that class should survive at all). | Authoritative for geographic metadata | Stable — rebuilt only when the producer releases new GAUL data | +| **Enrichment Asset** | The precomputed GAUL lookup (`data/gaul_lookup.parquet`), its identity in `contract/gaul_lookup.py`, its schema in `contract/gaul_schema.py`, and the keyed gathers that join it — `contract/historical.py` for the actuals artifact and `contract/wire/sidecar.py` for the §5 sidecar. *(A second gather lived in `contract/enrichment.py` with no production caller; retired in #90, register **C-75**.)* | Authoritative for geographic metadata | Stable — rebuilt only when the producer releases new GAUL data | | **Artifact Builders** | `contract/historical.py` — turns a frame plus the lookup into the partner-facing artifact. | Derived | Evolving | | **External Facts** | Facts read from systems this repo does not own: the producer's (`contract/source_metadata.py` — `last_valid_month_id`, D-07) and the store's (`contract/store_metadata.py`). | Authoritative (the owning system is the source of truth) | Evolving | | **Launch Declarations** | `contract/launch_config.py` — the delivery mode the launcher must declare. Omitting a key is **refused by name**, never inferred (ADR-003, register C-63). | Authoritative | Stable | @@ -73,7 +73,7 @@ described the cut-a-repo case; it now says which is which. 636 lines holding two of everything, and it called `unfao/extraction.py` *"the **single** pandas-aware module"* when pandas lived in three. Both drifted the same way: the ADR described the intended end state of a migration that then stopped one step short. Both are now true — -pandas has **zero runtime importers** (#89 made `contract/enrichment.py`'s a type-only import under `if TYPE_CHECKING` — pandas is in that class's interface, not its implementation) — and the load-bearing ones are +pandas is **absent from the package entirely** (#89 reduced the last one to a type-only import; #90 retired the module that held it — register C-75) — and the load-bearing ones are **mechanically checked** by `tests/test_doc_accuracy.py`, so the next drift fails CI instead of waiting for an audit. diff --git a/docs/CICs/GaulLookupEnricher.md b/docs/CICs/GaulLookupEnricher.md deleted file mode 100644 index a49cfc5..0000000 --- a/docs/CICs/GaulLookupEnricher.md +++ /dev/null @@ -1,193 +0,0 @@ -# Class Intent Contract: GaulLookupEnricher - -**Status:** Draft -**Owner:** PRIO MD&D Team -**Last reviewed:** 2026-06-18 -**Related ADRs:** ADR-011 (replace runtime mapper with precomputed lookup) - ---- - -## 1. Purpose - -> Attach the 9 geographic metadata columns to a prediction frame by merging a -> precomputed GAUL lookup table on the PRIO-GRID cell id. - -It is the lookup-based replacement for `PriogridCountryMapper`'s runtime spatial -enrichment: the spatial computation has already happened upstream (the -views-datafactory area-majority join), so this class does only a table join. - ---- - -## 2. Non-Goals (Explicit Exclusions) - -- This class does **not** perform spatial computation — no geometry, no - shapefiles, no geopandas, no area-majority calculation. -- This class does **not** build the lookup table (that is - `scripts/build_gaul_lookup.py`, run offline). -- This class does **not** fill, impute, or invent metadata for unmatched cells. -- This class does **not** validate the result. Null/coverage enforcement lives on the - delivery path — `contract/historical.assert_metadata_complete` at artifact build and - `delivery/coverage.py` for the region contract. It is **not** the manager's - `_validate()`, which stopped null-gating in #149 and now asserts only that the read - resolved. (Corrected 2026-08-02; PR #200 retired the same claim in the manager's CIC - and this one was left standing.) -- This class does **not** read from the datafactory, viewser, or Appwrite. - ---- - -## 3. Responsibilities and Guarantees - -- Loads exactly one lookup Parquet at construction and verifies it carries the key - plus the 9 contract columns; missing columns raise at construction, and so does an - **empty** lookup — every cell would gather to null and the delivery would then - complain about missing metadata rather than about a missing lookup (S4 / #89). -- Returns the input frame augmented with exactly the 9 columns of - `gaul_schema.METADATA_COLS`: codes numeric, coordinates float, **names and iso as - `object`**. - *Changed in S4 (#89).* They were `category`, inherited from the pandas merge that - read the artifact's dictionary encoding. The gather that replaced it assigns plain - values. Measured on a 200-row output: category 1,795,191 bytes, object 60,061 — - a categorical carries the artifact's full 64,742-entry dictionary whatever the - output size. The builder still writes the artifact with dictionary-encoded names - (`# names/iso categorical (C-32 memory)`); that governs the file, not this output. -- A cell id present in the lookup is enriched with that cell's metadata. -- A cell id **absent** from the lookup yields **null** metadata for that row — - never a sentinel, never a fabricated value (fail-loud downstream). -- Row count, row order **and index** of the input are preserved. Pinned by - `tests/test_enrichment.py::TestFramePropertiesPreserved` across six shapes — - ordered, reversed, duplicated, single, empty, and a non-default index. - (An earlier draft of this line claimed *eight* shapes "including unknown gids", - counting a throwaway development script rather than the committed suite, and naming - a shape that class does not exercise. Unknown gids are covered, for null-value - correctness, by `TestFailLoud` — a different guarantee.) - On **empty** input this is now *more* true than before: the pandas merge replaced the - input's `RangeIndex` with an object-dtype `Index`, where the gather leaves it - untouched. The only behavioural difference found, and it is in the direction the - guarantee above already claimed. - ---- - -## 4. Inputs and Assumptions - -- A lookup Parquet exists at the configured path (default: the committed - `views_postprocessing/data/gaul_lookup.parquet`), indexed by `priogrid_gid`, - containing only fully-complete cells (no nulls, no `-1`, no empty strings). -- The input DataFrame has a column named by `pg_id_col` holding PRIO-GRID cell - ids; a missing `pg_id_col` raises `ValueError`. -- The lookup is the single source of geographic truth — the caller does not - expect this class to reconcile it against any other source. - ---- - -## 5. Outputs and Side Effects - -- Output: the input frame (or, with `only_metadata=True`, just `pg_id_col` + - `time_id_col`) with the 9 metadata columns attached by **keyed gather**. - *Changed in S4 (#89):* this was a pandas left-merge on the lookup's index. The - lookup is now read with pyarrow, sorted once, and addressed by `np.searchsorted`. - Attaching metadata to a frame is a keyed gather, not frame algebra, and it never - needed a merge — which is also what frees the builder to stop writing pandas index - metadata (S5 / #90). -- Public attribute: `lookup_version` — a short, stampable id read from the - lookup's **declared** `lookup_version` metadata key at construction - (`@<8-char source digest>`). Delegates to - `contract.gaul_lookup.version`. The manager stamps it on each delivery so a - delivery is traceable to the exact lookup build. - **It does not degrade.** An artifact carrying no declared key raises - `gaul_lookup.LookupVersionError` (logged at ERROR first, per ADR-008) rather - than returning a placeholder. Until S5 (#186) this returned the string - `"unknown"` whenever views-datafactory's ingestion-ledger shape moved under - it — silently, in the one field register C-15 exists to answer *after* a - suspect delivery. See register **C-60**. -- Side effects: logs the lookup size + version at construction (INFO); logs a - WARNING with the count and sample of unmatched cell ids when any occur; logs - ignored mapper-only kwargs at DEBUG. No file writes, no network. - ---- - -## 6. Failure Modes and Loudness - -- **Raises** at construction if the lookup file is missing or lacks a contract - column. -- **Raises** `ValueError` if `pg_id_col` is not in the input. -- **Does not raise** on unmatched cells — it surfaces them as nulls and logs a - WARNING naming the unknown cells, and separately counting rows that carried no - usable cell id at all. This is deliberate: enforcement is a single point on the - delivery path (`historical.assert_metadata_complete`), so a coverage hole fails - loudly there, not in two places. Passing a sentinel for unmatched cells would be a **bug** (it would - bypass that gate). Aligns with ADR-003 (fail loud on semantic ambiguity). - ---- - -## 7. Boundaries and Interactions - -- Allowed to depend on: numpy, pyarrow, `gaul_schema`, and a local Parquet file. - **pandas is interface-only** — callers hand this class DataFrames and get one back, - but nothing here constructs, reads or joins one, and its import is under - `if TYPE_CHECKING` (S4 / #89). `tests/test_doc_accuracy.py` asserts by AST that the - package has **zero** runtime pandas importers. -- Must **not** depend on: geopandas/shapely, the runtime mapper, the - datafactory, viewser, Appwrite, or any network resource. -- Treats the lookup table as an opaque, trusted artifact produced by the build - script; it does not re-validate the table's spatial correctness. - ---- - -## 8. Examples of Correct Usage - -```python -enricher = GaulLookupEnricher() -out = enricher.enrich_dataframe_with_pg_info( - df.reset_index(), pg_id_col="priogrid_gid", time_id_col="month_id", - only_metadata=True, -) -# out has the 9 metadata columns; unmatched cells are null. -``` - -Signature-compatible with the runtime mapper it replaced (same method name and key -kwargs), which is why the mapper-only kwargs are still accepted and ignored. -**The manager does not call this class** — it has no `enrich` reference at all, and -reads the lookup directly via `gaul_lookup.load()` (register C-66). This example is a -build/verification-path usage. Whether the class should survive that is register -**C-75**. - ---- - -## 9. Examples of Incorrect Usage - -- Filling unmatched cells with `-1`/`""`/`"unknown"` to "avoid validation - errors" — defeats the fail-loud contract and ships wrong data to FAO. -- Using it to enrich against a lookup built for a different region without - expecting nulls for out-of-region cells. -- Calling it expecting spatial recomputation when the lookup is stale — regenerate - the lookup with the build script instead. - ---- - -## 10. Test Alignment - -`tests/test_enrichment.py`: -- **Green:** the 9 columns present; values match the lookup; row count preserved; - codes numeric / coords float. -- **Beige:** lookup integrity (cell count, no nulls, no `-1`, dtypes); coordinate - formula (independent oracle). -- **Red:** unknown cell id and excluded ocean cells yield null; missing - `pg_id_col` raises. - ---- - -## 11. Evolution Notes - -- Stable: the 9-column contract and the fail-loud-on-unmatched behavior (changing - either is a coordinated change across this repo and views-faoapi). -- Expected to change: the lookup's cell-set/region and its source GAUL version, - via re-running the build script. Such regenerations must not change the schema. - ---- - -## End of Contract - -This document defines the **intended meaning** of `GaulLookupEnricher`. - -Changes to behavior that violate this intent are bugs. -Changes to intent must update this contract. diff --git a/docs/CICs/README.md b/docs/CICs/README.md index 177b1d6..4b2b88b 100644 --- a/docs/CICs/README.md +++ b/docs/CICs/README.md @@ -51,7 +51,6 @@ Contracts must be clear enough that: ## Active Contracts - `UNFAOPostProcessorManager.md` — Pipeline orchestration manager (read → transform → validate → save) -- `GaulLookupEnricher.md` — Precomputed GAUL lookup enrichment (ADR-011; replaced the runtime mapper) --- diff --git a/docs/architecture/role_and_seams.md b/docs/architecture/role_and_seams.md index 1a4e99a..9af8da0 100644 --- a/docs/architecture/role_and_seams.md +++ b/docs/architecture/role_and_seams.md @@ -136,7 +136,7 @@ historical actuals arrive as a `FeatureFrame` via pipeline-core's `get_feature_f (#126 — this repo was its first production consumer), the forecast interior is `PredictionFrame`, and `contract/frames.py` is the **live constructor** every interior frame is built through — not the unused adapter it was when this was written, and it supports -`S > 1`. pandas survives in exactly one module, `contract/enrichment.py`, on the +`S > 1`. pandas is gone from the package entirely since #90 retired the last module holding it (register C-75); what remains is on the build/verification path. What **remains** of **register C-40** is narrower than this paragraph implies: the manager is @@ -181,7 +181,6 @@ views_postprocessing/ │ ├── historical.py the historical artifact, built pandas-free │ ├── gaul_lookup.py the GAUL asset: path, version, one read per delivery │ ├── gaul_schema.py the 9-column contract, declared as data -│ ├── enrichment.py GaulLookupEnricher (build/verification path; the last pandas) │ ├── source_metadata.py producer (datafactory) facts, e.g. last_valid_month_id │ ├── store_metadata.py prediction-store facts │ └── launch_config.py the delivery mode the launcher must declare @@ -204,7 +203,7 @@ views_postprocessing/ ## 7. Where to go next - **What was decided and why** → `docs/ADRs/` (esp. ADR-011 mapper→lookup; ADR-012 ontology). -- **Per-class contracts** → `docs/CICs/` (`UNFAOPostProcessorManager`, `GaulLookupEnricher`). +- **Per-class contracts** → `docs/CICs/` (`UNFAOPostProcessorManager`). - **Live risks / open constraints** → the technical risk register (C-40 the pandas gate, C-25/C-30/C-15 the delivery guards). - **The frame/draws future** → #45 (delivery-side draw carrier) and C-40. diff --git a/reports/technical_risk_register.md b/reports/technical_risk_register.md index 99292bf..548f439 100644 --- a/reports/technical_risk_register.md +++ b/reports/technical_risk_register.md @@ -6,8 +6,8 @@ | Owner | Dylan Pinheiro / PRIO MD&D Team | | Last Updated | 2026-08-03 | | Total Concerns | 83 | -| Open Concerns | 15 | -| Resolved Concerns | 68 | +| Open Concerns | 14 | +| Resolved Concerns | 69 | --- @@ -421,41 +421,6 @@ Cross-refs: **C-62** (the transitive dependency drag; the other 31 alerts), **C- --- -### C-75: `GaulLookupEnricher` has no production caller, and now implements a second copy of the delivery path's keyed gather - -| Field | Value | -|-------|-------| -| ID | C-75 | -| Tier | 3 — no correctness impact today: the class is off the delivery path, so a defect in it cannot reach the UN FAO. The cost is that **the verification path and the delivery path now implement the same algorithm twice**, and the tests that check the artifact run through the copy that does *not* ship. A fix applied to one and not the other makes the verification stop verifying what ships — quietly, because both would still pass their own tests. | -| Source | `code-review max` (2026-08-02) — PR #210, five parallel reviewers; two reached this independently | -| Trigger | When a bug is fixed in `contract/historical.py`'s gather (the one that ships), check whether `contract/enrichment.py`'s copy needs the same fix — nothing links them. Also fires at **S5 (#90)**: once the builder is pyarrow-native, the enricher's pandas interface is the last one in the package, and the question "does this class survive?" has to be answered rather than deferred again. | -| Owner | Whoever takes **#90** — the keep-or-retire decision is theirs to make and record, not to defer a third time. Added 2026-08-03: the first draft of this entry named two triggers and no owner, while citing ADR-014 §4 in its own body. This register had already learned that twice — *"a deferral needs an owner and a trigger, not just a reason"* (Cluster L) and *"a decision awaiting an owner, not a task awaiting effort"* (epic #181 closeout). | -| Location | `views_postprocessing/contract/enrichment.py` (the whole class; `_gather` specifically); the shipping twin is `views_postprocessing/contract/historical.py:54-68` | - -**Verified, not inferred (2026-08-02):** `grep -rn "GaulLookupEnricher\|enrich_dataframe_with_pg_info"` across the package finds **zero** production callers — the two hits are docstring mentions in `gaul_lookup.py`. The manager calls `gaul_lookup.load()` directly and has zero `enrich` references. **C-66**'s resolution already said this plainly: *"the pandas enricher leaves the delivery path entirely."* - -**What PR #210 did, and why that raises the question.** S4 (#89) rewrote this class's lookup side from a pandas merge to a numpy/pyarrow keyed gather: a measured dtype analysis, an empty-lookup guard, a mutation-proven bug fix, a corrected CIC, and five reviewers' attention. All of it spent on a method with no reachable caller outside its own test suite. The engineering is sound; what is missing is anyone having **decided** that the class should exist. - -**The duplication is the concrete consequence.** `_gather`'s `argsort → searchsorted → clip → equality-mask` is the same shape as `historical.py:54-68`. The policies differ deliberately — `historical` **raises** on an absent gid (*"geography must never silently vanish"*), the enricher returns nulls for the downstream gate to catch — so extracting a shared helper would mean parameterising the failure policy, which is the guessed abstraction **WET before DRY** exists to prevent. Two copies that are understood is the right call *today*. The trigger above is what stops "today" lasting indefinitely, per **ADR-014 §4**. - -**The precedent is C-45**, `unfao/frames.py`: an unused adapter carried on no live path, resolved by deleting it. This is the same shape with a different module, and the same question — keep it as the declared verification/reference implementation, or retire it and let the fidelity suite test `historical.py` directly. - -**DECISION 2026-08-04 (#90), which this entry's Owner field required of whoever took it: RETIRE.** - -The conditions are no longer arguable. The class has **zero production callers** — only three test files import it. Its last stated justification was "the build/verification path", and #90 rewrote that path arrow-native without touching it, so the justification is spent. It holds the package's **last pandas reference** (a `TYPE_CHECKING` import), which is the one thing standing between epic #85 and an honest close. And its `_gather` duplicates `contract/historical.py`'s shipping gather, which is independently covered by four test files. - -**C-45 is the precedent and it was resolved by deleting.** Same shape, different module. - -**Not executed in #90, deliberately.** The retirement touches ten files — the module, its 39 tests, references in two other test files, its CIC, ADR-012, `gaul_lookup.py`'s docstring, the machinery list in `test_clone_readiness.py`, and the pandas-importer assertion in `test_doc_accuracy.py`. Folding that into a builder rewrite would mix a behaviour-preserving change with a large deletion, which is the thing epic #148's S5 explicitly refused to do. It is the next change, not a later one. - -**Epic #85 and tracking #93 stay open until it lands**, because their claim — pandas pushed to the seams — only becomes true when this module is gone. - -**Deliberately NOT registered from the same review** (defects in unmerged code, all fixed in #210 before merge rather than tracked): a NaN gid crashing the warning path, the unvalidated int64 coercion at both ends, the AST guard's `else`-branch blind spot, ADR-012's stale pandas-merge claim, and three CIC claims retired elsewhere by #200. The register tracks standing risk; a defect fixed before it ships is not one. They are recorded in the PR. - -Cross-refs: **C-45** (RESOLVED — the same shape, resolved by deletion), **C-66** (RESOLVED — established the enricher left the delivery path), **C-40** (which calls `enrichment.py` and `extraction.py` together *"the retired-in-place `enrichment.py`/`extraction.py` legacy seams"*), **#89** / **#90** / epic **#85**, ADR-014 §4. - ---- - ### C-80: The doc-accuracy scan exempts ADRs and CICs — the two artifact classes that define the contracts | Field | Value | @@ -664,6 +629,51 @@ See also C-40 (the inheritance/representation coupling this migration unwinds), ## Resolved Concerns +### C-75: `GaulLookupEnricher` has no production caller, and now implements a second copy of the delivery path's keyed gather — RESOLVED + +| Field | Value | +|-------|-------| +| ID | C-75 | +| Tier | 3 — no correctness impact today: the class is off the delivery path, so a defect in it cannot reach the UN FAO. The cost is that **the verification path and the delivery path now implement the same algorithm twice**, and the tests that check the artifact run through the copy that does *not* ship. A fix applied to one and not the other makes the verification stop verifying what ships — quietly, because both would still pass their own tests. | +| Source | `code-review max` (2026-08-02) — PR #210, five parallel reviewers; two reached this independently | +| Trigger | When a bug is fixed in `contract/historical.py`'s gather (the one that ships), check whether `contract/enrichment.py`'s copy needs the same fix — nothing links them. Also fires at **S5 (#90)**: once the builder is pyarrow-native, the enricher's pandas interface is the last one in the package, and the question "does this class survive?" has to be answered rather than deferred again. | +| Owner | Whoever takes **#90** — the keep-or-retire decision is theirs to make and record, not to defer a third time. Added 2026-08-03: the first draft of this entry named two triggers and no owner, while citing ADR-014 §4 in its own body. This register had already learned that twice — *"a deferral needs an owner and a trigger, not just a reason"* (Cluster L) and *"a decision awaiting an owner, not a task awaiting effort"* (epic #181 closeout). | +| Location | `views_postprocessing/contract/enrichment.py` (the whole class; `_gather` specifically); the shipping twin is `views_postprocessing/contract/historical.py:54-68` | + +**Verified, not inferred (2026-08-02):** `grep -rn "GaulLookupEnricher\|enrich_dataframe_with_pg_info"` across the package finds **zero** production callers — the two hits are docstring mentions in `gaul_lookup.py`. The manager calls `gaul_lookup.load()` directly and has zero `enrich` references. **C-66**'s resolution already said this plainly: *"the pandas enricher leaves the delivery path entirely."* + +**What PR #210 did, and why that raises the question.** S4 (#89) rewrote this class's lookup side from a pandas merge to a numpy/pyarrow keyed gather: a measured dtype analysis, an empty-lookup guard, a mutation-proven bug fix, a corrected CIC, and five reviewers' attention. All of it spent on a method with no reachable caller outside its own test suite. The engineering is sound; what is missing is anyone having **decided** that the class should exist. + +**The duplication is the concrete consequence.** `_gather`'s `argsort → searchsorted → clip → equality-mask` is the same shape as `historical.py:54-68`. The policies differ deliberately — `historical` **raises** on an absent gid (*"geography must never silently vanish"*), the enricher returns nulls for the downstream gate to catch — so extracting a shared helper would mean parameterising the failure policy, which is the guessed abstraction **WET before DRY** exists to prevent. Two copies that are understood is the right call *today*. The trigger above is what stops "today" lasting indefinitely, per **ADR-014 §4**. + +**The precedent is C-45**, `unfao/frames.py`: an unused adapter carried on no live path, resolved by deleting it. This is the same shape with a different module, and the same question — keep it as the declared verification/reference implementation, or retire it and let the fidelity suite test `historical.py` directly. + +**DECISION 2026-08-04 (#90), which this entry's Owner field required of whoever took it: RETIRE.** + +The conditions are no longer arguable. The class has **zero production callers** — only three test files import it. Its last stated justification was "the build/verification path", and #90 rewrote that path arrow-native without touching it, so the justification is spent. It holds the package's **last pandas reference** (a `TYPE_CHECKING` import), which is the one thing standing between epic #85 and an honest close. And its `_gather` duplicates `contract/historical.py`'s shipping gather, which is independently covered by four test files. + +**C-45 is the precedent and it was resolved by deleting.** Same shape, different module. + +**Not executed in #90, deliberately.** The retirement touches ten files — the module, its 39 tests, references in two other test files, its CIC, ADR-012, `gaul_lookup.py`'s docstring, the machinery list in `test_clone_readiness.py`, and the pandas-importer assertion in `test_doc_accuracy.py`. Folding that into a builder rewrite would mix a behaviour-preserving change with a large deletion, which is the thing epic #148's S5 explicitly refused to do. It is the next change, not a later one. + +**Epic #85 and tracking #93 stay open until it lands**, because their claim — pandas pushed to the seams — only becomes true when this module is gone. + +**Deliberately NOT registered from the same review** (defects in unmerged code, all fixed in #210 before merge rather than tracked): a NaN gid crashing the warning path, the unvalidated int64 coercion at both ends, the AST guard's `else`-branch blind spot, ADR-012's stale pandas-merge claim, and three CIC claims retired elsewhere by #200. The register tracks standing risk; a defect fixed before it ships is not one. They are recorded in the PR. + +Cross-refs: **C-45** (RESOLVED — the same shape, resolved by deletion), **C-66** (RESOLVED — established the enricher left the delivery path), **C-40** (which calls `enrichment.py` and `extraction.py` together *"the retired-in-place `enrichment.py`/`extraction.py` legacy seams"*), **#89** / **#90** / epic **#85**, ADR-014 §4. + +**RESOLVED 2026-08-04 — retired, as the decision recorded above required.** `views_postprocessing/contract/enrichment.py` and its 39 tests are deleted, along with `docs/CICs/GaulLookupEnricher.md`. + +**No coverage of shipping code was lost.** The two tests elsewhere that imported the class both asserted only that its `lookup_version` agreed with `gaul_lookup.version()` — two readers of one fact, checked against each other. They now read the fact through the declared reader the delivery itself uses, which is the half that was ever load-bearing. The gather it duplicated is `contract/historical.py`'s, covered independently by four test files. + +**One guard was deleted rather than kept.** `test_gaul_lookup_access.py` asserted `"GaulLookupEnricher" not in` the manager source. With the class gone that assertion cannot fail, and a test that cannot fail is decoration (ADR-014 §2). What it protected — one lookup read per delivery — is the first assertion in the same function and still bites. + +**Fifteen files, and the sweep is the point.** The module, its tests, its CIC, the CIC index, two test files that imported it, the machinery list, the pandas-importer assertion, `README.md`'s dependency table and package tree, `role_and_seams.md`'s tree and contract list, ADR-012's ontology row and its pandas claim, and two module docstrings. Every one of those was a live claim about a class that no longer exists — which is the argument for C-80: none of the ADR or CIC references would have been caught by any guard. + +**What it makes true.** `grep -rn "^import pandas\|^from pandas" views_postprocessing/ scripts/` now returns nothing at all — not a runtime import, not a type-only one. Epic **#85**'s claim, *pandas pushed to the seams*, is finally literal rather than nearly-true, and #85 and #93 close with this. + +--- + ### C-76: `build_gaul_lookup.py` will write an empty lookup without complaint — RESOLVED | Field | Value | diff --git a/tests/test_clone_readiness.py b/tests/test_clone_readiness.py index 549bb77..79413fe 100644 --- a/tests/test_clone_readiness.py +++ b/tests/test_clone_readiness.py @@ -64,7 +64,6 @@ "views_postprocessing.contract.historical", "views_postprocessing.contract.gaul_lookup", "views_postprocessing.contract.gaul_schema", - "views_postprocessing.contract.enrichment", "views_postprocessing.contract.launch_config", "views_postprocessing.contract.source_metadata", "views_postprocessing.contract.store_metadata", diff --git a/tests/test_doc_accuracy.py b/tests/test_doc_accuracy.py index 3e295d0..1669d72 100644 --- a/tests/test_doc_accuracy.py +++ b/tests/test_doc_accuracy.py @@ -253,9 +253,11 @@ def test_pandas_is_not_imported_at_runtime_anywhere_in_the_package(): "the representation the migration removed. If it is genuinely needed, put it " "behind `if TYPE_CHECKING` or say in ADR-012 why it is not." ) - assert type_only == ["contract/enrichment.py"], ( - f"the type-only pandas imports moved: {type_only}. Not necessarily wrong — but " - "ADR-012 names the seam, so update it rather than letting the claim drift." + assert type_only == [], ( + f"a type-only pandas import reappeared: {type_only}. There is no pandas in this " + "package at all since #90 retired contract/enrichment.py (register C-75) — epic " + "#85's claim is now literal, not nearly-true. If one is genuinely needed, say in " + "ADR-012 why before adding it back." ) @@ -448,7 +450,7 @@ def test_the_ban_covers_the_post_148_deletions_and_spares_the_live_lookalike(): ``_BANNED`` says a deletion PR that does not extend it has not finished. No test can substitute for that; only the habit can. 2. **Too broad** — `FAO_PGMDataset` is a *views-faoapi* class, alive and correctly - cited in `contract/gaul_schema.py` and `contract/enrichment.py`. A careless + cited in `contract/gaul_schema.py`. A careless `PGMDataset` pattern would ban a true statement about another repo's code. """ for deleted in ( diff --git a/tests/test_enrichment.py b/tests/test_enrichment.py deleted file mode 100644 index ca1ea6c..0000000 --- a/tests/test_enrichment.py +++ /dev/null @@ -1,419 +0,0 @@ -"""Tests for the lookup-based enricher (ADR-011, Stage 1). - -Run against the committed gaul_lookup.parquet — no datafactory checkout or -shapefiles required. These tests pin the contract the enricher must satisfy so -that swapping it in for the runtime mapper (Stage 3) is invisible downstream. -""" - -import pandas as pd -import logging - -import numpy as np -import pyarrow as pa -import pyarrow.parquet as pq -import pytest - -from views_postprocessing.contract import gaul_lookup -from views_postprocessing.contract.enrichment import ( - GaulLookupEnricher, - METADATA_COLS, -) - -# The 5 africa_me_legacy ocean cells that have no GAUL assignment — they must -# NOT be in the lookup (they are excluded upstream by the land_gaul region). -OCEAN_CELLS = [62356, 94776, 99027, 107733, 107742] - -CODE_COLS = ["admin1_gaul1_code", "admin1_gaul0_code", "admin2_gaul2_code"] -NAME_COLS = ["admin1_gaul1_name", "admin1_gaul0_name", "admin2_gaul2_name", - "country_iso_a3"] - - -@pytest.fixture(scope="module") -def enricher(): - return GaulLookupEnricher() - - -@pytest.fixture(scope="module") -def lookup(): - """The committed artifact as a pandas frame, built here rather than reached for. - - Until S4 (#89) this was ``enricher._lookup`` — the enricher's own private pandas - frame. It no longer has one: the lookup side is numpy + pyarrow, and pandas is a - type-only import there. These tests are about the **artifact**, not the enricher, - so they load it directly. - - The ``set_index`` is deliberate and forward-looking. ``to_pandas()`` currently - restores ``priogrid_gid`` as the index from the parquet's embedded pandas - metadata — metadata that **S5 (#90) removes** when the builder becomes - pyarrow-native. Handling both shapes means these tests do not have to change - again then. - """ - df = gaul_lookup.load().to_pandas() - if "priogrid_gid" in df.columns: - df = df.set_index("priogrid_gid") - return df - - -class TestLookupIntegrity: - """The committed lookup must be clean by construction.""" - - def test_expected_cell_count(self, lookup): - # land_gaul region: 64,742 fully-complete land cells. - assert len(lookup) == 64_742 - - def test_no_nulls_anywhere(self, lookup): - assert int(lookup.isna().sum().sum()) == 0 - - def test_no_minus_one_sentinel_in_codes(self, lookup): - for c in CODE_COLS: - assert (lookup[c] != -1).all(), f"{c} has -1 sentinel" - - def test_no_empty_strings_in_names(self, lookup): - for c in NAME_COLS: - assert (lookup[c].astype(str).str.len() > 0).all() - - def test_has_exactly_the_nine_contract_columns(self, lookup): - assert list(lookup.columns) == METADATA_COLS - - def test_dtypes(self, lookup): - for c in CODE_COLS: - assert pd.api.types.is_numeric_dtype(lookup[c]), c - for c in ["pg_xcoord", "pg_ycoord"]: - assert pd.api.types.is_float_dtype(lookup[c]), c - for c in NAME_COLS: - assert isinstance(lookup[c].dtype, pd.CategoricalDtype), c - - def test_ocean_cells_excluded(self, lookup): - for gid in OCEAN_CELLS: - assert gid not in lookup.index, f"ocean cell {gid} should be excluded" - - -class TestCoordinateFormula: - """pg_xcoord/pg_ycoord must follow the PRIO-GRID 0.5-degree formula.""" - - def test_known_coordinates(self, lookup): - for gid in lookup.index[:50]: - x = -180.0 + ((gid - 1) % 720) * 0.5 + 0.25 - y = -90.0 + ((gid - 1) // 720) * 0.5 + 0.25 - assert lookup.loc[gid, "pg_xcoord"] == pytest.approx(x) - assert lookup.loc[gid, "pg_ycoord"] == pytest.approx(y) - - -class TestEnricherContract: - """enrich() must reproduce the 9-column contract the manager consumes.""" - - def _frame(self, gids): - return pd.DataFrame({ - "priogrid_gid": gids, - "month_id": [100] * len(gids), - }) - - def test_all_metadata_cols_present(self, enricher, lookup): - gids = lookup.index[:10].tolist() - out = enricher.enrich_dataframe_with_pg_info( - self._frame(gids), pg_id_col="priogrid_gid", time_id_col="month_id", - ) - for c in METADATA_COLS: - assert c in out.columns - - def test_no_nulls_for_known_gids(self, enricher, lookup): - gids = lookup.index[:100].tolist() - out = enricher.enrich_dataframe_with_pg_info( - self._frame(gids), pg_id_col="priogrid_gid", time_id_col="month_id", - ) - for c in METADATA_COLS: - assert out[c].isna().sum() == 0, c - - def test_values_match_lookup(self, enricher, lookup): - gids = lookup.index[:20].tolist() - out = enricher.enrich_dataframe_with_pg_info( - self._frame(gids), pg_id_col="priogrid_gid", time_id_col="month_id", - ).set_index("priogrid_gid") - for gid in gids: - assert out.loc[gid, "country_iso_a3"] == lookup.loc[gid, "country_iso_a3"] - assert out.loc[gid, "admin2_gaul2_code"] == lookup.loc[gid, "admin2_gaul2_code"] - - def test_row_count_preserved(self, enricher, lookup): - gids = lookup.index[:7].tolist() - out = enricher.enrich_dataframe_with_pg_info( - self._frame(gids), pg_id_col="priogrid_gid", time_id_col="month_id", - ) - assert len(out) == 7 - - def test_codes_numeric_coords_float(self, enricher, lookup): - gids = lookup.index[:5].tolist() - out = enricher.enrich_dataframe_with_pg_info( - self._frame(gids), pg_id_col="priogrid_gid", time_id_col="month_id", - ) - for c in CODE_COLS: - assert pd.api.types.is_numeric_dtype(out[c]), c - for c in ["pg_xcoord", "pg_ycoord"]: - assert pd.api.types.is_float_dtype(out[c]), c - - -class TestFailLoud: - """Unknown / excluded cells must surface as NaN, not a sentinel.""" - - def test_unknown_gid_yields_null(self, enricher, lookup): - good = int(lookup.index[0]) - df = pd.DataFrame({"priogrid_gid": [good, 999_999], "month_id": [1, 1]}) - out = enricher.enrich_dataframe_with_pg_info( - df, pg_id_col="priogrid_gid", time_id_col="month_id", - ).set_index("priogrid_gid") - assert out.loc[good, "country_iso_a3"] is not None - assert pd.isna(out.loc[999_999, "country_iso_a3"]) - for c in CODE_COLS: - assert pd.isna(out.loc[999_999, c]) - - def test_ocean_cell_yields_null(self, enricher): - df = pd.DataFrame({"priogrid_gid": OCEAN_CELLS, - "month_id": [1] * len(OCEAN_CELLS)}) - out = enricher.enrich_dataframe_with_pg_info( - df, pg_id_col="priogrid_gid", time_id_col="month_id", - ) - # Every ocean cell is absent from the lookup -> all-null metadata. - assert out["country_iso_a3"].isna().all() - - def test_missing_pg_id_col_raises(self, enricher): - with pytest.raises(ValueError, match="not found"): - enricher.enrich_dataframe_with_pg_info( - pd.DataFrame({"x": [1]}), pg_id_col="priogrid_gid", - ) - - -# ── the guarantees the CIC states, pinned (S4 / #89; ADR-014 §1) ───────────── -# -# The CIC claims row/order/index preservation "verified across eight input shapes" -# and names the output dtypes. That verification was a throwaway script run once -# during development — a guarantee resting on a claim rather than on a check, which -# is precisely what ADR-014 §1 forbids, written by the story after the ADR landed. -# These commit it. - - -class TestConstructionRefusals: - """Degenerate lookups must fail at construction, naming the lookup.""" - - def _write(self, path, gids): - cols = {"priogrid_gid": pa.array(gids, pa.int64())} - for c in METADATA_COLS: - cols[c] = pa.array( - [0.0] * len(gids) if c in ("pg_xcoord", "pg_ycoord") or c.endswith("_code") - else ["x"] * len(gids), - pa.float64() if c in ("pg_xcoord", "pg_ycoord") or c.endswith("_code") else pa.string(), - ) - t = pa.table(cols).replace_schema_metadata({b"lookup_version": b"t@00000000"}) - pq.write_table(t, path) - return path - - def test_an_empty_lookup_is_refused_at_construction(self, tmp_path): - """It used to raise IndexError from inside the gather instead — the guard - `(len(self._keys) > 0) & (...)` read as a guard and was not one, because `&` - evaluates both operands.""" - path = self._write(tmp_path / "empty.parquet", []) - with pytest.raises(ValueError, match="is empty"): - GaulLookupEnricher(path) - - def test_a_null_key_is_refused_at_construction(self, tmp_path): - """A null key would coerce to the same sentinel as an unusable query id, the - two would collide, and the row would be reported FOUND — receiving another - cell's metadata. That is the fabricated value this module forbids (cf. C-35).""" - cols = {"priogrid_gid": pa.array([1, None, 3], pa.int64())} - for c in METADATA_COLS: - numeric = c in ("pg_xcoord", "pg_ycoord") or c.endswith("_code") - cols[c] = pa.array([0.0] * 3 if numeric else ["x"] * 3, - pa.float64() if numeric else pa.string()) - path = tmp_path / "nullkey.parquet" - pq.write_table(pa.table(cols).replace_schema_metadata({b"lookup_version": b"t@0"}), path) - with pytest.raises(ValueError, match="null values"): - GaulLookupEnricher(path) - - -class TestUnusableCellIds: - """A value that is not a cell id gathers to null — never to a guessed cell. - - Each case was verified against the pandas merge this replaced; the behaviour - below is that merge's, not an invention. The drifted-float case is the one that - matters most: a blind `astype(np.int64)` truncates `54220.000000001` to `54220` - and matches a real, *different* cell, silently. - """ - - def _out(self, enricher, col): - return enricher.enrich_dataframe_with_pg_info( - pd.DataFrame({"priogrid_gid": col, "month_id": [1] * len(col)}), - pg_id_col="priogrid_gid", time_id_col="month_id", - ) - - def test_a_missing_gid_yields_null_and_does_not_crash(self, enricher, lookup): - good = lookup.index[:2].tolist() - out = self._out(enricher, [float(good[0]), float("nan"), float(good[1])]) - assert len(out) == 3 - assert out["country_iso_a3"].isna().sum() == 1 - - def test_a_non_integral_gid_never_matches_a_neighbouring_cell(self, enricher, lookup): - gid = int(lookup.index[100]) - out = self._out(enricher, [float(gid) + 1e-9]) - assert out["country_iso_a3"].isna().all(), ( - "a drifted float matched a cell — truncation turned it into a different, " - "real gid and fabricated that cell's geography" - ) - - def test_pandas_na_yields_null_rather_than_a_bare_typeerror(self, enricher, lookup): - good = lookup.index[:2].tolist() - out = self._out(enricher, pd.array([good[0], pd.NA, good[1]], dtype="Int64")) - assert len(out) == 3 and out["country_iso_a3"].isna().sum() == 1 - - def test_an_unusable_id_cannot_match_the_cell_it_was_substituted_with(self, tmp_path): - """The `& usable` term in `_gather`, proven load-bearing (ADR-014 §2). - - It survived removal against the committed artifact: an unusable id converts to - `0`, and `0` is not a real gid, so the key comparison already failed. That made - it look like decoration. It is not — it is the only thing standing between an - unusable id and the row it was substituted onto, and the substitute is a real - key the moment a lookup contains gid `0`. - - Without the mask, the NaN row below silently receives cell 0's geography. That - is the fabricated value this module's docstring forbids, produced from an input - the caller never wrote. - """ - cols = {"priogrid_gid": pa.array([0, 5, 9], pa.int64())} - for c in METADATA_COLS: - numeric = c in ("pg_xcoord", "pg_ycoord") or c.endswith("_code") - cols[c] = pa.array( - [0.0] * 3 if numeric else ["ZERO", "FIVE", "NINE"], - pa.float64() if numeric else pa.string(), - ) - path = tmp_path / "withzero.parquet" - pq.write_table( - pa.table(cols).replace_schema_metadata({b"lookup_version": b"t@00000000"}), path - ) - - out = GaulLookupEnricher(path).enrich_dataframe_with_pg_info( - pd.DataFrame({"priogrid_gid": [5.0, float("nan"), 9.0], "month_id": [1, 1, 1]}), - pg_id_col="priogrid_gid", time_id_col="month_id", - ) - assert out["country_iso_a3"].tolist() == ["FIVE", None, "NINE"], ( - "an unusable id matched the cell it was substituted with — `& usable` is " - "what prevents that, and this is the input that proves it" - ) - - def test_a_string_gid_column_is_refused_rather_than_parsed(self, enricher, lookup): - """A string gid is a declaration error, and the merge this replaced said so. - - `pandas` raised *"You are trying to merge on object and int64 columns. If you - wish to proceed you should use pd.concat"*. An - earlier draft of `_as_cell_ids` used `float(value)`, which happily parsed - `"54220"` and matched — inference where the old path declared (ADR-003). - """ - gid = str(int(lookup.index[0])) - out = self._out(enricher, [gid]) - assert out["country_iso_a3"].isna().all(), ( - f"the string {gid!r} was parsed into a cell id; a string column is a " - "declaration error, not a gid to be guessed at" - ) - - @pytest.mark.parametrize( - "label,array", - [ - # Each case must enter the branch it is testing. An earlier draft built - # them all with dtype=object, which routed every one through the - # element-wise branch — so the int and float bounds it claimed to test - # were never executed, and removing them left the suite green. - ("uint64 max (int branch)", np.array([np.iinfo(np.uint64).max], dtype=np.uint64)), - ("1e30 (float branch)", np.array([1e30], dtype=np.float64)), - ("inf (float branch)", np.array([np.inf], dtype=np.float64)), - ("-inf (float branch)", np.array([-np.inf], dtype=np.float64)), - ("string (object branch)", np.array(["123"], dtype=object)), - ("bool (object branch)", np.array([True], dtype=object)), - ("float exactly 2**63", np.array([2.0**63], dtype=np.float64)), - ("timedelta64", np.array([1], dtype="timedelta64[ns]")), - ], - ) - def test_out_of_range_and_non_numeric_values_are_unusable_not_wrapped( - self, enricher, label, array - ): - """Outside int64 the cast WRAPS rather than raising, and a wrapped id flagged - valid is exactly the fabricated value this conversion exists to prevent. - - `uint64` max becomes `-1`; `1e30` and `float(2**63)` become `INT64_MIN`. Each - passes a naive finite-and-integral test, so the bounds are separate conditions - rather than consequences of the others. - - Asserted through the PUBLIC method. An earlier draft called the private - `_as_cell_ids` and checked its mask — pinning an implementation detail rather - than the guarantee a caller relies on, and skipping `_gather`'s use of that - mask entirely. Every dtype below is reachable through a DataFrame column, so - there was never a reason to reach inside. - """ - out = enricher.enrich_dataframe_with_pg_info( - pd.DataFrame({"priogrid_gid": array, "month_id": [1] * len(array)}), - pg_id_col="priogrid_gid", time_id_col="month_id", - ) - assert out["country_iso_a3"].isna().all(), ( - f"{label} produced a match — an id the caller never wrote was treated as " - "a cell and that cell's geography was fabricated onto the row" - ) - - # AND at the mask, because the public assertion above cannot tell "refused" - # from "accepted but wrapped to an id that happens to be absent" — both give - # null. Reverting the float bound or re-admitting timedelta64 left the public - # assertion green while the value was silently accepted; only this sees it. - # The public check is the caller's guarantee; this one is the mutation proof. - _, usable = GaulLookupEnricher._as_cell_ids(array) - assert not usable.any(), ( - f"{label} was ACCEPTED as a cell id (it only looked refused because the " - f"wrapped id is absent from this lookup — against a lookup that contains " - f"it, this row would receive that cell's geography)" - ) - - def test_the_warning_names_unknown_cells_but_invents_no_id_for_unusable_ones( - self, enricher, lookup, caplog - ): - good = int(lookup.index[0]) - with caplog.at_level(logging.WARNING): - self._out(enricher, [float(good), float("nan"), 999999.0]) - message = " ".join(r.getMessage() for r in caplog.records) - assert "999999" in message, "the genuinely unknown cell must be named" - assert "no usable cell id" in message - assert "[0]" not in message, ( - "an unusable id was reported as cell 0 — a cell nobody asked about" - ) - - -class TestFramePropertiesPreserved: - """Row count, order and index survive the gather. Was CIC prose; now a check.""" - - @pytest.mark.parametrize("shape", ["ordered", "reversed", "duplicated", "single", "empty"]) - def test_row_count_and_order_are_preserved(self, enricher, lookup, shape): - gids = { - "ordered": lookup.index[:50].tolist(), - "reversed": lookup.index[:50].tolist()[::-1], - "duplicated": lookup.index[:5].tolist() * 3, - "single": lookup.index[:1].tolist(), - "empty": [], - }[shape] - out = enricher.enrich_dataframe_with_pg_info( - pd.DataFrame({"priogrid_gid": gids, "month_id": [1] * len(gids)}), - pg_id_col="priogrid_gid", time_id_col="month_id", - ) - assert len(out) == len(gids) - assert out["priogrid_gid"].tolist() == gids - - def test_a_non_default_index_is_preserved(self, enricher, lookup): - gids = lookup.index[:20].tolist() - df = pd.DataFrame({"priogrid_gid": gids, "month_id": [1] * 20}, - index=range(1000, 1020)) - out = enricher.enrich_dataframe_with_pg_info( - df, pg_id_col="priogrid_gid", time_id_col="month_id") - assert out.index.equals(df.index), ( - "the index moved. The pandas merge this replaced mutated it on empty " - "input; the gather must not mutate it on any input." - ) - - def test_name_columns_are_object_not_categorical(self, enricher, lookup): - """The CIC states this dtype. It changed in #89 and was measured, not guessed.""" - out = enricher.enrich_dataframe_with_pg_info( - pd.DataFrame({"priogrid_gid": lookup.index[:5].tolist(), "month_id": [1] * 5}), - pg_id_col="priogrid_gid", time_id_col="month_id", - ) - for c in NAME_COLS: - assert out[c].dtype == object, f"{c} is {out[c].dtype}, CIC says object" diff --git a/tests/test_gaul_lookup_access.py b/tests/test_gaul_lookup_access.py index d0de6c6..91ee742 100644 --- a/tests/test_gaul_lookup_access.py +++ b/tests/test_gaul_lookup_access.py @@ -153,19 +153,25 @@ def test_the_manager_reads_the_lookup_once_and_threads_it(): assert "_DEFAULT_LOOKUP" not in source, ( "the manager must not import the enricher's private path constant (C-68)" ) - assert "GaulLookupEnricher" not in source, ( - "the contract delivery does not use the pandas enricher; instantiating it " - "loads the lookup a third time into a representation nothing reads (C-66)" - ) + # The `GaulLookupEnricher` assertion that stood here was removed with the class in + # #90 (register C-75). A guard against a symbol that no longer exists cannot fail, + # and a test that cannot fail is decoration (ADR-014 §2). What it protected — one + # read per delivery — is the first assertion in this function and still bites. -def test_the_enricher_still_works_and_agrees_on_the_stamp(): - """The enricher is not retired — it remains the build/verification path's object. +def test_the_artifacts_identity_comes_from_one_declared_reader(): + """The stamp has exactly one source, and it is the one the delivery reads. - What changed is that it no longer *owns* the artifact's identity. Its - ``lookup_version`` must still report exactly what ``gaul_lookup.version`` does, or - provenance would differ depending on which path produced it. + **This test used to assert that `GaulLookupEnricher.lookup_version` agreed with + `gaul_lookup.version()`** — two readers of the same fact, checked against each + other. The enricher was retired in #90/C-75 as an object with no production caller, + so the agreement it policed no longer has two sides. What survives is the property + that mattered: the version the delivery stamps into its provenance is read from the + artifact, and resolves. """ - from views_postprocessing.contract.enrichment import GaulLookupEnricher - - assert GaulLookupEnricher().lookup_version == gaul_lookup.version() + version = gaul_lookup.version() + assert version, "the lookup must declare a version; C-60 made this raise rather than degrade" + assert "@" in version, ( + f"lookup_version must be '@', got {version!r} — the delivery " + "carries this verbatim into its provenance record (C-15)." + ) diff --git a/tests/test_gaul_lookup_fidelity.py b/tests/test_gaul_lookup_fidelity.py index 821ddfb..ee023c4 100644 --- a/tests/test_gaul_lookup_fidelity.py +++ b/tests/test_gaul_lookup_fidelity.py @@ -218,10 +218,16 @@ def test_lookup_declares_its_provenance(lookup_meta): def test_lookup_version_stamp_resolves(lookup): - """The stamp the delivery provenance carries (C-15) must not be 'unknown'.""" - from views_postprocessing.contract.enrichment import GaulLookupEnricher + """The stamp the delivery provenance carries (C-15) must not be 'unknown'. - version = GaulLookupEnricher(_LOOKUP).lookup_version + Read through ``gaul_lookup.version`` — the declared reader the delivery itself uses. + It was read through ``GaulLookupEnricher`` until that object was retired (#90, C-75); + the enricher was never on the delivery path, so this now checks the same fact through + the code that ships. + """ + from views_postprocessing.contract import gaul_lookup + + version = gaul_lookup.version(_LOOKUP) assert version != "unknown", ( "lookup_version resolved to 'unknown' — the delivery would ship untraceable " "provenance (C-60). The lookup's embedded source_provenance is absent or reshaped." diff --git a/tests/test_input_integrity_design_contract.py b/tests/test_input_integrity_design_contract.py index b86416e..125fc8a 100644 --- a/tests/test_input_integrity_design_contract.py +++ b/tests/test_input_integrity_design_contract.py @@ -79,9 +79,16 @@ def test_manager_does_not_inherit_forecasting_model_manager(): # housekeeping -------------------------------------------------------------- -def test_enrichment_does_not_call_a_dataframe_a_prediction_frame(): - src = (_PKG / "contract" / "enrichment.py").read_text().lower() - assert "prediction frame" not in src +# `test_enrichment_does_not_call_a_dataframe_a_prediction_frame` was removed with +# `contract/enrichment.py` in #90 (register C-75). It asserted that one module did not +# call a DataFrame a "prediction frame" — vocabulary hygiene in a module that no longer +# exists. +# +# Nothing replaces it, and that is the honest position rather than an oversight: the +# confusion it guarded against was possible only while a pandas object sat next to a +# frame-shaped API. `test_delivery_invariants_are_pandas_free` above, and the +# now-empty type-only-pandas assertion in `test_doc_accuracy.py`, together mean there +# is no DataFrame left in the package to mislabel. def test_no_lingering_mapping_directory(): diff --git a/views_postprocessing/contract/enrichment.py b/views_postprocessing/contract/enrichment.py deleted file mode 100644 index 75baa77..0000000 --- a/views_postprocessing/contract/enrichment.py +++ /dev/null @@ -1,317 +0,0 @@ -"""Lookup-based geographic enrichment (ADR-011). - -Drop-in replacement for the runtime spatial mapper's -``enrich_dataframe_with_pg_info``. Instead of loading 774 MB of shapefiles and -computing spatial intersections at run time, it merges a precomputed lookup -table (built by ``scripts/build_gaul_lookup.py`` from the views-datafactory's -area-majority GAUL parquets) onto the input DataFrame by PRIO-GRID cell id. - -No geopandas, no shapefiles, no spatial computation. The lookup contains only -fully-complete cells; an unknown or incomplete cell id gathers to null, so the -delivery's null gate still crashes (fail-loud) rather than shipping a hole. This -is intentional and matches the old mapper's behaviour (it returned ``None`` for -such cells). - -**The lookup side is pandas-free (S4 / #89, epic #85).** It is read with pyarrow -and held as numpy arrays plus a sorted key index; attaching metadata to a frame is -a **keyed gather**, not frame algebra, and it never needed a pandas merge. The -input frame is still whatever the caller passes — this class is the *build and -verification* path's object, and its callers hand it DataFrames. What changed is -that the ~880 KiB artifact is no longer materialised as a pandas frame, and the -join no longer depends on the artifact carrying pandas index metadata — which is -what unblocks S5 (#90) making the builder pyarrow-native. - -Produces exactly the 9-column contract enforced at -the delivery's artifact builders and at -views-faoapi ``handlers.py`` (``FAO_PGMDataset._METADATA_COLS``). -""" - -from __future__ import annotations - -import logging -from pathlib import Path -from typing import TYPE_CHECKING - -import numpy as np -import pyarrow as pa - -from views_postprocessing.contract import gaul_lookup - -if TYPE_CHECKING: # pragma: no cover — pandas is in this module's INTERFACE, not its - import pandas as pd # implementation. Callers hand it DataFrames; nothing here - # constructs, reads or joins one. Removing the runtime import is the point of - # S4 (#89): the ~880 KiB lookup is no longer materialised as a pandas frame, and the - # join no longer needs the artifact to carry pandas index metadata. -from views_postprocessing.contract.gaul_schema import METADATA_COLS - -logger = logging.getLogger(__name__) - -# The artifact's identity lives in `gaul_lookup` (#152, C-68) — this alias keeps the -# enricher's own default working without re-deriving the path. -_DEFAULT_LOOKUP = gaul_lookup.LOOKUP_PATH - -#: int64 bounds, named because they are a correctness condition rather than trivia: -#: a value outside them does not raise on cast, it WRAPS, and a wrapped id that is -#: flagged valid is the fabricated value this module forbids. -_INT64_MIN = int(np.iinfo(np.int64).min) -_INT64_MAX = int(np.iinfo(np.int64).max) - -#: Float-comparison bounds. Deliberately NOT the int bounds above: 2**63-1 is odd and -#: unrepresentable in float64, so comparing a float against it rounds up to 2**63 and -#: lets through the very values the bound excludes. 2**63 and -2**63 are both powers -#: of two and exact, so the float check is `>= -2**63` and `< 2**63`. -_INT64_MIN_F = -(2.0**63) -_INT64_MAX_EXCLUSIVE_F = 2.0**63 - -#: The lookup's key column. Named once — the artifact calls it `priogrid_gid`, the -#: wire calls it `priogrid_id` (§5.1), and confusing the two is a silent join failure. -_KEY = "priogrid_gid" - - -class GaulLookupEnricher: - """Merge precomputed GAUL metadata onto an input DataFrame by cell id.""" - - def __init__(self, lookup_path: str | Path | None = None) -> None: - self._lookup_path = Path(lookup_path) if lookup_path else _DEFAULT_LOOKUP - if not self._lookup_path.exists(): - err_msg = ( - f"GAUL lookup table not found at {self._lookup_path}. " - f"Build it with scripts/build_gaul_lookup.py." - ) - logger.error(err_msg) - raise FileNotFoundError(err_msg) - table = gaul_lookup.load(self._lookup_path) - missing = [c for c in (_KEY, *METADATA_COLS) if c not in table.column_names] - if missing: - err_msg = f"Lookup table is missing contract columns: {missing}" - logger.error(err_msg) - raise ValueError(err_msg) - - if table.column(_KEY).null_count: - err_msg = ( - f"GAUL lookup at {self._lookup_path} has null values in {_KEY!r}. A " - "null key cannot identify a cell, and coercing it would make it " - "collide with any unusable id on the query side — the row would then " - "be reported as FOUND and receive another cell's metadata. Rebuild " - "with scripts/build_gaul_lookup.py. (That script does not check the " - "key column either — its null check runs after the key becomes the " - "index, and DataFrame.isna() does not inspect an index. A null key is " - "unreachable there only because the earlier astype('int64') raises. " - "See register C-76.)" - ) - logger.error(err_msg) # ADR-008: logged persistently AND raised - raise ValueError(err_msg) - - if table.num_rows == 0: - err_msg = ( - f"GAUL lookup at {self._lookup_path} is empty. Every cell would gather " - "to null and the delivery would fail downstream complaining about " - "missing metadata rather than about a missing lookup. Rebuild it with " - "scripts/build_gaul_lookup.py." - ) - logger.error(err_msg) # ADR-008: logged persistently AND raised - raise ValueError(err_msg) - - # Sort the whole table once by key, in arrow, so the gather below is a binary - # search per row rather than a scan. `take` reorders every column together — - # doing it column-by-column in python was measurably quadratic and is exactly - # the mistake this comment exists to stop the next person repeating. - keys = table.column(_KEY).to_numpy(zero_copy_only=False).astype(np.int64) - order = np.argsort(keys, kind="stable") - table = table.take(pa.array(order)) - self._keys = keys[order] - # One `to_pylist` per column, not per row. Lists rather than numpy arrays - # because the columns are of mixed kind (float codes, string names) and the - # output is assembled per column anyway. - # - # DTYPE CHANGE, deliberate and measured (S4 / #89). The pandas merge this - # replaced produced `category` name columns, inherited from the artifact's - # dictionary encoding; assigning lists produces `object`. On a 200-row output: - # category 1,795,191 bytes vs object 60,061 — a categorical carries the full - # 64,742-entry dictionary whatever the output size, so this is far lighter for - # the small frames this object actually sees and heavier only past the point - # where the dictionary amortises. The builder's `# names/iso categorical (C-32 - # memory)` note governs the ARTIFACT; it never governed this method's output. - self._values = {col: table.column(col).to_pylist() for col in METADATA_COLS} - self.lookup_version = self._read_version(self._lookup_path) - logger.info( - "Loaded GAUL lookup: %d cells from %s (version=%s)", - len(self._keys), self._lookup_path, self.lookup_version, - ) - - @staticmethod - def _as_cell_ids(gids) -> tuple[np.ndarray, np.ndarray]: - """``(int64 ids, usable mask)`` — DECLARED, never coerced (ADR-003). - - A cell id is an integer. Anything that is not one — a missing value, a - non-integral float, a ``pd.NA`` — is marked **unusable** rather than cast, - and unusable ids gather to null exactly as an unknown gid does. The - downstream gate then sees a hole, which is what it exists for. - - **Why not just cast.** ``np.asarray(gids, dtype=np.int64)`` looks equivalent - and is not, in three ways found in review of #210: - - - a ``NaN`` becomes ``INT64_MIN`` with only a ``RuntimeWarning``. If a lookup - key were ever null it would take the same sentinel, the two would collide, - and the row would be reported FOUND — receiving another cell's metadata. - That is the fabricated value this module's docstring forbids (cf. C-35). - - a non-integral float **truncates silently**: ``54220.000000001`` becomes - ``54220`` and matches a real, *different* cell. Verified against the pandas - merge this replaced: it returns null there, and even warns. A silent wrong - match is strictly worse than the crash it would replace. - - a ``pd.NA`` raises a bare ``TypeError`` from numpy — no log, no - contract-shaped error, unlike every other guard in this file. - """ - arr = np.asarray(gids) - - if arr.dtype.kind in ("i", "u"): - # In range, or it is not the id the caller wrote. uint64 max silently - # wraps to -1 under `astype(np.int64)` and would be flagged VALID. - usable = (arr >= _INT64_MIN) & (arr <= _INT64_MAX) - return np.where(usable, arr, 0).astype(np.int64), usable - - if arr.dtype.kind == "f": - # Finite, integral, AND representable. `1e30` passes the first two and - # then overflows the cast to INT64_MIN — an id nobody asked for, marked - # valid. Same silent-coercion class this method exists to remove. - # STRICTLY below 2**63, not `<= _INT64_MAX`. `_INT64_MAX` is 2**63-1, - # which is odd and NOT representable in float64 — comparing against it - # promotes to float and rounds UP to 2**63, so the bound admitted exactly - # the values it was added to exclude. `float(2**63)` passed every check - # and then wrapped to INT64_MIN, flagged valid. `_INT64_MIN` has no - # equivalent hole: -2**63 is a power of two and exactly representable. - # - # `isfinite` is kept for intent and is SUBSUMED — `inf` fails the upper - # bound, `NaN` fails `arr == rint(arr)` (NaN equals nothing). It is the - # one condition here that survives its own removal, so it is defensive - # rather than load-bearing; the other three each fail the suite. - usable = ( - np.isfinite(arr) - & (arr == np.rint(arr)) - & (arr >= _INT64_MIN_F) - & (arr < _INT64_MAX_EXCLUSIVE_F) - ) - return np.where(usable, arr, 0).astype(np.int64), usable - - # object / pandas-nullable. Accept only values that ALREADY ARE integers. - # - # An earlier draft used `float(value)`, which parses. That accepted the string - # `"54220"` as a cell id — where the pandas merge this replaced raised - # `ValueError: You are trying to merge on object and int64 columns`. A string - # gid column is a declaration error and the old path said so; parsing it is - # inference (ADR-003) and it is the same defect as the drifted float, just - # pointing the other way. `bool` is excluded for the same reason: `True` is - # not cell 1. - ids = np.zeros(len(arr), dtype=np.int64) - usable = np.zeros(len(arr), dtype=bool) - for i, value in enumerate(arr): - # `np.timedelta64` IS an `np.integer` instance — a numpy quirk, and - # `np.datetime64` is not, so the hole was specific to that one type. A - # duration is not a cell id any more than `True` is cell 1. - if isinstance(value, (bool, np.timedelta64)): - continue - if not isinstance(value, (int, np.integer)): - continue - if _INT64_MIN <= int(value) <= _INT64_MAX: - ids[i], usable[i] = int(value), True - return ids, usable - - def _gather(self, gids) -> dict: - """The metadata for each gid, warning about the ones that got none. - - A sorted-key ``searchsorted`` rather than a hash map: the lookup is 64,742 - rows read once per process, and the gather is over the delivery's row count. - Absent gids yield ``None`` in every column — the null the downstream gate - exists to catch, not a sentinel it would pass. An id that is not a usable - cell id is treated the same way: absent, never guessed at. - """ - wanted, usable = self._as_cell_ids(gids) - # `self._keys` is non-empty — __init__ refuses an empty lookup. An earlier draft - # guarded with `(len(self._keys) > 0) & (...)`, which READS as a guard and is - # not one: `&` evaluates both operands, so the index happened regardless and an - # empty lookup raised IndexError from inside the gather rather than ValueError - # from the constructor. Guard where the condition is knowable, not where it bites. - idx = np.clip(np.searchsorted(self._keys, wanted), 0, len(self._keys) - 1) - # `& usable` is NOT belt-and-braces, and no substitute value would make it so. - # Unusable ids substitute to 0, but int64 reserves nothing — the lookup is - # arbitrary data and may legally contain 0, or -1, or INT64_MIN, or whatever - # else one might pick instead. The mask is what carries correctness; the - # substitute is only a placeholder. Pinned by - # `test_an_unusable_id_cannot_match_the_cell_it_was_substituted_with`. - found = (self._keys[idx] == wanted) & usable - out = { - col: [vals[i] if hit else None for i, hit in zip(idx, found)] - for col, vals in self._values.items() - } - - # Warn HERE rather than handing the caller three arrays to reassemble one - # message. `__init__` already co-locates detection and logging at every guard - # (ADR-008); this is the same shape. An earlier draft returned four values, - # three of which existed only to build the string below. - absent = ~found - n_unmapped = int(absent.sum()) - if n_unmapped: - unknown = sorted({int(i) for i, miss, ok in zip(wanted, absent, usable) if miss and ok}) - n_unusable = int((absent & ~usable).sum()) - detail = f"unknown cells {unknown[:20]}" if unknown else "no unknown cells" - if n_unusable: - detail += f"; {n_unusable} row(s) carried no usable cell id" - logger.warning( - "%d/%d rows have no lookup match (will fail validation): %s", - n_unmapped, len(wanted), detail, - ) - return out - - @staticmethod - def _read_version(path: Path) -> str: - """The lookup's build stamp. Delegates to ``gaul_lookup.version`` (#152) — - this was a ``@staticmethod`` that never touched the instance, i.e. a fact - about the artifact, not about the enricher.""" - return gaul_lookup.version(path) - - def enrich_dataframe_with_pg_info( - self, - df: pd.DataFrame, - pg_id_col: str = "priogrid_gid", - time_id_col: str = "month_id", - only_metadata: bool = True, - **ignored_mapper_kwargs, - ) -> pd.DataFrame: - """Return ``df`` with the 9 metadata columns merged in by cell id. - - Signature mirrors the mapper's method so the manager call site changes - minimally. Mapper-only kwargs (``batch_size``, ``use_multiprocessing``, - ``show_progress`` …) are accepted and ignored — a table join needs none - of them — but any unrecognised kwarg is logged at debug so a genuine - caller mistake is not wholly silent. - - Cells absent from the lookup get NaN metadata (fail-loud downstream). - """ - if ignored_mapper_kwargs: - logger.debug( - "GaulLookupEnricher ignoring mapper-only kwargs: %s", - sorted(ignored_mapper_kwargs), - ) - if pg_id_col not in df.columns: - err_msg = f"Column '{pg_id_col}' not found in DataFrame" - logger.error(err_msg) - raise ValueError(err_msg) - - if only_metadata: - keep = [pg_id_col] - if time_id_col in df.columns: - keep.append(time_id_col) - base = df[keep].copy() - else: - base = df.copy() - - gids = base[pg_id_col].to_numpy() - gathered = self._gather(gids) - - merged = base.copy() - for col in METADATA_COLS: - merged[col] = gathered[col] - return merged - - # Convenience alias for new call sites that don't need the legacy name. - enrich = enrich_dataframe_with_pg_info diff --git a/views_postprocessing/contract/frame_extraction.py b/views_postprocessing/contract/frame_extraction.py index ad266fa..2241fbf 100644 --- a/views_postprocessing/contract/frame_extraction.py +++ b/views_postprocessing/contract/frame_extraction.py @@ -1,24 +1,21 @@ """Frame-native representation seam: extract primitives from a views-frames frame. -The **frame counterpart** to ``extraction.py`` (the pandas seam). It returns the *same* -primitives — sets of ints, numpy month arrays — so the representation-free +Returns primitives — sets of ints, numpy month arrays — so the representation-free ``views_postprocessing.delivery`` invariants consume them unchanged. -Per the migration design (epic #85): pandas and views-frames do **not** coexist at runtime, -so these are deliberately **siblings** of the pandas readers in ``extraction.py``, not a -replacement, and there is **no shared ``Extractor`` Protocol** (a polymorphic interface -nobody dispatches on would be speculative — YAGNI/ISP). When the forecast interior moves to a -frame (S3 / #88), the manager calls *these*; the pandas readers stay for the still-pandas -historical path (gated on C-40 / S7). - -Scope: the readers the forecast interior needs — distinct cells and months from the frame's -index. Deliberately **not** here yet (no speculative code): -- the pandas→``(N, S)`` sample-array unpacker — added when rusty_bucket (#143) declares the - layout (the seam will be *told* the layout, never sniff it); -- a frame-native ``unmapped_cell_count`` — geographic metadata lives on the pandas enriched - frame, not the value frame, until the enrichment moves off pandas (S4 / #89); -- a frame-native ``drop_months_above`` — the observed-range clip is on the *historical* - frame, which is gated on the inbound retirement (S7 / #92). +**This module was one of a pair and is now alone.** It was written as the frame-native +sibling of a pandas seam (``extraction.py``), deliberately duplicated rather than +abstracted while both representations were live: pandas and views-frames did not coexist +at runtime, and a shared ``Extractor`` Protocol nobody dispatched on would have been +speculative. That migration is finished — the pandas seam was deleted in #151, and #90 +retired the last module holding a pandas import at all. There is no sibling and no +still-pandas path; what remains is this one seam, and the docstring is corrected rather +than left describing a pair. + +Scope: the readers the forecast interior needs — distinct cells and months from the +frame's index. Deliberately **not** here (no speculative code): a sample-array unpacker, +added when the producer declares its layout — the seam will be *told* the layout, never +sniff it (ADR-003). """ from __future__ import annotations diff --git a/views_postprocessing/contract/gaul_lookup.py b/views_postprocessing/contract/gaul_lookup.py index 23f17bb..7090812 100644 --- a/views_postprocessing/contract/gaul_lookup.py +++ b/views_postprocessing/contract/gaul_lookup.py @@ -7,11 +7,10 @@ times per run** (C-66) — once eagerly into a pandas enricher it never used, then twice more through pyarrow. -The artifact's identity does not belong to the enricher. ``GaulLookupEnricher`` is -one *consumer* of this asset (the pandas merge used by the build/verification path); -the contract delivery is another, and it wants the table and the stamp, not the -merge. Splitting them lets the delivery read the file once, in arrow, with no pandas -anywhere on the path. +The artifact's identity never belonged to that class, which is why splitting them was +the fix: the delivery wants the table and the stamp, not a merge. It reads the file +**once**, in arrow, with no pandas anywhere on the path. The enricher itself was retired +in #90 (register **C-75**) once it was established it had no production caller. Verified by ``tests/test_gaul_lookup_fidelity.py``: the committed artifact matches views-datafactory's authoritative GAUL parquets value-for-value, its key is unique, diff --git a/views_postprocessing/contract/gaul_schema.py b/views_postprocessing/contract/gaul_schema.py index c2b9513..1a49e49 100644 --- a/views_postprocessing/contract/gaul_schema.py +++ b/views_postprocessing/contract/gaul_schema.py @@ -1,6 +1,6 @@ """Single source of truth for the GAUL enrichment schema and PRIO-GRID geometry. -Shared by the production enricher (enrichment.py) and the build/diff tooling +Shared by the delivery's artifact builders and the build/diff tooling (scripts/) so the 9-column contract, the datafactory->contract rename map, and the PRIO-GRID coordinate formula are defined exactly once. diff --git a/views_postprocessing/unfao/managers/README.md b/views_postprocessing/unfao/managers/README.md index 701f8f1..0cc915f 100644 --- a/views_postprocessing/unfao/managers/README.md +++ b/views_postprocessing/unfao/managers/README.md @@ -91,5 +91,5 @@ Appwrite). - [`role_and_seams.md`](../../../docs/architecture/role_and_seams.md) — role + seams - [CIC: UNFAOPostProcessorManager](../../../docs/CICs/UNFAOPostProcessorManager.md) — class contract -- [CIC: GaulLookupEnricher](../../../docs/CICs/GaulLookupEnricher.md) — the build/verification path for the GAUL lookup. **Not on this manager's call graph** (register C-75); geography is attached by `contract/historical.py` and `contract/wire/sidecar.py` +- Geography is attached by `contract/historical.py` (the actuals artifact) and `contract/wire/sidecar.py` (the §5 GAUL sidecar), each a keyed gather over the lookup this manager reads once. *(A `GaulLookupEnricher` CIC was linked here until #90; that class had no production caller and was retired — register C-75.)* - ADR-011 (mapper → lookup), ADR-012 (current ontology)