From 083f8a982e08ea8bc5f63e59d44bf0dc38598eda Mon Sep 17 00:00:00 2001 From: Andras Schmelczer Date: Sun, 14 Jun 2026 14:52:44 +0100 Subject: [PATCH] Small fixes --- finder/README.md | 224 ++++++++++++++++++ finder/postcode_cache.py | 67 ++++++ finder/shutdown.py | 69 ++++++ finder/test_http_client.py | 90 +++++++ finder/test_main.py | 57 +++++ finder/test_postcode_cache.py | 58 +++++ finder/test_rightmove_concurrency.py | 165 +++++++++++++ finder/test_scraper_concurrency.py | 150 ++++++++++++ finder/test_shutdown.py | 130 ++++++++++ frontend/src/lib/crime-filter.ts | 5 +- frontend/src/lib/travel-params.ts | 8 +- frontend/src/lib/url-state.test.ts | 2 + frontend/src/lib/url-state.ts | 7 +- pipeline/transform/join_epc_pp.py | 20 +- pipeline/transform/merge.py | 75 +++++- pipeline/transform/test_join_epc_pp.py | 4 +- pipeline/transform/test_merge.py | 56 +++++ pipeline/transform/test_transform_poi.py | 84 +++++++ pipeline/transform/transform_poi.py | 70 +++++- server-rs/src/data.rs | 23 ++ server-rs/src/data/places.rs | 22 +- server-rs/src/data/property/address_search.rs | 20 +- server-rs/src/features.rs | 149 +++++++++--- server-rs/src/routes/export.rs | 29 ++- 24 files changed, 1505 insertions(+), 79 deletions(-) create mode 100644 finder/README.md create mode 100644 finder/postcode_cache.py create mode 100644 finder/shutdown.py create mode 100644 finder/test_http_client.py create mode 100644 finder/test_main.py create mode 100644 finder/test_postcode_cache.py create mode 100644 finder/test_rightmove_concurrency.py create mode 100644 finder/test_scraper_concurrency.py create mode 100644 finder/test_shutdown.py diff --git a/finder/README.md b/finder/README.md new file mode 100644 index 0000000..410d6cd --- /dev/null +++ b/finder/README.md @@ -0,0 +1,224 @@ +# Finder — property listing scraper + +Scrapes Greater-London sale listings from **Rightmove**, **OnTheMarket**, and +**Zoopla**, recovers each property's true full postcode, and writes a single +parquet (`data/online_listings_buy.parquet`) that the rest of the app consumes +(after a separate enrich step — see [Output](#output)). + +`main.py` is the only entry point; everything else is library code. + +--- + +## How it works (and why it's careful about postcodes) + +Every portal's **search** API exposes only an *outcode*-level address (e.g. +`"…, London, SW9"`) plus map coordinates — never the full unit postcode. The +full postcode lives on each listing's **detail page**, so the scraper fetches +detail pages to recover it, and only trusts a detail postcode when its outcode +agrees with the coordinate-nearest postcode (so a stale/wrong value can never +silently relocate a listing). When no trustworthy detail postcode is found, it +falls back to the coordinate-nearest postcode. See the module docstrings in +`rightmove.py`, `onthemarket.py`, and `zoopla.py` for the per-portal data model. + +Detail fetching is the dominant cost, so it is: + +- **cached across runs** — `data/detail_cache/{source}.json` maps listing id → + recovered postcode; a re-run only fetches *newly-appeared* listings; +- **fetched concurrently** for the HTTP portals (Rightmove, OnTheMarket), bounded + by a shared global rate limiter so the VPN egress stays polite; +- **gated and capped** per outcode. + +See [Performance](#performance--caching). + +--- + +## Prerequisites + +The scraper egresses through a VPN. There are two supported ways to provide it: + +- **Shared netns (compose, recommended):** an **external `media_gluetun`** + container (qmcgaw/gluetun) must already be running on the host. It is managed + by a *different* compose; `finder/docker-compose.yml` attaches to its network + namespace via `network_mode: "container:media_gluetun"`. +- **HTTP proxy (standalone):** reach a Gluetun HTTP proxy at `GLUETUN_PROXY` + (default `http://gluetun:8888`), or set `GLUETUN_PROXY=""` for a direct, + un-tunnelled connection. + +Also required: the ARCGIS postcode parquet at `../property-data/arcgis_data.parquet` +(override with `ARCGIS_PATH`). + +--- + +## Running + +### Docker Compose (recommended — the only way that does Zoopla) + +`finder/docker-compose.yml` brings up the scraper plus **FlareSolverr** (which +solves Zoopla's Cloudflare challenge), both sharing `media_gluetun`'s netns. This +is the intended production-like path. + +```bash +cd finder + +# Start the sidecars (finder stays up via `sleep infinity`). +docker compose up -d --build flaresolverr finder + +# Run scrapes inside the container (uv run uses the image's /opt/venv): +docker compose exec finder uv run python main.py --source all +docker compose exec finder uv run python main.py --source zoopla --outcodes SW9 --test + +docker compose down +``` + +> If a leftover `finder_flaresolverr` container exists from earlier testing, +> remove it first: `docker rm -f finder_flaresolverr`. + +In this setup `GLUETUN_PROXY=""` (the shared netns already tunnels everything), +`ZOOPLA_FETCHER=flaresolverr`, and `DATA_DIR` / `ARCGIS_PATH` are preset by the +compose file. + +### Standalone (quick Rightmove / OnTheMarket dev runs) + +Zoopla needs FlareSolverr, so standalone is for the HTTP portals. You just need +a venv and VPN reachability. + +```bash +cd finder + +# One-time: create the venv from the lockfile. +uv sync --frozen # creates .venv with httpx, polars, fake-useragent, … + +# Small, safe run into a temp dir (does NOT touch real data/): +.venv/bin/python main.py --source rightmove --outcodes SW9 \ + --max-properties-per-source 20 --output-dir /tmp/finder-smoke + +# Go direct instead of via the gluetun proxy hostname: +GLUETUN_PROXY="" .venv/bin/python main.py --source onthemarket --outcodes SW9 \ + --output-dir /tmp/finder-smoke +``` + +(`uv run python main.py …` works too and resolves the env automatically.) + +--- + +## CLI reference (`main.py`) + +| Flag | Default | Meaning | +|------|---------|---------| +| `--source rightmove,onthemarket` | `all` | Comma-separated portal(s): any of `rightmove`, `onthemarket`, `zoopla`, or `all`. | +| `--outcodes SW9,E14,BR1` | — | Specific outcodes (must be Greater-London-ish). Otherwise the full London set is loaded from ARCGIS. | +| `--limit-outcodes N` | — | Cap the number of outcodes (quick smoke). | +| `--max-properties-per-source N` | — | Stop each source after N transformed listings. | +| `--output-dir DIR` | `data/` | Where the parquet (and `detail_cache/`) are written. | +| `--test` | off | ~10 likely-London outcodes, ≤100 listings/source, writes to `data/test/`. | + +> **Always pass `--output-dir /tmp/...` for testing** — the default `data/` holds +> the real listings the app consumes. + +### Stopping a run + +`Ctrl+C` (SIGINT) — or `docker stop` (SIGTERM) — triggers a **graceful +shutdown**: every source stops at its next outcode boundary, in-flight delays +and retry backoffs wake immediately, and the run still persists the detail +caches and writes the listings collected so far before exiting (code `130`). +Press `Ctrl+C` a second time to force-quit. See `shutdown.py`. + +--- + +## Sources & what each needs + +| Source | Transport | Needs FlareSolverr? | Concurrency | Notes | +|--------|-----------|---------------------|-------------|-------| +| Rightmove | plain httpx | no | concurrent detail fetches | main path | +| OnTheMarket | plain httpx | no | concurrent detail fetches | `__NEXT_DATA__` JSON | +| Zoopla | browser / FlareSolverr | **yes** (`ZOOPLA_FETCHER=flaresolverr`, default) | serial (browser-bound) | Cloudflare-protected; skipped gracefully if FlareSolverr is unavailable | + +Rightmove and OnTheMarket run **concurrently in worker threads**; Zoopla runs on +the **main thread** (its per-outcode wall-clock guard uses `SIGALRM`, which only +fires on the main thread). One source failing never kills the others. + +--- + +## Output + +Each run writes `/online_listings_buy.parquet`. + +A **separate enrich step** (outside `finder/`) turns that into +`online_listings_buy_enriched.parquet`, which is what the Rust backend actually +loads (`--actual-listings-path …/online_listings_buy_enriched.parquet` in the +top-level `docker-compose.yml`). That enrich/scheduling pipeline is **not** +documented here — only the raw scrape is. + +The top-level `docker-compose.yml` (Rust `server`, `frontend`, `pocketbase`, +`screenshot`) is the **web app**; it is downstream of the scrape and is **not** +required to run the scraper. + +--- + +## Performance & caching + +| Mechanism | Where | Effect | +|-----------|-------|--------| +| **Persistent detail cache** | `data/detail_cache/{source}.json` | A listing's postcode never changes, so a re-run reuses cached results and only fetches new listings. Delete this folder to force a full re-fetch. | +| **Concurrent detail fetches** | Rightmove, OnTheMarket | Detail pages fetched in parallel instead of one-at-a-time. | +| **Global rate limiter** | `http_client.RATE_LIMITER` | Caps the *combined* request rate across all threads/portals so concurrency stays polite. | + +> **Note on the "accurate-pin skip" flag (`RIGHTMOVE_SKIP_DETAILS_FOR_ACCURATE_PINS`):** +> it is currently a **no-op**. The idea was to skip the detail fetch for listings +> the search already pins precisely (`location.pinType == "ACCURATE_POINT"`), but +> Rightmove's live search API does not include `pinType` in the payload (only +> `latitude`/`longitude`), so nothing is ever skipped. It degrades safely (no +> accuracy loss) but provides no speed-up today. + +--- + +## Configuration + +Environment variables (override the defaults in `constants.py`): + +| Variable | Default | Purpose | +|----------|---------|---------| +| `DATA_DIR` | `finder/data` | Output root. | +| `ARCGIS_PATH` | `../property-data/arcgis_data.parquet` | Postcode reference data. | +| `GLUETUN_PROXY` | `http://gluetun:8888` | HTTP proxy for egress; `""` = direct. | +| `GLUETUN_CONTROL_URL` | `http://gluetun:8000` | Gluetun control API. | +| `FLARESOLVERR_URL` | `http://gluetun:8191/v1` | FlareSolverr endpoint (Zoopla). | +| `ZOOPLA_FETCHER` | `flaresolverr` | `flaresolverr` or `camoufox`. | +| `ZOOPLA_OUTCODE_TIMEOUT_SECONDS` | `300` | Per-outcode wall-clock budget for Zoopla. | +| `DETAIL_FETCH_CONCURRENCY` | `8` | Parallel detail fetches (Rightmove/OTM). | +| `REQUESTS_PER_SECOND` | `10` | Global request-rate cap. Lower it if you see `429`/`403`. | +| `RIGHTMOVE_SKIP_DETAILS_FOR_ACCURATE_PINS` | `1` | Inert today (see note above). | + +Non-env code constants worth knowing (`constants.py` / `onthemarket.py`): +`RIGHTMOVE_FETCH_DETAILS`, `RIGHTMOVE_MAX_DETAILS_PER_OUTCODE` (4000), +`OTM_FETCH_DETAILS`, `OTM_MAX_DETAILS_PER_OUTCODE` (400), +`ZOOPLA_FETCH_DETAILS`, `ZOOPLA_MAX_DETAILS_PER_OUTCODE` (4000). + +--- + +## Tests + +`pytest` is not a declared dependency; run it ephemerally with uv (no project +change needed): + +```bash +cd finder +uv run --with pytest pytest -q +``` + +--- + +## Repo layout + +| File | Responsibility | +|------|----------------| +| `main.py` | CLI entry point: parse args, build the postcode index, call `run_scrape`. | +| `scraper.py` | Orchestration: per-source runners, provider parallelism, cache load/save, merge + write. | +| `rightmove.py` / `onthemarket.py` / `zoopla.py` | Per-portal search + detail scraping and parsing. | +| `transform.py` | Raw listing → output schema; postcode trust rules. | +| `http_client.py` | Shared httpx client, retry/backoff, and the global `RATE_LIMITER`. | +| `postcode_cache.py` | Persistent (cross-run) detail-cache load/save. | +| `spatial.py` | Grid spatial index for coordinate → nearest postcode. | +| `storage.py` | Parquet writer (server-ready column names). | +| `constants.py` | Tunables and endpoints. | +| `test_*.py` | Unit tests. | diff --git a/finder/postcode_cache.py b/finder/postcode_cache.py new file mode 100644 index 0000000..272333e --- /dev/null +++ b/finder/postcode_cache.py @@ -0,0 +1,67 @@ +"""Persistent detail-fetch caches that survive across scrape runs. + +Each portal recovers a listing's true postcode (Rightmove/OnTheMarket) or full +geo dict (Zoopla) from its detail page. That value never changes for a given +listing id, yet the in-memory caches are discarded at the end of every run — so +each run re-fetches every listing's detail page from scratch. Persisting the +cache to disk means a steady-state run only fetches NEWLY-appeared listings, +typically a small fraction of the market, which is the single biggest saving +for a recurring scrape (and it *reduces* request volume rather than adding to +it). + +The stored values are always JSON-serialisable: Rightmove/OnTheMarket cache +``str | None`` postcodes; Zoopla caches a flat dict of primitives (or ``None``). +A cached ``None`` (a listing whose detail page yielded nothing usable) is +persisted too, so a postcode-less listing is not re-fetched every run. +""" + +import json +import logging +import os +import tempfile +from pathlib import Path + +log = logging.getLogger("rightmove") + + +def load_cache(path: str | Path) -> dict: + """Load a persisted detail cache. Returns ``{}`` when absent or unreadable. + + A corrupt or non-object file is treated as empty rather than fatal — a bad + cache must never block a scrape; the worst case is re-fetching details.""" + p = Path(path) + if not p.exists(): + return {} + try: + with open(p, encoding="utf-8") as fh: + data = json.load(fh) + except (OSError, ValueError) as exc: + log.warning("Could not read detail cache %s: %s", p, exc) + return {} + if not isinstance(data, dict): + log.warning("Detail cache %s is not a JSON object; ignoring", p) + return {} + log.info("Loaded %d cached detail entries from %s", len(data), p) + return data + + +def save_cache(path: str | Path, data: dict) -> None: + """Atomically write a detail cache to disk (temp file + ``os.replace``). + + Creates the parent directory if needed. Write failures are logged and + swallowed: persisting the cache is an optimisation, never a hard + requirement, so a read-only disk must not fail an otherwise-good scrape.""" + p = Path(path) + try: + p.parent.mkdir(parents=True, exist_ok=True) + fd, tmp = tempfile.mkstemp(dir=str(p.parent), suffix=".tmp") + try: + with os.fdopen(fd, "w", encoding="utf-8") as fh: + json.dump(data, fh) + os.replace(tmp, p) + finally: + if os.path.exists(tmp): + os.unlink(tmp) + log.info("Saved %d cached detail entries to %s", len(data), p) + except OSError as exc: + log.warning("Could not write detail cache %s: %s", p, exc) diff --git a/finder/shutdown.py b/finder/shutdown.py new file mode 100644 index 0000000..beed235 --- /dev/null +++ b/finder/shutdown.py @@ -0,0 +1,69 @@ +"""Process-wide cooperative shutdown for the scrapers. + +A single :class:`threading.Event` is set the first time the process receives +SIGINT (Ctrl+C) or SIGTERM. Every scrape loop polls :func:`stop_requested` at +its outcode/page boundaries and every blocking delay goes through :func:`sleep`, +which wakes the instant a stop is requested. So Ctrl+C makes each source stop +*starting* new work and unwind through its normal ``finally`` blocks — detail +caches are persisted and whatever has been collected so far is still written — +instead of hanging until the worker threads happen to finish (the orchestrator's +``ThreadPoolExecutor`` used to block the exit waiting on them) or losing the run +outright. + +A second Ctrl+C escalates to a hard :class:`KeyboardInterrupt` for an impatient +operator. SIGALRM is deliberately left untouched: Zoopla's per-outcode +wall-clock guard owns it.""" + +import logging +import signal +import threading + +log = logging.getLogger("finder") + +_STOP = threading.Event() + + +def stop_requested() -> bool: + """True once a shutdown signal has been received.""" + return _STOP.is_set() + + +def request_stop() -> None: + """Ask every scrape loop to stop starting new work.""" + _STOP.set() + + +def reset() -> None: + """Clear the flag — for tests and repeated in-process runs.""" + _STOP.clear() + + +def sleep(seconds: float) -> None: + """Sleep that wakes immediately once a stop is requested. + + Drop-in for ``time.sleep`` in the scrape loops, inter-outcode pauses and + retry backoffs so a pending Ctrl+C is never stuck behind a multi-second + delay.""" + if seconds <= 0: + return + _STOP.wait(seconds) + + +def install_signal_handlers() -> None: + """Route SIGINT/SIGTERM to :func:`request_stop` (second signal = hard exit). + + Signal handlers run in the main thread, so this must be called from there.""" + + def _handle(signum, _frame): + if _STOP.is_set(): + # Operator pressed Ctrl+C twice: abandon the graceful unwind. + raise KeyboardInterrupt + log.warning( + "Signal %s received; finishing in-flight work and saving partial " + "results (press Ctrl+C again to force-quit)...", + signal.Signals(signum).name, + ) + _STOP.set() + + for sig in (signal.SIGINT, signal.SIGTERM): + signal.signal(sig, _handle) diff --git a/finder/test_http_client.py b/finder/test_http_client.py new file mode 100644 index 0000000..11e31ad --- /dev/null +++ b/finder/test_http_client.py @@ -0,0 +1,90 @@ +"""Tests for the shared request rate limiter (http_client.RateLimiter). + +Uses a fake clock so the spacing logic is asserted deterministically rather than +by sleeping for real.""" + +import threading + +import pytest + +import http_client +import shutdown +from http_client import RateLimiter + + +class _FakeClock: + """A monotonic clock that only advances when sleep() is called.""" + + def __init__(self, start=1000.0): + self.t = start + self.sleeps = [] + + def monotonic(self): + return self.t + + def sleep(self, seconds): + self.sleeps.append(seconds) + self.t += seconds + + +def _install(monkeypatch): + clock = _FakeClock() + monkeypatch.setattr(http_client.time, "monotonic", clock.monotonic) + # The limiter waits via the interruptible shutdown.sleep so a Ctrl+C wakes a + # worker parked on a reserved rate-limit slot instead of stalling the exit. + monkeypatch.setattr(shutdown, "sleep", clock.sleep) + return clock + + +def test_first_acquire_does_not_sleep(monkeypatch): + clock = _install(monkeypatch) + RateLimiter(10).acquire() + assert clock.sleeps == [] + + +def test_subsequent_acquires_are_spaced_by_the_interval(monkeypatch): + clock = _install(monkeypatch) + rl = RateLimiter(10) # 0.1s minimum interval + rl.acquire() # no wait + rl.acquire() # waits ~0.1 + rl.acquire() # waits ~0.1 + assert clock.sleeps == pytest.approx([0.1, 0.1]) + + +def test_acquire_after_idle_gap_does_not_sleep(monkeypatch): + clock = _install(monkeypatch) + rl = RateLimiter(10) + rl.acquire() + clock.t += 5.0 # plenty of idle time passes + rl.acquire() + assert clock.sleeps == [] # the slot was already free + + +def test_zero_rate_disables_limiting(monkeypatch): + def _boom(_seconds): # pragma: no cover - must never be called + raise AssertionError("disabled limiter must not sleep") + + monkeypatch.setattr(shutdown, "sleep", _boom) + rl = RateLimiter(0) + for _ in range(100): + rl.acquire() + + +def test_concurrent_acquires_are_all_spaced(monkeypatch): + # Real clock, tiny rate: N threads hitting acquire() at once must be + # serialised so the total wall time is at least (N-1) * interval. + rl = RateLimiter(200) # 5ms interval — fast but measurable + barrier = threading.Barrier(8) + + def worker(): + barrier.wait() + rl.acquire() + + start = http_client.time.monotonic() + threads = [threading.Thread(target=worker) for _ in range(8)] + for t in threads: + t.start() + for t in threads: + t.join() + elapsed = http_client.time.monotonic() - start + assert elapsed >= 7 * 0.005 * 0.5 # allow scheduling slack, but not ~0 diff --git a/finder/test_main.py b/finder/test_main.py new file mode 100644 index 0000000..0e14393 --- /dev/null +++ b/finder/test_main.py @@ -0,0 +1,57 @@ +"""Tests for the CLI argument parsing in main.py.""" + +import pytest + +import main + + +# --------------------------------------------------------------------------- +# selected_sources — comma-separated --source values +# --------------------------------------------------------------------------- + + +def test_all_expands_to_every_source(): + assert main.selected_sources("all") == ["rightmove", "onthemarket", "zoopla"] + + +def test_single_source(): + assert main.selected_sources("rightmove") == ["rightmove"] + + +def test_comma_separated_sources(): + assert main.selected_sources("rightmove,onthemarket") == [ + "rightmove", + "onthemarket", + ] + + +def test_whitespace_and_case_are_tolerated(): + assert main.selected_sources(" Rightmove , ZOOPLA ") == ["rightmove", "zoopla"] + + +def test_order_is_canonical_and_deduplicated(): + # Caller order ignored, duplicates collapsed: keeps SOURCES order so the + # downstream merge/dedup stays deterministic. + assert main.selected_sources("zoopla,rightmove,rightmove") == [ + "rightmove", + "zoopla", + ] + + +def test_all_anywhere_in_the_list_wins(): + assert main.selected_sources("rightmove,all") == [ + "rightmove", + "onthemarket", + "zoopla", + ] + + +def test_empty_value_is_rejected(): + with pytest.raises(SystemExit): + main.selected_sources(" , ") + + +def test_unknown_source_is_rejected(): + with pytest.raises(SystemExit) as excinfo: + main.selected_sources("rightmove,foo") + assert "foo" in str(excinfo.value) diff --git a/finder/test_postcode_cache.py b/finder/test_postcode_cache.py new file mode 100644 index 0000000..11c91d4 --- /dev/null +++ b/finder/test_postcode_cache.py @@ -0,0 +1,58 @@ +"""Tests for the persistent detail-fetch cache (postcode_cache.py).""" + +from postcode_cache import load_cache, save_cache + + +def test_save_then_load_round_trips_str_and_none_values(tmp_path): + path = tmp_path / "rightmove.json" + data = {"123": "SW9 0HD", "456": None, "789": "E20 1FH"} + save_cache(path, data) + assert load_cache(path) == data + + +def test_save_then_load_round_trips_nested_dicts(tmp_path): + # Zoopla caches a flat dict of primitives (or None) per listing id. + path = tmp_path / "zoopla.json" + data = { + "59888978": { + "lat": 51.4, + "lng": -0.11, + "postcode": "BR1 2AB", + "uprn": "100023", + "number_or_name": "12", + "full_address": "12 High St", + }, + "broken": None, + } + save_cache(path, data) + assert load_cache(path) == data + + +def test_load_missing_file_returns_empty(tmp_path): + assert load_cache(tmp_path / "does-not-exist.json") == {} + + +def test_load_corrupt_file_returns_empty(tmp_path): + path = tmp_path / "corrupt.json" + path.write_text("{not valid json", encoding="utf-8") + assert load_cache(path) == {} + + +def test_load_non_object_file_returns_empty(tmp_path): + path = tmp_path / "list.json" + path.write_text("[1, 2, 3]", encoding="utf-8") + assert load_cache(path) == {} + + +def test_save_creates_parent_directory(tmp_path): + path = tmp_path / "nested" / "dir" / "cache.json" + save_cache(path, {"a": "B1 1AA"}) + assert path.exists() + assert load_cache(path) == {"a": "B1 1AA"} + + +def test_save_overwrites_existing(tmp_path): + path = tmp_path / "c.json" + save_cache(path, {"a": "X1 1AA"}) + save_cache(path, {"b": "Y2 2BB"}) + assert load_cache(path) == {"b": "Y2 2BB"} diff --git a/finder/test_rightmove_concurrency.py b/finder/test_rightmove_concurrency.py new file mode 100644 index 0000000..cadc3b1 --- /dev/null +++ b/finder/test_rightmove_concurrency.py @@ -0,0 +1,165 @@ +"""Tests for Rightmove's accurate-pin skip and concurrent detail fetching.""" + +import threading + +import rightmove +from rightmove import _needs_detail_fetch, _paginate, _prime_detail_postcodes + + +def _prop(pid, pin=None): + loc = {"latitude": 51.5, "longitude": -0.1} + if pin is not None: + loc["pinType"] = pin + return {"id": pid, "location": loc} + + +# --------------------------------------------------------------------------- +# _needs_detail_fetch — accurate-pin skip +# --------------------------------------------------------------------------- + + +def test_needs_detail_fetch_skips_only_accurate_pins(monkeypatch): + monkeypatch.setattr(rightmove, "RIGHTMOVE_SKIP_DETAILS_FOR_ACCURATE_PINS", True) + assert _needs_detail_fetch(_prop("1", "ACCURATE_POINT")) is False + assert _needs_detail_fetch(_prop("2", "APPROXIMATE_POINT")) is True + assert _needs_detail_fetch(_prop("3")) is True # no pinType -> fetch + assert _needs_detail_fetch({"id": "4"}) is True # no location -> fetch + + +def test_needs_detail_fetch_disabled_always_fetches(monkeypatch): + monkeypatch.setattr(rightmove, "RIGHTMOVE_SKIP_DETAILS_FOR_ACCURATE_PINS", False) + assert _needs_detail_fetch(_prop("1", "ACCURATE_POINT")) is True + + +# --------------------------------------------------------------------------- +# _prime_detail_postcodes — worklist selection + concurrent fetch +# --------------------------------------------------------------------------- + + +def _record_fetcher(monkeypatch): + fetched = [] + lock = threading.Lock() + + def fake_fetch(_client, pid): + with lock: + fetched.append(pid) + rightmove._detail_postcode_cache[pid] = f"PC-{pid}" + return f"PC-{pid}" + + monkeypatch.setattr(rightmove, "_fetch_detail_postcode", fake_fetch) + return fetched + + +def test_prime_skips_cached_accurate_and_dedups(monkeypatch): + rightmove._detail_postcode_cache.clear() + rightmove._detail_postcode_cache["cached"] = "SW9 9ZZ" + monkeypatch.setattr(rightmove, "RIGHTMOVE_SKIP_DETAILS_FOR_ACCURATE_PINS", True) + fetched = _record_fetcher(monkeypatch) + + props = [ + _prop("cached", "APPROXIMATE_POINT"), # already cached -> skip + _prop("acc", "ACCURATE_POINT"), # accurate pin -> skip + _prop("a", "APPROXIMATE_POINT"), # fetch + _prop("a", "APPROXIMATE_POINT"), # duplicate id -> skip + _prop("b"), # no pinType -> fetch + {"id": ""}, # no id -> skip + ] + _prime_detail_postcodes(object(), props, True, 10) + + assert sorted(fetched) == ["a", "b"] + assert rightmove._detail_postcode_cache["a"] == "PC-a" + assert rightmove._detail_postcode_cache["b"] == "PC-b" + rightmove._detail_postcode_cache.clear() + + +def test_prime_respects_detail_cap(monkeypatch): + rightmove._detail_postcode_cache.clear() + monkeypatch.setattr(rightmove, "RIGHTMOVE_SKIP_DETAILS_FOR_ACCURATE_PINS", True) + fetched = _record_fetcher(monkeypatch) + + props = [_prop(str(i), "APPROXIMATE_POINT") for i in range(10)] + _prime_detail_postcodes(object(), props, True, 3) + + assert sorted(fetched) == ["0", "1", "2"] + rightmove._detail_postcode_cache.clear() + + +def test_prime_is_a_noop_when_disabled_or_cap_zero(monkeypatch): + rightmove._detail_postcode_cache.clear() + + def _boom(*_a): # pragma: no cover - must never be called + raise AssertionError("must not fetch when disabled") + + monkeypatch.setattr(rightmove, "_fetch_detail_postcode", _boom) + _prime_detail_postcodes(object(), [_prop("a")], False, 10) # fetch_details off + _prime_detail_postcodes(object(), [_prop("a")], True, 0) # cap 0 + assert rightmove._detail_postcode_cache == {} + + +# --------------------------------------------------------------------------- +# _paginate — end-to-end (network stubbed): accurate pins fall back to +# coordinates, approximate pins use the detail postcode. +# --------------------------------------------------------------------------- + + +class _StubIndex: + def __init__(self, postcode): + self._postcode = postcode + + def nearest(self, _lat, _lng): + return self._postcode + + +def _full_prop(pid, pin): + return { + "id": pid, + "location": {"latitude": 51.477, "longitude": -0.116, "pinType": pin}, + "price": {"amount": 500000, "displayPrices": []}, + "displayAddress": "Caldwell Street, Stockwell, London, SW9", + "propertySubType": "Flat", + "bedrooms": 2, + "bathrooms": 1, + "propertyUrl": f"/properties/{pid}", + } + + +def test_paginate_uses_detail_for_approximate_and_coords_for_accurate(monkeypatch): + rightmove._detail_postcode_cache.clear() + monkeypatch.setattr(rightmove, "RIGHTMOVE_SKIP_DETAILS_FOR_ACCURATE_PINS", True) + + raw = [ + _full_prop("approx", "APPROXIMATE_POINT"), + _full_prop("acc", "ACCURATE_POINT"), + ] + monkeypatch.setattr( + rightmove, "_collect_search_props", lambda *a, **k: (raw, len(raw)) + ) + + fetched = [] + + def fake_fetch(_client, pid): + fetched.append(pid) + rightmove._detail_postcode_cache[pid] = "SW9 0HD" + return "SW9 0HD" + + monkeypatch.setattr(rightmove, "_fetch_detail_postcode", fake_fetch) + + props, count = _paginate( + object(), + "1", + "SW9", + {"channel": "BUY", "transactionType": "BUY", "sortType": "2"}, + _StubIndex("SW9 1AA"), + fetch_details=True, + detail_cap=10, + ) + + assert count == 2 + assert fetched == ["approx"] # accurate pin was never fetched + by_id = {p["id"]: p for p in props} + assert by_id["approx"]["Postcode"] == "SW9 0HD" + assert by_id["approx"]["Postcode source"] == "detail_address" + # Accurate pin keeps the coordinate-nearest postcode. + assert by_id["acc"]["Postcode"] == "SW9 1AA" + assert by_id["acc"]["Postcode source"] != "detail_address" + rightmove._detail_postcode_cache.clear() diff --git a/finder/test_scraper_concurrency.py b/finder/test_scraper_concurrency.py new file mode 100644 index 0000000..6f9bbe3 --- /dev/null +++ b/finder/test_scraper_concurrency.py @@ -0,0 +1,150 @@ +"""Tests for the orchestrator's provider parallelism and cache persistence.""" + +import threading + +import onthemarket +import rightmove +import scraper +import zoopla + + +# --------------------------------------------------------------------------- +# _run_sources — Zoopla inline, others in threads, failures isolated +# --------------------------------------------------------------------------- + + +def test_run_sources_runs_every_source_and_isolates_failures(): + order = [] + lock = threading.Lock() + + def rm(): + with lock: + order.append("rm") + + def otm(): + with lock: + order.append("otm") + raise RuntimeError("otm boom") + + def zoo(): + with lock: + order.append("zoo") + + errors: list[str] = [] + scraper._run_sources(zoo, [("rightmove", rm), ("onthemarket", otm)], errors) + + assert set(order) == {"rm", "otm", "zoo"} + # The failing source is recorded but did not stop the others. + assert any("onthemarket" in e and "boom" in e for e in errors) + assert not any("rightmove" in e for e in errors) + + +def test_run_sources_records_zoopla_failure(): + errors: list[str] = [] + + def zoo(): + raise ValueError("zoo down") + + scraper._run_sources(zoo, [], errors) + assert any("zoopla" in e and "zoo down" in e for e in errors) + + +def test_run_sources_handles_no_background_runners(): + ran = [] + errors: list[str] = [] + scraper._run_sources(lambda: ran.append("z"), [], errors) + assert ran == ["z"] + assert errors == [] + + +def test_run_sources_handles_zoopla_absent(): + ran = [] + errors: list[str] = [] + scraper._run_sources(None, [("rightmove", lambda: ran.append("rm"))], errors) + assert ran == ["rm"] + assert errors == [] + + +# --------------------------------------------------------------------------- +# Persistent cache wiring +# --------------------------------------------------------------------------- + + +def test_seed_and_save_detail_caches_round_trip(tmp_path): + rightmove._detail_postcode_cache.clear() + rightmove._detail_postcode_cache.update({"111": "SW9 0HD", "222": None}) + + scraper._save_detail_caches(["rightmove"], tmp_path) + assert (tmp_path / "rightmove.json").exists() + + rightmove._detail_postcode_cache.clear() + scraper._seed_detail_caches(["rightmove"], tmp_path) + assert rightmove._detail_postcode_cache == {"111": "SW9 0HD", "222": None} + rightmove._detail_postcode_cache.clear() + + +def test_seed_detail_caches_tolerates_missing_files(tmp_path): + rightmove._detail_postcode_cache.clear() + # No file written yet — seeding must not raise and must leave cache empty. + scraper._seed_detail_caches(["rightmove"], tmp_path) + assert rightmove._detail_postcode_cache == {} + + +# --------------------------------------------------------------------------- +# run_scrape — full orchestration wiring (sources stubbed, no network) +# --------------------------------------------------------------------------- + + +def _fake_prop(pid, postcode): + # Minimal shape: only the fields _merge_properties / _dedup_key read. + return {"id": pid, "Postcode": postcode, "price": 100000, "Bedrooms": 2} + + +def test_run_scrape_runs_all_sources_merges_and_persists_caches(tmp_path, monkeypatch): + for module in (rightmove, onthemarket, zoopla): + module.detail_cache_snapshot() # ensure hooks exist + rightmove._detail_postcode_cache.clear() + onthemarket._detail_postcode_cache.clear() + zoopla._detail_cache.clear() + + def fake_rm(outcodes, pc_index, results, errors, cap): + results["rightmove"].append(_fake_prop("rm_1", "SW9 0HD")) + + def fake_otm(outcodes, pc_index, results, errors, cap): + results["onthemarket"].append(_fake_prop("otm_1", "E1 6AN")) + + def fake_zoo(outcodes, pc_index, pc_coords, results, errors, cap): + results["zoopla"].append(_fake_prop("z_1", "BR1 2AB")) + + monkeypatch.setattr(scraper, "_scrape_rightmove", fake_rm) + monkeypatch.setattr(scraper, "_scrape_onthemarket", fake_otm) + monkeypatch.setattr(scraper, "_scrape_zoopla", fake_zoo) + monkeypatch.setattr(scraper, "build_postcode_coords", lambda: {}) + + written = {} + monkeypatch.setattr( + scraper, + "write_parquet", + lambda props, path: written.update(count=len(props), path=path), + ) + + result = scraper.run_scrape( + ["SW9", "E1", "BR1"], + pc_index=None, + pc_coords=None, + sources=["rightmove", "onthemarket", "zoopla"], + output_dir=tmp_path, + max_properties_per_source=None, + ) + + # All three sources ran and their listings were merged + written. + assert written["count"] == 3 + assert result["counts"]["total"] == 3 + assert result["errors"] == [] + # Persistent cache files were created for every selected source. + for source in ("rightmove", "onthemarket", "zoopla"): + assert (tmp_path / "detail_cache" / f"{source}.json").exists() + + rightmove._detail_postcode_cache.clear() + onthemarket._detail_postcode_cache.clear() + zoopla._detail_cache.clear() diff --git a/finder/test_shutdown.py b/finder/test_shutdown.py new file mode 100644 index 0000000..b30cbdc --- /dev/null +++ b/finder/test_shutdown.py @@ -0,0 +1,130 @@ +"""Tests for cooperative Ctrl+C shutdown across the scrapers.""" + +import signal +import threading +import time + +import pytest + +import http_client +import scraper +import shutdown + + +@pytest.fixture(autouse=True) +def _clean_stop_flag(): + """Every test starts (and leaves) the global stop flag cleared.""" + shutdown.reset() + yield + shutdown.reset() + + +# --------------------------------------------------------------------------- +# The flag primitive +# --------------------------------------------------------------------------- + + +def test_request_and_reset_toggle_the_flag(): + assert shutdown.stop_requested() is False + shutdown.request_stop() + assert shutdown.stop_requested() is True + shutdown.reset() + assert shutdown.stop_requested() is False + + +def test_sleep_returns_immediately_once_stop_requested(): + shutdown.request_stop() + started = time.monotonic() + shutdown.sleep(30) # would hang for 30s without the early wake + assert time.monotonic() - started < 1.0 + + +def test_sleep_wakes_when_stop_requested_from_another_thread(): + def _trip(): + time.sleep(0.05) + shutdown.request_stop() + + waker = threading.Thread(target=_trip) + waker.start() + started = time.monotonic() + shutdown.sleep(30) + waker.join() + assert time.monotonic() - started < 1.0 + + +# --------------------------------------------------------------------------- +# Signal handlers: first signal requests stop, second forces a hard exit +# --------------------------------------------------------------------------- + + +def test_signal_handler_requests_stop_then_escalates(): + previous = signal.getsignal(signal.SIGINT) + try: + shutdown.install_signal_handlers() + handler = signal.getsignal(signal.SIGINT) + assert callable(handler) + + # First Ctrl+C: graceful stop requested, no exception. + handler(signal.SIGINT, None) + assert shutdown.stop_requested() is True + + # Second Ctrl+C: escalate to a hard interrupt. + with pytest.raises(KeyboardInterrupt): + handler(signal.SIGINT, None) + finally: + signal.signal(signal.SIGINT, previous) + + +# --------------------------------------------------------------------------- +# HTTP layer bails out instead of issuing more requests +# --------------------------------------------------------------------------- + + +def test_fetch_with_retry_returns_none_without_touching_client(): + class _ExplodingClient: + def get(self, *_a, **_k): # pragma: no cover - must never be called + raise AssertionError("no request should be made after shutdown") + + shutdown.request_stop() + assert http_client.fetch_with_retry(_ExplodingClient(), "http://x") is None + + +# --------------------------------------------------------------------------- +# A source loop stops at the next outcode boundary +# --------------------------------------------------------------------------- + + +def _londonish_prop(outcode): + return {"id": outcode, "Postcode": f"{outcode} 0HD", "price": 100000, "Bedrooms": 2} + + +def test_scrape_rightmove_stops_after_signal(monkeypatch): + visited = [] + + class _DummyClient: + def close(self): + pass + + monkeypatch.setattr(scraper, "make_client", lambda: _DummyClient()) + monkeypatch.setattr(scraper, "resolve_outcode_id", lambda _client, oc: oc) + monkeypatch.setattr(scraper, "DELAY_BETWEEN_OUTCODES", 0) + + def fake_search(_client, _oid, outcode, *_a, **_k): + visited.append(outcode) + # Operator hits Ctrl+C while the first outcode is being processed. + shutdown.request_stop() + return [_londonish_prop(outcode)] + + monkeypatch.setattr(scraper, "rightmove_search_outcode", fake_search) + + results = {source: [] for source in scraper.SOURCE_ORDER} + errors: list[str] = [] + scraper._scrape_rightmove( + ["SW9", "E1", "BR1"], pc_index=None, results=results, + errors=errors, max_properties_per_source=None, + ) + + # Only the first outcode ran; the loop bailed before the next boundary. + assert visited == ["SW9"] + assert len(results["rightmove"]) == 1 + assert errors == [] diff --git a/frontend/src/lib/crime-filter.ts b/frontend/src/lib/crime-filter.ts index 76fca4f..9e6ad0a 100644 --- a/frontend/src/lib/crime-filter.ts +++ b/frontend/src/lib/crime-filter.ts @@ -100,9 +100,10 @@ export function getSpecificCrimeFilterMeta(features: FeatureMeta[]): FeatureMeta step: 1, description: 'Violence, burglary, robbery, drugs, shoplifting, vehicle crime, anti-social behaviour, public order, theft, and other crime types', - detail: 'Filter by one street-level crime category at a time using yearly averages per LSOA.', + detail: + 'Filter by one street-level crime category at a time using an area-normalised crime density near each postcode (not a count of incidents per year).', source: 'crime', - suffix: '/yr', + suffix: '', }; } diff --git a/frontend/src/lib/travel-params.ts b/frontend/src/lib/travel-params.ts index 0897e6f..bfe6419 100644 --- a/frontend/src/lib/travel-params.ts +++ b/frontend/src/lib/travel-params.ts @@ -42,10 +42,12 @@ export function dedupeTravelTimeEntries(entries: TravelTimeEntry[]): TravelTimeE label: existing.label || entry.label, timeRange: mergeTimeRanges(existing.timeRange, entry.timeRange), useBest: existing.useBest || entry.useBest, - // noChange/noBuses are part of the dedupe key, so for two entries to - // collide here they must already have matching flags. Carry them through - // explicitly so unset (undefined) doesn't clobber set (false/true). + // noChange/oneChange/noBuses are part of the dedupe key, so for two + // entries to collide here they must already have matching flags. Carry + // them through explicitly so unset (undefined) doesn't clobber set + // (false/true). noChange: existing.noChange ?? entry.noChange, + oneChange: existing.oneChange ?? entry.oneChange, noBuses: existing.noBuses ?? entry.noBuses, }; } diff --git a/frontend/src/lib/url-state.test.ts b/frontend/src/lib/url-state.test.ts index bfd3c2f..5c48d55 100644 --- a/frontend/src/lib/url-state.test.ts +++ b/frontend/src/lib/url-state.test.ts @@ -49,6 +49,7 @@ describe('url-state', () => { timeRange: [0, 30], useBest: true, noChange: false, + oneChange: false, noBuses: false, }, ]); @@ -118,6 +119,7 @@ describe('url-state', () => { timeRange: [10, 45], useBest: false, noChange: false, + oneChange: false, noBuses: false, }, ]); diff --git a/frontend/src/lib/url-state.ts b/frontend/src/lib/url-state.ts index d61cf22..843e5e2 100644 --- a/frontend/src/lib/url-state.ts +++ b/frontend/src/lib/url-state.ts @@ -304,9 +304,9 @@ export function parseUrlState(): UrlState { // Travel time: repeated `tt` params // Format: serverMode:slug:label[:b][:min:max] // serverMode is one of: car | bicycle | walking | transit | transit-no-bus - // | transit-no-change | transit-no-change-no-bus. transit-one-change[-no-bus] - // variants are server-side only and will cause the entry to be dropped here - // (parseServerMode returns null) so we don't silently broaden the user's filter. + // | transit-no-change | transit-no-change-no-bus | transit-one-change + // | transit-one-change-no-bus. Unknown modes cause the entry to be dropped + // here (parseServerMode returns null) so we don't silently broaden the filter. const ttParams = params.getAll('tt'); if (ttParams.length > 0) { const entries: TravelTimeEntry[] = []; @@ -337,6 +337,7 @@ export function parseUrlState(): UrlState { timeRange, useBest, noChange: parsedMode.noChange, + oneChange: parsedMode.oneChange, noBuses: parsedMode.noBuses, }); } diff --git a/pipeline/transform/join_epc_pp.py b/pipeline/transform/join_epc_pp.py index 40fed2c..fcbd92f 100644 --- a/pipeline/transform/join_epc_pp.py +++ b/pipeline/transform/join_epc_pp.py @@ -52,6 +52,13 @@ JUMP_MIN_PRICE = 2_000_000 MIN_BUILD_YEAR = 1700 MAX_BUILD_YEAR = 2030 +# Open-ended "before YYYY" bands (EPC only emits "before 1900") have no lower +# bound. Rather than dropping these mostly Victorian-and-older dwellings, we +# publish a representative year this many years under the stated upper bound +# ("before 1900" -> 1890) so they still carry a build year and survive an +# age-range filter; is_construction_date_approximate flags it as an estimate. +OPEN_BAND_YEARS_BEFORE_BOUND = 10 + # Plausibility bounds for raw EPC dimensions. EPC lodgements contain data-entry # errors (0 m storey heights, 116 m "interior height", 9,210 m² floor areas, 99 # habitable rooms) that otherwise propagate verbatim into the published per- @@ -68,10 +75,13 @@ def epc_band_to_year(band: pl.Expr) -> pl.Expr: EPC age bands are ranges (e.g. ``1950-1966``); we use the band MIDPOINT (1958) rather than the lower bound, which previously biased every band-derived - year ~10-15 years too young. Open-ended lower bands (``before 1900``) are too - wide to pin to a year and return null. Single-year / ``... onwards`` bands use - that year. Already-numeric inputs (a year produced by an earlier call) pass - through unchanged. Years outside [MIN_BUILD_YEAR, MAX_BUILD_YEAR] are nulled. + year ~10-15 years too young. Open-ended lower bands (``before 1900``) have no + lower bound, so we publish a representative year OPEN_BAND_YEARS_BEFORE_BOUND + years under the stated upper bound (``before 1900`` -> 1890) rather than + dropping the dwelling; is_construction_date_approximate flags it as an + estimate. Single-year / ``... onwards`` bands use that year. Already-numeric + inputs (a year produced by an earlier call) pass through unchanged. Years + outside [MIN_BUILD_YEAR, MAX_BUILD_YEAR] are nulled. """ text = ( band.cast(pl.Utf8) @@ -82,7 +92,7 @@ def epc_band_to_year(band: pl.Expr) -> pl.Expr: high = text.str.extract(r"(\d{4})\D+(\d{4})", 2).cast(pl.Int32, strict=False) year = ( pl.when(text.str.starts_with("before ")) - .then(None) + .then(low - OPEN_BAND_YEARS_BEFORE_BOUND) .when(high.is_not_null()) .then(((low + high) / 2).round(0).cast(pl.Int32)) .otherwise(low) diff --git a/pipeline/transform/merge.py b/pipeline/transform/merge.py index 2da4bdd..e8dd73b 100644 --- a/pipeline/transform/merge.py +++ b/pipeline/transform/merge.py @@ -1149,8 +1149,9 @@ def _canonical_epc_property_type_expr() -> pl.Expr: def _construction_year_expr(column: str = "construction_age_band") -> pl.Expr: # Use the shared band->midpoint-year mapping so the direct-EPC / listings - # path matches join_epc_pp (band midpoint, not lower bound; 'before 1900' and - # implausible years -> null). Already-numeric inputs pass through unchanged. + # path matches join_epc_pp (band midpoint, not lower bound; 'before 1900' -> + # 1890 representative year; implausible years -> null). Already-numeric inputs + # pass through unchanged. return epc_band_to_year(pl.col(column)) @@ -1728,7 +1729,7 @@ def _property_match_candidate_frame(wide: pl.LazyFrame) -> pl.DataFrame: def _index_candidates( - candidates: pl.DataFrame, postcode_key: str, uprn_key: str + candidates: pl.DataFrame, postcode_key: str, uprn_key: str, address_key: str ) -> tuple[dict[str, list[dict]], dict[str, dict]]: """Index candidate rows for matching, in a single pass over the frame. @@ -1736,17 +1737,41 @@ def _index_candidates( fuzzy street-address match; the UPRN index drives the exact match and is postcode-independent, so it still resolves when a listing's postcode is slightly off. + + The EPC register's UPRN is NOT unique: a single building/parent UPRN fans + across many distinct flats (up to 58 distinct (address, postcode) rows in + the 2026-06 data; ~9k UPRNs collide, touching ~20k epc_pp rows). Such a + UPRN cannot serve as a 1:1 exact-match key — it would mis-link a listing to + one arbitrary flat — so any UPRN that resolves to more than one distinct + ``(postcode_key, address_key)`` identity is dropped from ``uprn_index``; + those listings fall back to the fuzzy street-address matcher, which + disambiguates the specific flat. A UPRN repeated for the SAME identity + (one genuine property) is kept. """ buckets: dict[str, list[dict]] = {} - uprn_index: dict[str, dict] = {} + # uprn -> first_row, plus the identity of that first row; a uprn drops out + # the moment a second distinct (postcode, address) identity appears. + uprn_first: dict[str, dict] = {} + uprn_identity: dict[str, tuple] = {} + uprn_dropped: set[str] = set() for row in candidates.iter_rows(named=True): postcode = row.get(postcode_key) if postcode: buckets.setdefault(postcode, []).append(row) uprn = _normalize_uprn(row.get(uprn_key)) - if uprn and uprn not in uprn_index: - uprn_index[uprn] = row - return buckets, uprn_index + if not uprn or uprn in uprn_dropped: + continue + identity = (row.get(postcode_key), row.get(address_key)) + if uprn not in uprn_first: + uprn_first[uprn] = row + uprn_identity[uprn] = identity + elif identity != uprn_identity[uprn]: + # Same UPRN, different (postcode, address): a non-unique parent/ + # building UPRN. Remove it so it cannot act as a 1:1 key. + del uprn_first[uprn] + del uprn_identity[uprn] + uprn_dropped.add(uprn) + return buckets, uprn_first def _best_listing_property_candidate( @@ -1783,7 +1808,10 @@ def _match_listing_properties( return _empty_listing_property_matches() buckets, uprn_index = _index_candidates( - property_candidates, "_property_match_postcode", "uprn" + property_candidates, + "_property_match_postcode", + "uprn", + "_property_match_address", ) best_matches = [] for listing in listing_matches.iter_rows(named=True): @@ -1909,7 +1937,10 @@ def _match_direct_epc( return _empty_direct_epc_matches() buckets, uprn_index = _index_candidates( - epc_candidates, "_direct_epc_match_postcode", "_direct_epc_uprn" + epc_candidates, + "_direct_epc_match_postcode", + "_direct_epc_uprn", + "_direct_epc_match_address", ) street_index, noise_tokens = _index_epc_streets(epc_candidates) street_score_cache: dict[tuple[str, str], list[tuple[int, str]]] = {} @@ -2214,6 +2245,25 @@ class _BuildResult: listings: pl.DataFrame | None = None +# Property-level Yes/No flags default to "No" once all EPC + listings overlays +# have been coalesced. A property with no EPC match has no recorded social +# tenure / listed status, which is "No", not "unknown": join_epc_pp fills +# was_council_house with "No" only for EPC-matched rows (it runs before the +# fuzzy join), so without this the ~32% of EPC-unmatched properties would +# publish null instead of "No". +_PROPERTY_LEVEL_NO_DEFAULT_COLUMNS = (LISTED_BUILDING_FEATURE, "was_council_house") + + +def _fill_property_level_no_defaults(frame: pl.LazyFrame) -> pl.LazyFrame: + """Default the property-level Yes/No flag columns to "No" where null.""" + return frame.with_columns( + *( + pl.col(column).fill_null("No") + for column in _PROPERTY_LEVEL_NO_DEFAULT_COLUMNS + ) + ) + + def _build( epc_pp_path: Path, arcgis_path: Path, @@ -2303,7 +2353,12 @@ def _build( ) wide = _filter_to_active_english_postcodes(wide, active_postcodes) - wide = wide.with_columns(pl.col(LISTED_BUILDING_FEATURE).fill_null("No")) + # Default property-level Yes/No flags to "No" here: after the listings + # overlay coalesce (so a directly-matched "Yes" survives) and before the + # rename to "Former council house". This is the single place the FINAL + # was_council_house column (null for ~32% EPC-unmatched rows) gets its "No" + # default, alongside Listed building. + wide = _fill_property_level_no_defaults(wide) # NSPL Feb 2026 renamed geographic code columns to {field}{year}cd. # `_active_english_postcode_area` aliases them back to the short canonical diff --git a/pipeline/transform/test_join_epc_pp.py b/pipeline/transform/test_join_epc_pp.py index 7f60279..3ea19a2 100644 --- a/pipeline/transform/test_join_epc_pp.py +++ b/pipeline/transform/test_join_epc_pp.py @@ -774,7 +774,7 @@ def test_epc_band_to_year_uses_midpoint_and_clamps(): "b": [ "England and Wales: 1950-1966", # midpoint 1958 "1900-1929", # midpoint 1914 - "England and Wales: before 1900", # too wide -> null + "England and Wales: before 1900", # open-ended -> 1900 - 10 = 1890 "2012 onwards", # single year "1012", # implausible -> null "2202", # implausible -> null @@ -784,4 +784,4 @@ def test_epc_band_to_year_uses_midpoint_and_clamps(): } ) years = df.select(epc_band_to_year(pl.col("b")).alias("y"))["y"].to_list() - assert years == [1958, 1914, None, 2012, None, None, None, 1958] + assert years == [1958, 1914, 1890, 2012, None, None, None, 1958] diff --git a/pipeline/transform/test_merge.py b/pipeline/transform/test_merge.py index f271d44..8d7788d 100644 --- a/pipeline/transform/test_merge.py +++ b/pipeline/transform/test_merge.py @@ -16,6 +16,7 @@ from pipeline.transform.merge import ( _best_listing_match, _coalesce_direct_epc_columns, _dedupe_collapsed_properties, + _fill_property_level_no_defaults, _filter_to_active_english_postcodes, _join_area_side_tables, _finalize_listings, @@ -943,6 +944,61 @@ def test_match_direct_epc_matches_by_uprn_across_postcodes() -> None: assert matches["_direct_epc_match_method"].to_list() == ["uprn"] +def test_match_direct_epc_ignores_nonunique_building_uprn() -> None: + # A parent/building UPRN that resolves to several distinct (address, + # postcode) flats cannot act as a 1:1 exact-match key — it would mis-link + # the listing to one arbitrary flat. The listing must fall through to the + # street-address matcher, which resolves the specific flat. + matches = _match_direct_epc( + _listing_matches( + [ + { + "_listing_uprn": "100000000001", + "_listing_match_address": "FLAT 2 EXAMPLE COURT", + "_listing_match_postcode": "AA11AA", + } + ] + ), + _direct_epc_candidates( + [ + { + "_direct_epc_uprn": "100000000001", + "_direct_epc_match_address": "FLAT 1 EXAMPLE COURT", + "_direct_epc_address": "Flat 1, Example Court", + }, + { + "_direct_epc_uprn": "100000000001", + "_direct_epc_match_address": "FLAT 2 EXAMPLE COURT", + "_direct_epc_address": "Flat 2, Example Court", + }, + ] + ), + ) + + assert matches.height == 1 + # Resolved by address, not the ambiguous building UPRN, and to the right flat. + assert matches["_direct_epc_match_method"].to_list() != ["uprn"] + assert matches["_direct_epc_address"].to_list() == ["Flat 2, Example Court"] + + +def test_fill_property_level_no_defaults_defaults_unmatched_to_no() -> None: + # EPC-unmatched properties arrive with null was_council_house / Listed + # building; they must publish "No" (not null), while a genuine "Yes" is + # preserved. + frame = pl.LazyFrame( + { + LISTED_BUILDING_FEATURE: ["Yes", None, "No"], + "was_council_house": ["Yes", None, "No"], + } + ) + + out = _fill_property_level_no_defaults(frame).collect() + + assert out["was_council_house"].to_list() == ["Yes", "No", "No"] + assert out[LISTED_BUILDING_FEATURE].to_list() == ["Yes", "No", "No"] + assert out["was_council_house"].null_count() == 0 + + def test_match_direct_epc_matches_by_address_in_same_postcode() -> None: matches = _match_direct_epc( _listing_matches([{"_listing_match_address": "1 EXAMPLE ROAD"}]), diff --git a/pipeline/transform/test_transform_poi.py b/pipeline/transform/test_transform_poi.py index 4599c8d..11bb3a4 100644 --- a/pipeline/transform/test_transform_poi.py +++ b/pipeline/transform/test_transform_poi.py @@ -3,9 +3,12 @@ import json import polars as pl from pipeline.transform.transform_poi import ( + BUS_STOP_DEDUP_RADIUS_M, _load_ofsted_ratings, _school_icon_category_expr, + _significant_tokens, osm_groceries_colocated_with_geolytix, + osm_stops_near_naptan, transform, transform_grocery_retail_points, ) @@ -71,6 +74,61 @@ def test_osm_groceries_colocated_with_geolytix_handles_empty_inputs(): assert osm_groceries_colocated_with_geolytix(osm, empty_glx) == [] +def test_significant_tokens_keeps_ms_short_brand(): + # "M&S" strips to "ms" (len 2); without the short-brand allowlist it would + # tokenise to set() and never dedupe against the GEOLYTIX "M&S" chain. + assert _significant_tokens("M&S") == {"ms"} + assert _significant_tokens("M&S Simply Food") == {"ms", "simply", "food"} + # Other sub-3-char tokens stay excluded (allowlist must remain minimal). + assert _significant_tokens("Co") == set() + + +def test_osm_groceries_colocated_with_geolytix_dedupes_m_and_s(): + # GEOLYTIX brand "M&S" must now match a colocated OSM "M&S Simply Food". + geolytix = pl.DataFrame({"category": ["M&S"], "lat": [53.0], "lng": [-1.5]}) + osm = pl.DataFrame( + { + "id": ["ms-dup"], + "name": ["M&S Simply Food"], + "lat": [53.00001], + "lng": [-1.5], + } + ) + assert osm_groceries_colocated_with_geolytix(osm, geolytix, radius_m=50.0) == [ + "ms-dup" + ] + + +def test_osm_groceries_colocated_default_radius_catches_60m_offset(): + # OSM and GEOLYTIX routinely place the same store 50-100m apart. The widened + # 100m default catches a ~60m same-brand duplicate that the old 50m missed. + geolytix = pl.DataFrame({"category": ["Tesco"], "lat": [53.0], "lng": [-1.5]}) + osm = pl.DataFrame( + # ~60 m north of the GEOLYTIX point. + {"id": ["dup"], "name": ["Tesco Express"], "lat": [53.00054], "lng": [-1.5]} + ) + assert osm_groceries_colocated_with_geolytix(osm, geolytix) == ["dup"] + assert osm_groceries_colocated_with_geolytix(osm, geolytix, radius_m=50.0) == [] + + +def test_osm_stops_near_naptan_drops_within_radius_keeps_far(): + # A ~60m-offset OSM platform duplicates the NaPTAN stop (dropped at the 100m + # default); a genuine opposite-direction stop ~160m away is kept. + naptan = pl.DataFrame({"id": ["n1"], "lat": [51.5000], "lng": [-0.1000]}) + osm = pl.DataFrame( + { + "id": ["dup", "far"], + # ~60 m and ~160 m north of the NaPTAN stop. + "lat": [51.50054, 51.50144], + "lng": [-0.1000, -0.1000], + } + ) + assert BUS_STOP_DEDUP_RADIUS_M == 100.0 + assert osm_stops_near_naptan(osm, naptan) == ["dup"] + # At the old 50m radius the duplicate survived (the bug this fix closes). + assert osm_stops_near_naptan(osm, naptan, radius_m=50.0) == [] + + def _write_boundary(tmp_path): """A FeatureCollection whose single feature covers the London-area test coords used by the transform() fixtures, so in_england_mask keeps them.""" @@ -505,6 +563,32 @@ def test_osm_supermarkets_dropped(tmp_path): assert convenience.height == 1 +def test_transform_relabels_osm_iceland_to_brand(tmp_path): + # OSM tags Iceland as shop/frozen_food -> "Deli & Specialty". A genuine + # Iceland store GEOLYTIX lacks (away from any GEOLYTIX point) must survive + # AND be relabelled to the "Iceland" brand, not ship mislabelled as a deli. + raw = pl.DataFrame( + { + "id": ["ice1"], + "name": ["Iceland"], + "category": ["shop/frozen_food"], + "lat": [51.40], + "lng": [-0.05], + } + ) + inputs = _write_transform_inputs(tmp_path, raw) + + out = transform(**inputs).collect() + + row = out.filter(pl.col("name") == "Iceland") + assert row.height == 1 + assert row["group"][0] == "Groceries" + assert row["category"][0] == "Iceland" + assert row["icon_category"][0] == "Iceland" + # No deli pin remains for the relabelled store. + assert out.filter(pl.col("category") == "Deli & Specialty").height == 0 + + def test_transform_grocery_dedup_drops_only_grocery_aspect(tmp_path): # The _write_transform_inputs fixture seeds 5 GEOLYTIX "Tesco" points at # (51.52, -0.14). An OSM object colocated there carrying "Tesco" in its name diff --git a/pipeline/transform/transform_poi.py b/pipeline/transform/transform_poi.py index 01b4303..26b48e2 100644 --- a/pipeline/transform/transform_poi.py +++ b/pipeline/transform/transform_poi.py @@ -1579,8 +1579,11 @@ def transform_gias_schools( # Store". GEOLYTIX is authoritative for its chains, so an OSM grocery row that # sits on top of a GEOLYTIX point AND carries that point's brand name is the # same physical store and is dropped. Independent corner shops never carry a -# chain brand, so they are kept. -GROCERY_DEDUP_RADIUS_M = 50.0 +# chain brand, so they are kept. 100m (not 50m) because OSM and GEOLYTIX +# routinely place the same branded store 50-100m apart (entrance vs centroid vs +# car park); at 50m ~166 same-brand duplicates (Iceland, Morrisons Daily, Tesco +# Express, Sainsbury's Local, ...) survived just outside the radius. +GROCERY_DEDUP_RADIUS_M = 100.0 # Brand-token aliases so an OSM name spelt differently from the GEOLYTIX brand # still matches. GEOLYTIX's "Co-op" tokenises to "coop", but OSM frequently @@ -1591,15 +1594,34 @@ _GROCERY_TOKEN_ALIASES = { "cooperatives": "coop", } +# Brand tokens shorter than the 3-char floor that must still match. "M&S" strips +# to "ms" (len 2) and would otherwise tokenise to set(), so M&S never deduped and +# OSM M&S grocery rows survived as phantom duplicates of the GEOLYTIX M&S chain. +# "M&S" is the only GEOLYTIX brand whose alnum form is <3 chars, so this +# allowlist is safe (no other brand collides). +_GROCERY_SHORT_BRAND_TOKENS = {"ms"} + +# OSM tags Iceland/Farmfoods stores shop/frozen_food, which CATEGORY_MAP routes +# to "Deli & Specialty". The grocery dedup drops the ones colocated with a +# GEOLYTIX twin, but genuine stores GEOLYTIX lacks would otherwise survive +# mislabelled as deli, shadowing the brand pill and falling outside the headline +# grocery metric. Relabel the survivors (matched by name) to the brand so they +# line up with the GEOLYTIX rows. +OSM_BRAND_RELABEL = { + "Iceland": "Iceland", + "Farmfoods": "Farmfoods", +} + def _significant_tokens(name: str | None) -> set[str]: - """Lower-case alphanumeric tokens of length >= 3 from a POI name (aliased).""" + """Lower-case alphanumeric tokens of length >= 3 from a POI name (aliased), + plus short brand tokens in _GROCERY_SHORT_BRAND_TOKENS (e.g. "ms" for M&S).""" if not name: return set() tokens: set[str] = set() for raw in str(name).lower().split(): token = "".join(ch for ch in raw if ch.isalnum()) - if len(token) >= 3: + if len(token) >= 3 or token in _GROCERY_SHORT_BRAND_TOKENS: tokens.add(_GROCERY_TOKEN_ALIASES.get(token, token)) return tokens @@ -1608,7 +1630,14 @@ def _significant_tokens(name: str | None) -> set[str]: # gaps. Where NaPTAN already has a stop within this radius the area is covered, # so the colocated OSM platform is dropped to avoid double-counting; OSM # platforms with no nearby NaPTAN stop (the gaps) are kept. -BUS_STOP_DEDUP_RADIUS_M = 50.0 +# +# 100 m, not 50 m: at 50 m ~24.6k OSM platforms 50-100 m from a NaPTAN stop +# survived and double-counted (16.7k at 50-75 m, 7.8k at 75-100 m). The NaPTAN +# nearest-neighbour spacing (opposite-side-of-road pairs) has a 166.7 m median +# (p25=98.7 m), so 100 m clears the 50-100 m duplicate spike while leaving a +# genuine opposite-direction stop (~166 m away) in place; 125-150 m would start +# dropping those real opposite-side stops. +BUS_STOP_DEDUP_RADIUS_M = 100.0 def osm_stops_near_naptan( @@ -1652,9 +1681,9 @@ def osm_groceries_colocated_with_geolytix( An OSM Groceries row is a duplicate when a GEOLYTIX point lies within ``radius_m`` metres AND that point's brand tokens (its ``category``, e.g. - "Tesco", "Co-op") are all present in the OSM row's name — i.e. the same - physical branded store. Brands with no token >= 3 chars (e.g. "M&S") never - match, so they are conservatively kept rather than risk a false drop. + "Tesco", "Co-op", "M&S") are all present in the OSM row's name — i.e. the + same physical branded store. Short brands like "M&S" match via + _GROCERY_SHORT_BRAND_TOKENS; brands that still tokenise to set() are kept. ``osm_groceries`` needs columns ``id``, ``name``, ``lat``, ``lng``; ``geolytix`` needs ``category`` (the brand), ``lat``, ``lng``. @@ -1774,6 +1803,31 @@ def transform( # pre-deduplicated. lf = lf.unique(subset=["id", "category"], keep="first", maintain_order=True) + # Relabel OSM Iceland/Farmfoods rows that CATEGORY_MAP put in "Deli & + # Specialty" to the brand, so the genuine stores GEOLYTIX lacks (the grocery + # dedup drops the colocated twins) line up with the GEOLYTIX brand pills and + # the headline grocery metric instead of shadowing them as deli. + for _brand_name, _brand in OSM_BRAND_RELABEL.items(): + _is_brand = ( + (pl.col("group") == "Groceries") + & (pl.col("category") == "Deli & Specialty") + & (pl.col("name") == _brand_name) + ) + lf = lf.with_columns( + pl.when(_is_brand) + .then(pl.lit(_brand)) + .otherwise(pl.col("category")) + .alias("category"), + pl.when(_is_brand) + .then(pl.lit(_brand)) + .otherwise(pl.col("icon_category")) + .alias("icon_category"), + pl.when(_is_brand) + .then(pl.lit("\U0001f6d2")) + .otherwise(pl.col("emoji")) + .alias("emoji"), + ) + naptan_df = pl.scan_parquet(naptan_path).collect() mask = in_england_mask( boundary_path, diff --git a/server-rs/src/data.rs b/server-rs/src/data.rs index e941e80..e79943a 100644 --- a/server-rs/src/data.rs +++ b/server-rs/src/data.rs @@ -6,6 +6,29 @@ mod postcodes; mod property; pub mod travel_time; +/// Apostrophe-like code points that should be elided (not treated as word breaks) when +/// normalizing search text. Keyboards, autocorrect and copy-paste all produce different glyphs +/// for what users mean as an apostrophe; treating any of them as a separator turns "King's Cross" +/// into the tokens `king s cross` and breaks matching. Covers the straight ASCII apostrophe, the +/// typographic left/right single quotes, grave/acute accents, the modifier-letter apostrophes, +/// prime, and the fullwidth apostrophe. +pub(crate) fn is_apostrophe(ch: char) -> bool { + matches!( + ch, + '\u{0027}' // ' APOSTROPHE + | '\u{0060}' // ` GRAVE ACCENT + | '\u{00B4}' // ´ ACUTE ACCENT + | '\u{02B9}' // ʹ MODIFIER LETTER PRIME + | '\u{02BB}' // ʻ MODIFIER LETTER TURNED COMMA (ʻokina) + | '\u{02BC}' // ʼ MODIFIER LETTER APOSTROPHE + | '\u{2018}' // ' LEFT SINGLE QUOTATION MARK + | '\u{2019}' // ' RIGHT SINGLE QUOTATION MARK + | '\u{201B}' // ‛ SINGLE HIGH-REVERSED-9 QUOTATION MARK + | '\u{2032}' // ′ PRIME + | '\u{FF07}' // ＇ FULLWIDTH APOSTROPHE + ) +} + fn panic_payload_message(payload: &(dyn std::any::Any + Send)) -> String { if let Some(message) = payload.downcast_ref::<&'static str>() { (*message).to_string() diff --git a/server-rs/src/data/places.rs b/server-rs/src/data/places.rs index 7f0b662..6f11afc 100644 --- a/server-rs/src/data/places.rs +++ b/server-rs/src/data/places.rs @@ -161,7 +161,7 @@ pub fn normalize_search_text(text: &str) -> String { let mut last_was_space = true; for ch in text.chars() { - if ch == '\'' || ch == '’' || ch == '`' { + if super::is_apostrophe(ch) { continue; } @@ -769,6 +769,26 @@ impl PlaceData { mod tests { use super::*; + #[test] + fn normalize_search_text_elides_every_apostrophe_variant() { + // Whatever glyph the keyboard / autocorrect / paste produced for the apostrophe, the + // normalized form must be identical — otherwise "King's Cross" tokenizes as `king s cross` + // and matching breaks. See is_apostrophe for the full set. + let expected = "kings cross"; + for q in [ + "King's Cross", // U+0027 straight + "King’s Cross", // U+2019 right single quote + "King‘s Cross", // U+2018 left single quote + "King´s Cross", // U+00B4 acute accent + "Kingʼs Cross", // U+02BC modifier letter apostrophe + "King′s Cross", // U+2032 prime + "King`s Cross", // U+0060 grave accent + "Kingʻs Cross", // U+02BB modifier letter turned comma + ] { + assert_eq!(normalize_search_text(q), expected, "failed for {q:?}"); + } + } + #[test] fn place_index_tokens_dedup_and_min_length() { // "a" is too short; aliases split on " | ". diff --git a/server-rs/src/data/property/address_search.rs b/server-rs/src/data/property/address_search.rs index 0a2c4d1..34acc21 100644 --- a/server-rs/src/data/property/address_search.rs +++ b/server-rs/src/data/property/address_search.rs @@ -37,7 +37,7 @@ fn tokenize_address_text(text: &str) -> Vec { for ch in text.chars() { if ch.is_ascii_alphanumeric() { current.push(ch.to_ascii_lowercase()); - } else if matches!(ch, '\'' | '’' | '`') { + } else if crate::data::is_apostrophe(ch) { continue; } else if !current.is_empty() { tokens.push(std::mem::take(&mut current)); @@ -784,6 +784,24 @@ impl PropertyData { mod tests { use super::*; + #[test] + fn tokenize_address_text_elides_every_apostrophe_variant() { + // The query and the index both run through this tokenizer, so any apostrophe glyph must + // join the word ("O'Brien" -> "obrien") rather than split it; otherwise the same address + // tokenizes differently depending on which quote glyph was typed. + let expected = vec!["obrien".to_string(), "road".to_string()]; + for addr in [ + "O'Brien Road", // U+0027 straight + "O’Brien Road", // U+2019 right single quote + "O‘Brien Road", // U+2018 left single quote + "O´Brien Road", // U+00B4 acute accent + "OʼBrien Road", // U+02BC modifier letter apostrophe + "O′Brien Road", // U+2032 prime + ] { + assert_eq!(tokenize_address_text(addr), expected, "failed for {addr:?}"); + } + } + #[test] fn full_postcode_detection_accepts_common_formats() { assert!(is_full_postcode_compact("SW1A1AA")); diff --git a/server-rs/src/features.rs b/server-rs/src/features.rs index 51bacca..6c82179 100644 --- a/server-rs/src/features.rs +++ b/server-rs/src/features.rs @@ -31,10 +31,17 @@ pub struct FeatureConfig { pub absolute: bool, } -/// Features whose histogram bins should be exactly 1 unit wide (one per integer). -/// p1/p99 are snapped to integer boundaries before binning. +/// Explicit override for features that should use integer-width histogram bins +/// (one bin per integer, p1/p99 snapped to integer boundaries) even if they +/// don't match the automatic rule in [`has_integer_bins`]. Most integer count +/// features are detected automatically from their config. pub const INTEGER_BIN_FEATURES: &[&str] = &["Number of bedrooms & living rooms"]; +/// Largest p1–p99 span (in integer units) for which one-bin-per-integer +/// histograms stay reasonable. Above this, percentile binning is used instead so +/// wide-range features (years, percentages, prices) don't allocate one bin each. +const MAX_INTEGER_BIN_RANGE: f32 = 16.0; + pub struct EnumFeatureConfig { pub name: &'static str, /// If set, values are presented in this order instead of alphabetical. @@ -473,11 +480,11 @@ pub static FEATURE_GROUPS: &[FeatureGroup] = &[ high: 98.0, }, step: 1.0, - description: "Aggregate of serious crime categories per year", - detail: "Sum of violence, robbery, burglary, and weapons possession per year near the postcode, counted from police.uk street-level crime points (anonymised, snapped to nearby map points). This is an area-normalised incident density for the surrounding streets, not a per-resident risk: busy commercial centres rank high however few people live there. Averaged over the months the local police force actually published data; known force gaps (e.g. Greater Manchester since mid-2019) are excluded rather than counted as zero crime.", + description: "Relative density of serious crime categories near the postcode", + detail: "Combined density of violence, robbery, burglary, and weapons possession near the postcode, counted from police.uk street-level crime points (anonymised, snapped to nearby map points). This is an area-normalised incident density for the surrounding streets, not a count of incidents per year and not a per-resident risk: busy commercial centres rank high however few people live there. It is normalised to a median-sized catchment so areas are comparable, and computed over the months the local police force actually published data; known force gaps (e.g. Greater Manchester since mid-2019) are excluded rather than counted as zero crime.", source: "crime", prefix: "", - suffix: "/yr", + suffix: "", raw: false, absolute: false, }), @@ -488,11 +495,11 @@ pub static FEATURE_GROUPS: &[FeatureGroup] = &[ high: 98.0, }, step: 1.0, - description: "Aggregate of minor crime categories per year", - detail: "Sum of anti-social behaviour, shoplifting, bicycle theft, and other lower-severity crime per year near the postcode, counted from police.uk street-level crime points (anonymised, snapped to nearby map points). This is an area-normalised incident density for the surrounding streets, not a per-resident risk: busy commercial centres rank high however few people live there. Averaged over the months the local police force actually published data; known force gaps (e.g. Greater Manchester since mid-2019) are excluded rather than counted as zero crime.", + description: "Relative density of minor crime categories near the postcode", + detail: "Combined density of anti-social behaviour, shoplifting, bicycle theft, and other lower-severity crime near the postcode, counted from police.uk street-level crime points (anonymised, snapped to nearby map points). This is an area-normalised incident density for the surrounding streets, not a count of incidents per year and not a per-resident risk: busy commercial centres rank high however few people live there. It is normalised to a median-sized catchment so areas are comparable, and computed over the months the local police force actually published data; known force gaps (e.g. Greater Manchester since mid-2019) are excluded rather than counted as zero crime.", source: "crime", prefix: "", - suffix: "/yr", + suffix: "", raw: false, absolute: false, }), @@ -507,7 +514,7 @@ pub static FEATURE_GROUPS: &[FeatureGroup] = &[ detail: "Average number of violence and sexual offences per year near the postcode, from police.uk street-level crime data. Includes assault, harassment, and sexual offences.", source: "crime", prefix: "", - suffix: "/yr", + suffix: "", raw: false, absolute: false, }), @@ -522,7 +529,7 @@ pub static FEATURE_GROUPS: &[FeatureGroup] = &[ detail: "Average number of burglary offences per year near the postcode, from police.uk street-level crime data. Includes residential and commercial burglary.", source: "crime", prefix: "", - suffix: "/yr", + suffix: "", raw: false, absolute: false, }), @@ -537,7 +544,7 @@ pub static FEATURE_GROUPS: &[FeatureGroup] = &[ detail: "Average number of robbery offences per year near the postcode, from police.uk street-level crime data. Robbery involves theft with force or threat of force.", source: "crime", prefix: "", - suffix: "/yr", + suffix: "", raw: false, absolute: false, }), @@ -552,7 +559,7 @@ pub static FEATURE_GROUPS: &[FeatureGroup] = &[ detail: "Average number of vehicle crime incidents per year near the postcode, from police.uk street-level crime data. Includes theft of and from vehicles.", source: "crime", prefix: "", - suffix: "/yr", + suffix: "", raw: false, absolute: false, }), @@ -567,7 +574,7 @@ pub static FEATURE_GROUPS: &[FeatureGroup] = &[ detail: "Average number of anti-social behaviour incidents per year near the postcode, from police.uk street-level crime data. Includes nuisance, environmental, and personal anti-social behaviour.", source: "crime", prefix: "", - suffix: "/yr", + suffix: "", raw: false, absolute: false, }), @@ -582,7 +589,7 @@ pub static FEATURE_GROUPS: &[FeatureGroup] = &[ detail: "Average number of criminal damage and arson incidents per year near the postcode, from police.uk street-level crime data.", source: "crime", prefix: "", - suffix: "/yr", + suffix: "", raw: false, absolute: false, }), @@ -597,7 +604,7 @@ pub static FEATURE_GROUPS: &[FeatureGroup] = &[ detail: "Average number of 'other theft' offences per year near the postcode, from police.uk street-level crime data. Includes theft not classified under burglary, vehicle crime, shoplifting, or bicycle theft.", source: "crime", prefix: "", - suffix: "/yr", + suffix: "", raw: false, absolute: false, }), @@ -612,7 +619,7 @@ pub static FEATURE_GROUPS: &[FeatureGroup] = &[ detail: "Average number of theft from the person offences per year near the postcode, from police.uk street-level crime data. Includes pickpocketing and bag snatching without force.", source: "crime", prefix: "", - suffix: "/yr", + suffix: "", raw: false, absolute: false, }), @@ -627,7 +634,7 @@ pub static FEATURE_GROUPS: &[FeatureGroup] = &[ detail: "Average number of shoplifting offences per year near the postcode, from police.uk street-level crime data.", source: "crime", prefix: "", - suffix: "/yr", + suffix: "", raw: false, absolute: false, }), @@ -642,7 +649,7 @@ pub static FEATURE_GROUPS: &[FeatureGroup] = &[ detail: "Average number of bicycle theft offences per year near the postcode, from police.uk street-level crime data.", source: "crime", prefix: "", - suffix: "/yr", + suffix: "", raw: false, absolute: false, }), @@ -657,7 +664,7 @@ pub static FEATURE_GROUPS: &[FeatureGroup] = &[ detail: "Average number of drug offences per year near the postcode, from police.uk street-level crime data. Includes possession and trafficking offences.", source: "crime", prefix: "", - suffix: "/yr", + suffix: "", raw: false, absolute: false, }), @@ -672,7 +679,7 @@ pub static FEATURE_GROUPS: &[FeatureGroup] = &[ detail: "Average number of possession of weapons offences per year near the postcode, from police.uk street-level crime data.", source: "crime", prefix: "", - suffix: "/yr", + suffix: "", raw: false, absolute: false, }), @@ -687,7 +694,7 @@ pub static FEATURE_GROUPS: &[FeatureGroup] = &[ detail: "Average number of public order offences per year near the postcode, from police.uk street-level crime data. Includes causing fear, alarm, or distress.", source: "crime", prefix: "", - suffix: "/yr", + suffix: "", raw: false, absolute: false, }), @@ -702,7 +709,7 @@ pub static FEATURE_GROUPS: &[FeatureGroup] = &[ detail: "Average number of other crime offences per year near the postcode, from police.uk street-level crime data. A catch-all category for offences not classified elsewhere.", source: "crime", prefix: "", - suffix: "/yr", + suffix: "", raw: false, absolute: false, }), @@ -734,7 +741,7 @@ pub static FEATURE_GROUPS: &[FeatureGroup] = &[ }, step: 1.0, description: "Percentage of population identifying as White", - detail: "From the 2021 Census. Percentage of the local authority population identifying as White (English, Welsh, Scottish, Northern Irish, British, Irish, Gypsy or Irish Traveller, Roma, or any other White background).", + detail: "From the 2021 Census. Percentage of the local neighbourhood (LSOA) population identifying as White (English, Welsh, Scottish, Northern Irish, British, Irish, Gypsy or Irish Traveller, Roma, or any other White background).", source: "ethnicity", prefix: "", suffix: "%", @@ -749,7 +756,7 @@ pub static FEATURE_GROUPS: &[FeatureGroup] = &[ }, step: 0.1, description: "Percentage of population identifying as South Asian", - detail: "From the 2021 Census. Percentage of the local authority population identifying as Indian, Pakistani, Bangladeshi, or any other Asian background.", + detail: "From the 2021 Census. Percentage of the local neighbourhood (LSOA) population identifying as Indian, Pakistani, or Bangladeshi.", source: "ethnicity", prefix: "", suffix: "%", @@ -764,7 +771,7 @@ pub static FEATURE_GROUPS: &[FeatureGroup] = &[ }, step: 0.1, description: "Percentage of population identifying as Black", - detail: "From the 2021 Census. Percentage of the local authority population identifying as Black, Black British, Caribbean, or African.", + detail: "From the 2021 Census. Percentage of the local neighbourhood (LSOA) population identifying as Black, Black British, Caribbean, or African.", source: "ethnicity", prefix: "", suffix: "%", @@ -779,7 +786,7 @@ pub static FEATURE_GROUPS: &[FeatureGroup] = &[ }, step: 0.1, description: "Percentage of population identifying as East Asian", - detail: "From the 2021 Census. Percentage of the local authority population identifying as Chinese.", + detail: "From the 2021 Census. Percentage of the local neighbourhood (LSOA) population identifying as Chinese.", source: "ethnicity", prefix: "", suffix: "%", @@ -794,7 +801,7 @@ pub static FEATURE_GROUPS: &[FeatureGroup] = &[ }, step: 0.1, description: "Percentage of population identifying as Southeast Asian", - detail: "From the 2021 Census. Percentage of the local authority population identifying as another (non-Chinese) East or Southeast Asian background, e.g. Vietnamese, Filipino, or Thai.", + detail: "From the 2021 Census. Percentage of the local neighbourhood (LSOA) population identifying as another (non-Chinese) East or Southeast Asian background, e.g. Vietnamese, Filipino, or Thai.", source: "ethnicity", prefix: "", suffix: "%", @@ -809,7 +816,7 @@ pub static FEATURE_GROUPS: &[FeatureGroup] = &[ }, step: 0.1, description: "Percentage of population identifying as Mixed or Multiple ethnic groups", - detail: "From the 2021 Census. Percentage of the local authority population identifying as Mixed or Multiple ethnic groups (White and Black Caribbean, White and Black African, White and Asian, or any other Mixed or Multiple background).", + detail: "From the 2021 Census. Percentage of the local neighbourhood (LSOA) population identifying as Mixed or Multiple ethnic groups (White and Black Caribbean, White and Black African, White and Asian, or any other Mixed or Multiple background).", source: "ethnicity", prefix: "", suffix: "%", @@ -824,7 +831,7 @@ pub static FEATURE_GROUPS: &[FeatureGroup] = &[ }, step: 0.1, description: "Percentage of population identifying as Other ethnic group", - detail: "From the 2021 Census. Percentage of the local authority population identifying as Other ethnic group (Arab or any other ethnic group not covered by the main categories).", + detail: "From the 2021 Census. Percentage of the local neighbourhood (LSOA) population identifying as Other ethnic group (Arab or any other ethnic group not covered by the main categories).", source: "ethnicity", prefix: "", suffix: "%", @@ -997,9 +1004,48 @@ pub fn order_for(name: &str) -> Option<&'static [&'static str]> { .flatten() } -/// Whether this feature should use integer-width histogram bins. +/// Whether this feature should use integer-width histogram bins: one bin per +/// integer value with p1/p99 snapped to integer boundaries. This keeps the +/// histogram bars aligned 1:1 with the integer axis labels in the UI (otherwise +/// fractional percentiles and non-unit bin widths make the lit bar drift away +/// from its label, e.g. a value of 3 landing under the "2" tick). +/// +/// Applies to small-range integer-valued count features (school catchments, +/// bedrooms, …) plus the dynamic POI count features. Wide-range or fractional +/// features (years, percentages, crime averages) keep percentile binning. pub fn has_integer_bins(name: &str) -> bool { - INTEGER_BIN_FEATURES.contains(&name) || dynamic_poi_count_radius(name).is_some() + if INTEGER_BIN_FEATURES.contains(&name) || dynamic_poi_count_radius(name).is_some() { + return true; + } + matches!(numeric_config_for(name), Some(c) if is_integer_count_feature(c)) +} + +/// True for a numeric feature whose values are small integer counts: unit step +/// with fixed integer bounds spanning a small enough range for one bin per value. +fn is_integer_count_feature(config: &FeatureConfig) -> bool { + if config.step != 1.0 { + return false; + } + match config.bounds { + Bounds::Fixed { min, max } => { + min.fract() == 0.0 + && max.fract() == 0.0 + && max > min + && (max - min) <= MAX_INTEGER_BIN_RANGE + } + Bounds::Percentile { .. } => false, + } +} + +/// Look up the static numeric feature config by exact name. +fn numeric_config_for(name: &str) -> Option<&'static FeatureConfig> { + FEATURE_GROUPS + .iter() + .flat_map(|group| group.features.iter()) + .find_map(|feature| match feature { + Feature::Numeric(c) if c.name == name => Some(c), + _ => None, + }) } /// Look up the Bounds config for a numeric feature by name. @@ -1084,3 +1130,44 @@ pub const POI_GROUP_ORDER: &[&str] = &[ "Services", "Shops", ]; + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn integer_bins_cover_small_integer_count_features() { + // Small integer count features must get one bin per integer so the + // histogram bars line up with their integer axis labels. + for name in [ + "Good+ primary school catchments", + "Good+ secondary school catchments", + "Outstanding primary school catchments", + "Outstanding secondary school catchments", + "Number of bedrooms & living rooms", + ] { + assert!(has_integer_bins(name), "{name} should use integer bins"); + } + } + + #[test] + fn integer_bins_exclude_wide_range_and_fractional_features() { + // step == 1.0 but wide-range (would explode bin count) or fractional + // (averages/percentiles) features must keep percentile binning. + for name in [ + "Construction year", // Fixed but spans ~2000 years + "Date of last transaction", // Fixed, fractional years + "Income Score", // 0..100 percentile + "% White", // 0..100 percentage + "Noise (dB)", // 50..80, range > threshold + "Serious crime (avg/yr)", // Percentile bounds, fractional + "Interior height (m)", // step 0.1 + "Estimated current price", // step 10000 + ] { + assert!( + !has_integer_bins(name), + "{name} should not use integer bins" + ); + } + } +} diff --git a/server-rs/src/routes/export.rs b/server-rs/src/routes/export.rs index b057285..d15b711 100644 --- a/server-rs/src/routes/export.rs +++ b/server-rs/src/routes/export.rs @@ -440,6 +440,11 @@ pub async fn get_export( .map_err(|err| (StatusCode::BAD_REQUEST, err).into_response())? }; let has_poi_filters = !parsed_poi_filters.is_empty(); + // The active filters decide which feature columns land on the "Selected" + // sheet. We keep them for column selection in both modes; they are only + // *applied* as row filters in bounds mode (in list mode the user picks the + // postcodes explicitly, so rows are never filtered). + let column_filters_str = params.filters.clone(); let filters_str = if is_postcode_mode { None } else { @@ -653,7 +658,7 @@ pub async fn get_export( }; // Determine column order: filter features first, then remaining - let filter_feature_names = extract_filter_feature_names(filters_str.as_deref()); + let filter_feature_names = extract_filter_feature_names(column_filters_str.as_deref()); let field_indices = parse_field_indices_with_poi( fields_str.as_deref(), @@ -841,18 +846,16 @@ pub async fn get_export( frontend_params ); - // Bounds mode: two sheets — "Selected" (filter features with link + screenshot) - // and "All Data" (all features). - // List mode: single sheet "Postcodes" with all data, no link or screenshot - // (the supplied list isn't tied to a map view). - let sheet_configs: Vec<(&str, &[usize], bool)> = if postcode_list_entries.is_some() { - vec![("Postcodes", &all_feature_indices, false)] - } else { - vec![ - ("Selected", &filter_feature_indices, true), - ("All Data", &all_feature_indices, false), - ] - }; + // Two sheets in both modes: "Selected" (just the features behind the + // active filters) and "All Data" (every feature). The Selected sheet + // carries the dashboard link + screenshot only in bounds mode, where the + // export is tied to a map view; in list mode the user picked the + // postcodes explicitly, so there's no map view to link or screenshot. + let is_list_mode = postcode_list_entries.is_some(); + let sheet_configs: Vec<(&str, &[usize], bool)> = vec![ + ("Selected", &filter_feature_indices, !is_list_mode), + ("All Data", &all_feature_indices, false), + ]; for (sheet_name, feat_indices, include_header) in &sheet_configs { let sheet = workbook.add_worksheet();