From 810a9dbf2779e02e415a2b6ad873f683aec5b2d7 Mon Sep 17 00:00:00 2001 From: Polichinl Date: Thu, 17 Sep 2026 11:28:28 +0200 Subject: [PATCH] fix(mapping): refuse a CM frame with two country_ids under one ISO code (#290) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The bundled metadata carries dead/live country pairs sharing an ISO code (Sudan 59/245, Serbia 230/233, … — 11 groups, 9 of which join the shapefile). If a forecast frame carries both members, the isoab→ADM0_A3 merge yields two rows per polygon and `pivot_table(aggfunc="first")` plus the hover-props `drop_duplicates` silently pick one: reproduced — with [59, 245] the map drew the DEAD Sudan's value and the hover reported 59. Ascending country_id makes the dead id win in 4 of the 9 joinable pairs; the pick is arbitrary in all nine. Guard at the single CM chokepoint (build_mapping_dataframe, after the geometry check, before the frame is stored): >1 distinct country_id per ADM0_A3 → logger.error + ValueError naming each collided code and its ids (register C-227; ADR-008). Raise, not warn: a forecast frame's entity set is fixed at its origin, so it never legitimately carries both. Per CODE, not per (code, month): real transitions put both ids in the same month anyway (Sudan 59+245 at 379), and the hover props key on code alone. Detection only — resolution belongs to the producer (pipeline-core ADR-064). PGM raster/image paths untouched (unreachable at CM). RED→GREEN in tests/test_mapping_characterization.py::TestDuplicateIsoIsLoud against the REAL bundle (both 59 and 245 resolve to SDN there); control: 245 alone renders one row. The guard's first catch was our own test double: the 191-country fixture e2e tests labelled the map via `mock_isoab_for_index`, which cycles 10 ISO codes positionally — ~19 country_ids per code, a collision real data never has. Those tests now use the real bundled metadata (offline since C-22; zero collisions on the fixture); the double carries a docstring warning. Latent today (every real forecast fixture carries one member per pair); elevate C-227 to Tier 1 on the first observed dead-over-live draw. Register: C-227 registered + resolved, Cluster F; 91 concerns (68 resolved, 23 open). Closes #290 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01N7YBtiS2kSVj27spMKDYfh --- reports/technical_risk_register.md | 20 ++++++++-- tests/conftest.py | 8 +++- tests/test_e2e_fixture.py | 16 +++++--- tests/test_mapping_characterization.py | 54 ++++++++++++++++++++++++++ views_reporting/mapping/mapping.py | 37 +++++++++++++++++- 5 files changed, 124 insertions(+), 11 deletions(-) diff --git a/reports/technical_risk_register.md b/reports/technical_risk_register.md index 30c22cb..92ec49b 100644 --- a/reports/technical_risk_register.md +++ b/reports/technical_risk_register.md @@ -2,7 +2,7 @@ **Last updated:** 2026-09-17 **Governing ADR:** ADR-010 (Technical Risk Register) -**Entry count:** 90 concerns (67 resolved, 23 open) + 5 disagreements (4 resolved) +**Entry count:** 91 concerns (68 resolved, 23 open) + 5 disagreements (4 resolved) --- @@ -28,7 +28,7 @@ Root causes shared by multiple concerns. Resolving the root tends to dissolve or | **C — PRIO-GRID scale discipline** | Repo handles ~260K-cell geodata without size discipline, at rest and at render | C-23 (shapefile in git), C-26 ✓ (render OOM → three-tier ladder), C-205 ✓ (globe legibility), C-209 (raster guard measures data rows, not the rendered lattice — reopened 2026-07-15, RESOLVED 2026-07-16), C-189 ✓ / C-190 ✓ (aggregation/omission methodology guards — closed superseded, ADR-021) | **Render side resolved (2026-06-29, globe epic #188): C-26 ✓ + C-205 ✓ — the three-tier choropleth → raster-heatmap → PNG ladder with a coastline overlay renders the full globe within budget. Remaining open: C-23 (56 MB cell shapefile committed — at-rest discipline).** C-189 ✓ / C-190 ✓ closed 2026-07-31 (superseded: ADR-021's per-cell-faithful raster/PNG designs the coarsening/subset path out; re-register if such a feature is ever proposed). | | **D — Ingestion-layer boundary** | loaders/ crossed the pipeline-core boundary ahead of governance | C-30, C-31, C-32 | Resolved (PR #82) | | **E — Legacy transform machinery** | RESOLVED (2026-06-20, #119) — `DatasetTransformationModule` removed + direct `polars` declaration dropped (polars stays transitive via pipeline-core) | C-25 ✓ (+ resolved C-10, C-04, C-02) | ✓ Resolved | -| **F — Value-correctness & contract assurance** | The load → compute → render → reconcile chain is tested for shape / does-not-crash, not for value equality, contract conformance, or input completeness | C-29 ✓ (render fidelity), C-35 ✓ (MAP/HDI correctness — render path + PosteriorDistributionAnalyzer both on the views-frames tower + law tests; RESOLVED, ADR-019 / #157), C-185 (`*_map` is a tower tip, not a MAP — naming debt), C-39 ✓ (metadata accessors untested — resolved, `tests/test_metadata_accessors.py`), C-41 ✓ (canonical-token contract test), C-116 ✓ (multi-match → silent wrong value — RESOLVED: fail-loud default + visible "ambiguous" cell + collision contract test), C-111 ✓ (input completeness — RESOLVED 2026-07-31, S4 #266), C-113 (actuals provenance), C-112 (bundled-data staleness — forward, pairs with C-22), C-186 ✓ (views-frames version → forecast-output drift), C-208 ✓ (gapped-lattice cell stretch/misplacement — rendering-geometry faithfulness, RESOLVED 2026-07-16), C-192 ✓ (cross-repo eval contract untested — RESOLVED 2026-07-31, executable seam test) (+ resolved C-01, C-11) | Open — highest latent severity; mostly "write the missing correctness/contract test" (an assurance sprint). Phase 1 (correctness/contract core) landed 2026-06-28: C-29 ✓ (render==source fidelity test), C-41 ✓ (canonical-metric contract test), C-111 values-half guarded. C-186 ✓ (tower behavioural-regime tests, Phase 2, 2026-06-28). Remaining: C-112 (guards live — watch the regen cadence), C-113 / C-185 (non-test); C-111 ✓ resolved (coverage checks, S4 #266); C-192 ✓ resolved (seam test landed, S2 #264); C-39 ✓ resolved (accessor tests landed, 2026-07). | +| **F — Value-correctness & contract assurance** | The load → compute → render → reconcile chain is tested for shape / does-not-crash, not for value equality, contract conformance, or input completeness | C-29 ✓ (render fidelity), C-35 ✓ (MAP/HDI correctness — render path + PosteriorDistributionAnalyzer both on the views-frames tower + law tests; RESOLVED, ADR-019 / #157), C-185 (`*_map` is a tower tip, not a MAP — naming debt), C-39 ✓ (metadata accessors untested — resolved, `tests/test_metadata_accessors.py`), C-41 ✓ (canonical-token contract test), C-116 ✓ (multi-match → silent wrong value — RESOLVED: fail-loud default + visible "ambiguous" cell + collision contract test), C-111 ✓ (input completeness — RESOLVED 2026-07-31, S4 #266), C-113 (actuals provenance), C-112 (bundled-data staleness — forward, pairs with C-22), C-186 ✓ (views-frames version → forecast-output drift), C-208 ✓ (gapped-lattice cell stretch/misplacement — rendering-geometry faithfulness, RESOLVED 2026-07-16), C-192 ✓ (cross-repo eval contract untested — RESOLVED 2026-07-31, executable seam test), C-227 ✓ (CM choropleth silently drew the dead country on duplicate ISO codes — RESOLVED 2026-09-17, loud guard at the merge chokepoint; latent, never observed) (+ resolved C-01, C-11) | Open — highest latent severity; mostly "write the missing correctness/contract test" (an assurance sprint). Phase 1 (correctness/contract core) landed 2026-06-28: C-29 ✓ (render==source fidelity test), C-41 ✓ (canonical-metric contract test), C-111 values-half guarded. C-186 ✓ (tower behavioural-regime tests, Phase 2, 2026-06-28). Remaining: C-112 (guards live — watch the regen cadence), C-113 / C-185 (non-test); C-111 ✓ resolved (coverage checks, S4 #266); C-192 ✓ resolved (seam test landed, S2 #264); C-39 ✓ resolved (accessor tests landed, 2026-07). | | **G — Partner-deliverable readiness** | Reports are built for internal preview, not yet hardened as a standalone, traceable, decision-appropriate *partner artifact* | C-28 ✓ (offline / self-contained — RESOLVED, vendored Tailwind, #132), C-34 ✓ (provenance / auditability — RESOLVED, footer stamp, #131), C-187 ✓ (vendored-CSS class coverage — RESOLVED, #132), C-188 ✓ (machine-readable provenance — RESOLVED, #132), C-109 (decision-appropriate uncertainty), C-214 ✓ (plotly-injection heuristics — RESOLVED 2026-07-31, S3 #265) | Open — C-109 (decision-appropriate uncertainty, partially delivered 2026-07: the #230 P(any) exceedance map layer) is the live remainder; C-214 ✓ resolved (S3 #265); offline + provenance now shipped | | **H — Published-state qualification gap (dev-tandem ↔ PyPI seam)** | Contracts, pins, and names are qualified against dev branches moving in tandem, never against **published** artifacts — the coordinated publish (#179) is the single event where every deferred qualification fires at once | C-213 ✓ (root — vpc pin outran PyPI; import surface canonicalized + guarded 2026-07-31 S1 #263; ordering half resolved 2026-08-03 when vpc 3.0.0 published pre-qualified), C-192 ✓ (cross-repo seam untested — resolved S2 #264), C-36 (upstream transitive pins — re-probed at vpc's 2026-08-03 publish: envelope matches ours, ingester3==2.1.1 unchanged so the levenshtein wall persists), C-185 (rename waits on a coordinated cross-repo change), C-186 ✓ residual (b) (exact-pin views-frames for release), C-211 (partial — cross-repo decision, not publish-gated) | Open (added review-rr 2026-07-31) — mitigation is the #179 release-gate runbook (largely written); C-192's executable contract test landed (S2 #264); C-36 stays upstream-blocked regardless. **SECOND CONFIRMED FIRING (2026-08-02, v0.3.3):** 0.3.2's published `requires_dist` pin `views-evaluation>=0.4.0,<1.0.0` — qualified only against the dev-tandem git source — excluded views-evaluation **1.0.0** the day it hit PyPI, breaking platform co-resolution at the coordinated publish (caught by a sibling-repo agent). Fixed same-day: pin `>=1.0.0,<2.0.0`, interim git-source retired (its documented trigger fired), suite green against the RELEASED 1.0.0. The root cause has now bitten in production twice (vpc pin C-213(a) + this); every remaining dev-tandem git source (vpc, until their 3.0.0 publishes) carries the same latent defect until re-qualified at its publish. **CYCLE CLOSED (2026-08-03, #279):** vpc 3.0.0 published to PyPI carrying the pre-qualified pins (the vpc#313 warning worked — no third firing); the last dev-tandem git source retired, `views-reporting==0.3.3` co-resolves from PyPI alone, suite green against the released 3.0.0. **No dev-tandem git sources remain anywhere in pyproject/uv.lock** — the cluster's generating condition is discharged for this release cycle; it re-arises only if a future unreleased-dependency tandem is reintroduced. **THIRD EXPOSURE — of the published-metadata facet only, caught as a prediction (2026-09-17, #289):** no dev-tandem was reintroduced and the `<2.0.0` ceiling WAS correctly qualified against the released 1.0.0; what is Cluster-H-shaped is the second half below. views-evaluation's pandas-free 2.0.0 (their #66/#72) and vpc's coming `^2.0.0` pin (vpc#515) would make our `<2.0.0` ceiling unsatisfiable in the shared env. Source widened to a SPAN pin `>=1.0.0,<3.0.0` — deliberately NOT the requested `>=2.0.0`, because 2.0.0 was unpublished at fix time (PyPI latest 1.0.0) and a floor naming an unpublished version is exactly C-213(a)'s mistake. Safety evidence: everything we import lives in `evaluation.metric_frame` (+ `metric_catalog` in one test), and no 2.0.0 story changes the MetricFrame format or the names we import (S8's gate: "MetricFrame format unchanged"; `[frames]` survives; the only merged changes to that module since 1.0.0 are to the provenance stamp's value — the `+g` suffix and S1's bounding of it — which we treat as opaque text). **Not yet pre-empted at the seam that matters:** the shared env resolves from PyPI, where 0.3.3's immutable metadata still says `<2.0.0` — vpc#515's "after views-reporting has widened" precondition is met only by a **0.3.4 publish**, which collides with the publish-last rule and is therefore a decision, not a mechanical step. Re-qualification trigger: views-evaluation 2.0.0 on PyPI → re-lock, suite, then the `>=2.0.0` floor + 0.3.4. | | **I — Post-B2 eval-report comparison seam (the baselines-vanishing braid)** | The B2 inversion shipped with its production wiring unverified end-to-end: consumer and producer disagree on frame location, declarations/targets/pins/envs each independently blank comparison rows, and every failure mode is silenced or mislabeled — so the deliverable degraded for months with green CI | C-215 (root, Tier 1 — subject-rooted source + seam test pinning it), C-216 (target drift + stranded fixes), C-217 (pins/envs make the seam unreachable), C-218 (Tier 1 — zero-report silence), C-219 (declaration/announce layer), C-220 (post-fix detonation), C-221 (corruption path), C-222 (vintage semantics dropped), C-223 (cache short-circuit / no baseline orchestrator), C-224 (3.0.0 classification drop), C-225/C-226 (ride-alongs) | Open (registered 2026-08-17 from /code-review max on the baselines-vanishing mystery; reconstruction VERIFIED). Fix sequencing constraint: C-220/C-225(AP-sort) must land WITH the C-215/C-216 resolution fix; C-192's locked seam contract must be corrected in the same change. Cross-repo: vpc stage + views-models configs/branches own most mechanisms; this repo owns the template/source/announce layers and the seam test. | @@ -376,6 +376,20 @@ C-34 (provenance) and C-28 (offline) now anchor **Cluster G** (partner-deliverab ## Resolved Concerns +### C-227: CM choropleth silently draws the DEAD country when a frame carries two country_ids under one ISO code — RESOLVED + +| Field | Value | +|-------|-------| +| ID | C-227 | +| Tier | 2 — silent-wrong-map class (a value drawn over the wrong polygon with no signal), but LATENT: every real FORECAST fixture carries exactly ONE member of each dead/live pair (verified 2026-09-17; the historical fixtures carry both, but never reach the map). **Elevate to Tier 1 on the first observed dead-over-live draw.** | +| Source | Issue #290 (filed from the pipeline-core side while investigating vpc#509); mechanism reproduced empirically this session | +| Trigger | When a producer emits a dissolved entity beside its successor in one CM frame — views-r2darts2 ≥0.2.0 forecasts every entity ever seen (r2darts2#49); pipeline-core 3.3.0 ADR-064 now refuses such predictions at ITS boundary, but nothing at OUR boundary did until this fix. Also: any future producer/aggregator that keys rows by something other than the live `country_id`. | +| Location | `views_reporting/mapping/mapping.py::build_mapping_dataframe` CM branch (the single chokepoint; merge `left_on="isoab", right_on="ADM0_A3"`) → `_plot_interactive_map` `pivot_table(index=ADM0_A3, aggfunc="first")` + `drop_duplicates(ADM0_A3)` for hover props, and `_plot_static_map`'s overpaint (last drawn wins). Data: `views_reporting/metadata/data/country.parquet` — **11 duplicate-isoab groups / 27 rows** (issue #290 counted 4 in the CM panel; the file also has SUN×6, YEM×3, YUG, DEU, ETH, SAU, ZAF); 9 of the 11 join the shapefile (SUN/YUG sit in the C-206 retired allowlist). | +| Narrative | The bundled metadata legitimately carries dead/live pairs sharing an ISO code (Sudan 59 pre-2011 vs 245, Serbia 230/233, Tanzania 236/242, Indonesia 208/209 …). If a frame carries BOTH members, the merge inflates to two rows per polygon and every downstream resolution site picks silently: reproduced with `[59, 245]` — the pivot drew the dead Sudan's value for SDN and the hover reported country_id 59. Row order is frame order = ascending `country_id`, so the DEAD id wins in 4 of the 9 joinable pairs (IDN, SDN, SRB, TZA — the four #290 counted) and the live one happens to win in the other 5 (DEU, ETH, SAU, YEM, ZAF); the pick is arbitrary in all nine. The shapefile side is fan-out-safe (177 unique ADM0_A3 post-C-206), so inflation can only come from the frame. The PGM crosswalk resolves the same 11 groups at build time by #242's most-recent-observation rule (C-211); render time has no month-of-last-observation to apply it, so it can only DETECT. Unlike ragged coverage (C-111, legitimate, warns once) a FORECAST frame never legitimately carries both — its entity set is fixed at the origin — so this raises (ADR-008, with the required ERROR log), naming each collided code and its ids. Per CODE rather than per (code, month), deliberately: the repo's own historical fixture shows real transitions put both ids in the same month (Sudan 59+245 at 379, Indonesia 208+209 at 269), so per-month would not rescue them, and the hover props (`drop_duplicates(ADM0_A3)`) key on code alone. Mirror image of C-206: that was the MISSING-row failure at this merge line, this is the EXTRA-row one. | +| Cross-refs | C-206 (same merge line, opposite failure — omission), C-211 (the coding transition; the PGM crosswalk's #242 collision rule), C-111 (coverage-check precedent; why this raises instead of warns), C-190 (omission-reads-as-no-risk sibling), C-212 (guard uses the two index columns only), Cluster F; r2darts2#49 / vpc ADR-064 (the producers and their upstream refusal); #290. | +| Resolved | 2026-09-17 (#290 fix PR) | +| Resolution | Guard at the CM chokepoint, immediately after `__check_missing_geometries` (rows without geometry don't count) and before `self._mapping_dataframe` is set: `groupby(ADM0_A3)[country_id].nunique() > 1` → `ValueError` naming each collided ISO with its sorted country_ids and the register ref — "no silent country picking". RED→GREEN in `tests/test_mapping_characterization.py::TestDuplicateIsoIsLoud` against the REAL bundle (no label mock — both 59 and 245 resolve to SDN there): `[59, 245]` raises naming both ids; `[245]` alone renders one SDN row. PGM raster/image sites untouched (unreachable at CM; `gid` keys are unique by construction). **First catch was our own test double:** the 191-country fixture e2e tests labelled the map through `mock_labels_for_index`, a positional double cycling 10 ISO codes — ~19 country_ids per code, a collision real data never has, which the silent pick had been absorbing. Those tests now use the REAL bundled metadata (offline since C-22; zero collisions on the fixture); the synthetic e2e tests keep the double (3-4 countries). Residual: detection only — resolution belongs to the producer (ADR-064). **Scope residual:** the CM map today renders only forecast frames (`forecast.py`); if a future feature maps a historical/actuals frame spanning a country-system change, this guard refuses it with a producer-blaming message that is the wrong diagnosis for that shape — such a map needs a month-aware rule (C-211's #242 most-recent-observation) before it can exist. Also noted: `tests/conftest.py::mock_isoab_for_index` (which `mock_labels_for_index` delegates to) cycles 10 codes positionally and now carries a docstring warning that >10 entities trip this guard. | + ### C-213: Coordinated-release coupling to views-pipeline-core — the `>=3.0.0` pin outruns PyPI (2.3.0) and the consumed import paths are un-qualified against vpc's shim removals — RESOLVED | Field | Value | @@ -1230,7 +1244,7 @@ Concerns are registered via the `register-risk` skill and curated via the `revie - **C-xx:** Concern entries (technical risks, code quality issues, architectural debt) - **D-xx:** Disagreement entries (unresolved debates between expert perspectives) -- **ID numbering:** the native sequence ran C-01–C-48; the register then jumped to **C-107** (migrated from pipeline-core C-133, 2026-06-19) and continued C-108–C-118; a later numbering re-sync jumped to **C-185+** (now through C-213). The **C-49–C-106 and C-119–C-184 ranges are intentionally unused** (no backfill). **C-193–C-204 were never standalone entries:** C-193–C-199 are unused; of the expert-code-review finding IDs, C-200 and C-202 were deduped into C-192 at intake, C-201 went moot with the reconciliation deletion (#72), and C-203/C-204 were absorbed into C-26's resolution narrative — mentions of "C-203"/"C-204" (there and in C-209) refer to those review findings, not to register entries. New entries continue from the current maximum. +- **ID numbering:** the native sequence ran C-01–C-48; the register then jumped to **C-107** (migrated from pipeline-core C-133, 2026-06-19) and continued C-108–C-118; a later numbering re-sync jumped to **C-185+** (now through C-227). The **C-49–C-106 and C-119–C-184 ranges are intentionally unused** (no backfill). **C-193–C-204 were never standalone entries:** C-193–C-199 are unused; of the expert-code-review finding IDs, C-200 and C-202 were deduped into C-192 at intake, C-201 went moot with the reconciliation deletion (#72), and C-203/C-204 were absorbed into C-26's resolution narrative — mentions of "C-203"/"C-204" (there and in C-209) refer to those review findings, not to register entries. New entries continue from the current maximum. Concerns are closed when: - The underlying issue is resolved (code change merged) diff --git a/tests/conftest.py b/tests/conftest.py index 4b074a3..f97baf1 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -117,7 +117,13 @@ def mock_name_for_df(df, entity_id="country_id", time_id="month_id"): def mock_isoab_for_index(index, level): - """Fake isoab DataFrame keyed by a (time, entity) MultiIndex.""" + """Fake isoab DataFrame keyed by a (time, entity) MultiIndex. + + Cycles REAL_ISO_CODES positionally, so it is only collision-free for + <= 10 entities. With more, two entity ids share a code and the CM map's + duplicate-ISO guard (C-227, #290) refuses to render — use the real bundled + metadata for larger frames, as tests/test_e2e_fixture.py does. + """ entity_name = index.names[1] entity_ids = sorted(set(index.get_level_values(entity_name))) code_map = { diff --git a/tests/test_e2e_fixture.py b/tests/test_e2e_fixture.py index f3efe88..3a1e4b9 100644 --- a/tests/test_e2e_fixture.py +++ b/tests/test_e2e_fixture.py @@ -23,7 +23,7 @@ except ImportError: pytest.skip("views_frames not installed", allow_module_level=True) -from tests.conftest import mock_labels_for_index, mock_name_for_index +from tests.conftest import mock_name_for_index from views_reporting.loaders import load_predictions from views_reporting.statistics import calculate_map_frame @@ -59,12 +59,16 @@ def _load_historical(model_dir): def _patch_metadata(): - """Patch the index-keyed metadata accessors used by the frame path.""" + """Patch the historical-graph name accessor only. + + The map labels (isoab/name) deliberately come from the REAL bundled + metadata (offline since C-22): these fixtures carry ~191 real countries, + and the positional `mock_labels_for_index` double cycles 10 ISO codes, so + it fabricates ~19 country_ids per code — a collision the real data never + has, which the C-227 duplicate-ISO guard now correctly refuses (#290). + The synthetic e2e tests keep the double; they use 3-4 countries. + """ return [ - patch( - "views_reporting.mapping._frame_adapter.get_labels_for_index", - side_effect=mock_labels_for_index, - ), patch( "views_reporting.visualizations.historical.get_name_for_index", side_effect=mock_name_for_index, diff --git a/tests/test_mapping_characterization.py b/tests/test_mapping_characterization.py index 4283411..8609ccf 100644 --- a/tests/test_mapping_characterization.py +++ b/tests/test_mapping_characterization.py @@ -132,3 +132,57 @@ def test_target_values_per_time_entity(self, monkeypatch): np.testing.assert_allclose( actual, np.array(self.EXPECTED_VALUES), atol=1e-4 ) + + +# ── Duplicate ISO code on the way into the CM choropleth (#290 / C-227) ────── + + +def _cm_map_frame(country_ids, month_id=528): + """An S==1 CM frame for the given country_ids at one month, no mocks: + the REAL bundled metadata resolves isoab, which is the point — dead/live + pairs (Sudan 59/245) share one code there.""" + from views_frames import PredictionFrame, SpatioTemporalIndex + + ids = np.asarray(country_ids, dtype=np.int64) + index = SpatioTemporalIndex( + time=np.full(len(ids), month_id, dtype=np.int64), + unit=ids, + level=SpatialLevel.CM, + ) + values = np.arange(1, len(ids) + 1, dtype=np.float32).reshape(-1, 1) + return PredictionFrame(values, index) + + +@pytest.mark.red_team +@pytest.mark.slow +class TestDuplicateIsoIsLoud: + """Two country_ids under one ISO code must refuse to render (#290). + + Before the guard, `pivot_table(aggfunc="first")` silently drew whichever + row came first — for Sudan (and 3 other pairs) that is the DEAD entity + (59, pre-2011) over the LIVE one (245); for 5 other pairs the live one + happened to win. Arbitrary either way: a wrong-or-lucky map with no signal + is ADR-008's forbidden shape, so this raises rather than picks. + """ + + def test_dead_and_live_sudan_together_raise_naming_both(self): + mapper = MappingModule( + frame=_cm_map_frame([59, 245]), + level=SpatialLevel.CM, + target_column="pred_ged_sb_map", + ) + with pytest.raises(ValueError, match="SDN") as excinfo: + mapper.get_subset_mapping_dataframe(entity_ids=None, time_ids=None) + message = str(excinfo.value) + assert "59" in message and "245" in message + assert "C-227" in message + + def test_live_sudan_alone_renders_one_row(self): + mapper = MappingModule( + frame=_cm_map_frame([245]), + level=SpatialLevel.CM, + target_column="pred_ged_sb_map", + ) + out = mapper.get_subset_mapping_dataframe(entity_ids=None, time_ids=None) + assert list(out["ADM0_A3"]) == ["SDN"] + assert list(out["country_id"]) == [245] diff --git a/views_reporting/mapping/mapping.py b/views_reporting/mapping/mapping.py index 3734ff9..b64fc6c 100644 --- a/views_reporting/mapping/mapping.py +++ b/views_reporting/mapping/mapping.py @@ -420,7 +420,7 @@ def __check_missing_geometries( or logs warnings about their presence. Internal Use: - Called by __init_mapping_dataframe() during data preparation. + Called by build_mapping_dataframe() during data preparation. Args: mapping_dataframe: GeoDataFrame to validate @@ -498,6 +498,41 @@ def build_mapping_dataframe( merged_gdf = self.__check_missing_geometries( gpd.GeoDataFrame(flat, geometry="geometry", crs=self._world.crs) ) + # Two country_ids under one ISO code (the bundled metadata carries + # dead/live pairs — Sudan 59/245, Serbia 230/233, …) would reach the + # choropleth as two rows per polygon, and `pivot_table(aggfunc= + # "first")` plus the hover-props `drop_duplicates` would silently + # pick one — the DEAD one for 4 of the 9 joinable pairs. A forecast + # frame's entity set is fixed at its origin, so it never legitimately + # carries both; refuse rather than pick (register C-227). Per CODE, + # not per (code, month): real transition data puts both ids in the + # same month anyway, and the hover props key on code alone. + ids_per_code = merged_gdf.groupby(self._location_col)[ + self._entity_id + ].nunique() + collided = ids_per_code[ids_per_code > 1].index + if len(collided): + details = { + code: sorted( + merged_gdf.loc[ + merged_gdf[self._location_col] == code, self._entity_id + ] + .unique() + .tolist() + ) + for code in collided + } + message = ( + f"CM map has more than one {self._entity_id} per ISO code — " + f"{details} (register C-227): the choropleth would silently " + f"pick one of them per polygon. A forecast frame carrying a " + f"dissolved entity beside its successor must be resolved at " + f"the producer (pipeline-core ADR-064) — no silent country " + f"picking. (A historical/actuals frame spanning a country-system " + f"change is a different shape and would need a month-aware rule.)" + ) + logger.error(message) + raise ValueError(message) else: flat = flat.merge( self._world,