fix(tradein): region_for_point резолвит по настоящему полигону, не по bbox (#3052)
All checks were successful
CI Trade-In / changes (pull_request) Successful in 10s
CI Trade-In / frontend-checks (pull_request) Has been skipped
CI Trade-In / browser-tests (pull_request) Has been skipped
CI / changes (pull_request) Successful in 12s
CI / backend-tests (pull_request) Has been skipped
CI / frontend-tests (pull_request) Has been skipped
CI / openapi-codegen-check (pull_request) Has been skipped
CI Trade-In / backend-tests (pull_request) Successful in 6m3s
All checks were successful
CI Trade-In / changes (pull_request) Successful in 10s
CI Trade-In / frontend-checks (pull_request) Has been skipped
CI Trade-In / browser-tests (pull_request) Has been skipped
CI / changes (pull_request) Successful in 12s
CI / backend-tests (pull_request) Has been skipped
CI / frontend-tests (pull_request) Has been skipped
CI / openapi-codegen-check (pull_request) Has been skipped
CI Trade-In / backend-tests (pull_request) Successful in 6m3s
Прямоугольники bbox_region Москвы (77) и области (50) пересекались: bbox_region(50) геометрически содержит bbox_region(77) как прямоугольник, и region_for_point отдавал точку первому по площади bbox совпадению без учёта того, что реальные админ-границы не пересекаются. Из-за этого Химки, Реутов, Котельники, Люберцы резолвились в Москву — 8 647 московских лотов уходили в отказ по покрытию, а 11 172 областные сделки ложно попадали в московское ядро, хотя коридор ДКП фильтрует строго по коду региона. Границы — полигоны OSM/Nominatim (region_boundaries/boundaries.geojson.json, ODbL, _source/_license сохранены в файле по требованию лицензии), парсятся один раз на импорте модуля. Point-in-polygon — ray casting (PNPOLY) с even-odd правилом по плоскому списку колец: один и тот же код корректно обрабатывает и дыру (Москва вырезана из полигона области) и мультиполигон (10 несвязных частей Москвы — Зеленоград, анклавы). Без shapely — его нет в зависимостях tradein backend, тянуть ради одной функции незачем. bbox_region остаётся дешёвым предфильтром перед полигоном (его читают geocoder.py и msk_raw_import.py), просто больше не финальный ответ: порядок _POINT_LOOKUP_ORDER (компактный bbox раньше объёмного) теперь экономит polygon-проверки на частом случае Москва/область, а не определяет корректность. Тест на Химки/Балашиху (test_3051_region_registry_moscow_oblast.py) раньше фиксировал это как "известное ограничение" bbox-резолва — обновлён под исправленное поведение (code 50, не 77). Новый test_3052_region_polygon_ boundary.py — приёмка по девяти контрольным точкам issue #3052 + инвариант "полигон вложен в свой bbox_region" (bbox не должен молча отсекать то, что полигон бы принял). Исключение (50, 77) в test_regions_do_not_overlap НЕ снято: тест проверяет raw-прямоугольники (is_within_bbox/bbox_region), а не итоговый резолв точки — bbox_region(77) геометрически остаётся подмножеством bbox_region(50) как прямоугольник независимо от полигонов, это не тот инвариант, который фикс меняет. Честная непересекаемость теперь проверена на уровне результата region_for_point в test_3052_region_polygon_boundary.py. Замер: ~25 мкс на вызов region_for_point (240k вызовов, точки по всем трём регионам вперемешку) — в пределах бюджета.
This commit is contained in:
parent
811b9ecc8f
commit
30832c27ed
5 changed files with 220 additions and 33 deletions
File diff suppressed because one or more lines are too long
|
|
@ -10,13 +10,21 @@
|
|||
Регион 50 (Московская область) отложен сознательно — обоснование в #2996:
|
||||
10 121 текстовое имя города против 612 у Москвы, вся мина имён — в области.
|
||||
|
||||
#3052: `region_for_point` резолвит по НАСТОЯЩЕЙ границе региона (полигон
|
||||
OSM/Nominatim, `region_boundaries/boundaries.geojson.json`), не по
|
||||
прямоугольнику `bbox_region` — прямоугольники Москвы и области пересекались
|
||||
(Химки/Реутов/Котельники/Люберцы уходили в Москву), полигоны нет.
|
||||
|
||||
Модуль — ЛИСТ дерева импортов: не импортирует ничего из app.* (его читают
|
||||
geocoder / location_index / matching.normalize, циклы недопустимы).
|
||||
geocoder / location_index / matching.normalize, циклы недопустимы). json/
|
||||
pathlib — стандартная библиотека, листовость не нарушают.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import json
|
||||
from dataclasses import dataclass
|
||||
from pathlib import Path
|
||||
|
||||
# bbox = (lat_min, lat_max, lon_min, lon_max) — тот же порядок, что исторический
|
||||
# geocoder.EKB_BBOX_TIGHT (см. is_within_bbox ниже).
|
||||
|
|
@ -90,6 +98,80 @@ def is_within_bbox(lat: float, lon: float, bbox: BBox) -> bool:
|
|||
return lat_min <= lat <= lat_max and lon_min <= lon <= lon_max
|
||||
|
||||
|
||||
# ── #3052: настоящая граница региона (point-in-polygon вместо bbox) ─────────
|
||||
#
|
||||
# Источник — region_boundaries/boundaries.geojson.json: полигоны OSM/Nominatim
|
||||
# (см. поля _source/_license внутри файла, лицензия ODbL требует их хранить).
|
||||
# Парсим один раз на импорте модуля — 27 точек лишний JSON-parse на каждый
|
||||
# вызов region_for_point был бы дороже самого ray casting.
|
||||
#
|
||||
# Ring — одно кольцо GeoJSON (lon, lat) точек: внешний контур ИЛИ дыра.
|
||||
# Полигон/мультиполигон региона хранится как ПЛОСКИЙ список всех его колец
|
||||
# (для MultiPolygon 77 — кольца всех 10 частей вперемешку, у Polygon 50 —
|
||||
# внешнее кольцо + 9 дыр). Плоский список работает благодаря even-odd
|
||||
# правилу: точка внутри региона ⟺ она попадает внутрь НЕЧЁТНОГО числа колец
|
||||
# из списка. Это ОДНОВРЕМЕННО корректно обрабатывает дыры (Москва — дыра в
|
||||
# кольцах региона 50: попадание в кольцо-дыру снимает чётность, снятую
|
||||
# внешним кольцом) и непересекающиеся части мультиполигона (попадание ровно
|
||||
# в одно кольцо — нечётность не портится соседними частями, которые точка не
|
||||
# задевает) — без явного разделения "внешний контур минус дыры".
|
||||
Ring = list[tuple[float, float]]
|
||||
|
||||
_BOUNDARIES_PATH = Path(__file__).parent / "region_boundaries" / "boundaries.geojson.json"
|
||||
|
||||
|
||||
def _flatten_rings(geometry: dict[str, object]) -> list[Ring]:
|
||||
"""Все кольца геометрии (Polygon или MultiPolygon) одним плоским списком."""
|
||||
coords = geometry["coordinates"]
|
||||
if geometry["type"] == "Polygon":
|
||||
polygons = [coords]
|
||||
elif geometry["type"] == "MultiPolygon":
|
||||
polygons = coords # type: ignore[assignment]
|
||||
else:
|
||||
raise ValueError(f"неподдержанный тип геометрии: {geometry['type']}")
|
||||
rings: list[Ring] = []
|
||||
for polygon in polygons:
|
||||
for ring in polygon: # type: ignore[union-attr]
|
||||
rings.append([(pt[0], pt[1]) for pt in ring]) # type: ignore[index]
|
||||
return rings
|
||||
|
||||
|
||||
def _load_region_boundaries() -> dict[int, list[Ring]]:
|
||||
"""code региона → плоский список колец его полигона/мультиполигона."""
|
||||
with _BOUNDARIES_PATH.open(encoding="utf-8") as f:
|
||||
payload = json.load(f)
|
||||
return {
|
||||
int(code): _flatten_rings(entry["geometry"]) for code, entry in payload["regions"].items()
|
||||
}
|
||||
|
||||
|
||||
_REGION_BOUNDARIES: dict[int, list[Ring]] = _load_region_boundaries()
|
||||
|
||||
|
||||
def _point_in_ring(x: float, y: float, ring: Ring) -> bool:
|
||||
"""PNPOLY (W. R. Franklin) ray casting для одного кольца."""
|
||||
inside = False
|
||||
xj, yj = ring[-1]
|
||||
for xi, yi in ring:
|
||||
if (yi > y) != (yj > y) and x < (xj - xi) * (y - yi) / (yj - yi) + xi:
|
||||
inside = not inside
|
||||
xj, yj = xi, yi
|
||||
return inside
|
||||
|
||||
|
||||
def _point_in_region_polygon(code: int, lat: float, lon: float) -> bool:
|
||||
"""True если (lat, lon) внутри настоящей границы региона `code`.
|
||||
|
||||
even-odd по всем кольцам сразу (см. комментарий выше про плоский список).
|
||||
Региона без загруженной границы — граничит только по bbox (см. вызов в
|
||||
region_for_point), сюда такой код не попадает."""
|
||||
inside = False
|
||||
for ring in _REGION_BOUNDARIES[code]:
|
||||
if _point_in_ring(lon, lat, ring):
|
||||
inside = not inside
|
||||
return inside
|
||||
|
||||
|
||||
# Тиры обогащения (строковые ключи — по label'ам _with_budget в estimator).
|
||||
TIER_AVITO_IMV = "avito_imv"
|
||||
TIER_YANDEX_VALUATION = "yandex_valuation"
|
||||
|
|
@ -262,35 +344,55 @@ def _bbox_area(bbox: BBox) -> float:
|
|||
# Порядок обхода для region_for_point: от САМОГО специфичного (маленький
|
||||
# bbox_region) к самому общему — НЕ sorted(REGIONS) по числовому коду.
|
||||
#
|
||||
# Почему код региона как ключ порядка сломался: bbox_region(50) (Московская
|
||||
# область целиком, lat 54.20..56.96/lon 35.14..40.21) геометрически СОДЕРЖИТ
|
||||
# bbox_region(77) (Москва, 55.10..56.10/36.80..38.10) как прямоугольники — а
|
||||
# 50 < 66 < 77 по числу. При обходе `sorted(REGIONS)` регион 50 проверялся бы
|
||||
# ПЕРВЫМ (50 < 77) и забирал бы себе ВСЕ точки Москвы, включая центр
|
||||
# (55.75, 37.62) — она лежит в bbox_region обоих регионов одновременно. Старый
|
||||
# докстринг называл это «на случай, если когда-нибудь пересекутся» — случай
|
||||
# наступил прямо при добавлении региона 50, не гипотетически.
|
||||
# #3052: сам резолв региона идёт по настоящему полигону (see
|
||||
# _point_in_region_polygon), не по прямоугольнику — реальные админ-границы
|
||||
# 50 и 77 не пересекаются (Москва вырезана дырой из полигона области), так
|
||||
# что для КОНЕЧНОГО результата порядок обхода больше не обязателен: у точки
|
||||
# есть ровно один полигон-кандидат, bbox какого региона ни проверяй первым.
|
||||
# Порядок остаётся не как костыль корректности, а как ДЕШЁВЫЙ предварительный
|
||||
# отсев: bbox_region(50) (Московская область целиком, lat 54.20..56.96/
|
||||
# lon 35.14..40.21) геометрически СОДЕРЖИТ bbox_region(77) (Москва,
|
||||
# 55.10..56.10/36.80..38.10) как прямоугольники, а 50 < 77 по числовому коду.
|
||||
# Если проверять регионы в порядке `sorted(REGIONS)`, для точки в центре
|
||||
# Москвы bbox-отсев региона 50 пройдёт ПЕРВЫМ и завернёт в дорогой
|
||||
# point-in-polygon по 1 227 точкам области раньше, чем дело дойдёт до
|
||||
# компактного полигона Москвы (386 точек) — лишняя работа на каждый вызов,
|
||||
# не баг результата (полигон 50 всё равно отвергнет точку — она в дыре), но
|
||||
# systematic overhead на самом частом случае (Москва/область — соседи).
|
||||
#
|
||||
# Площадь bbox_region (см. `_bbox_area`) как ключ сортировки решает это БЕЗ
|
||||
# ручного списка: чем компактнее регион, тем раньше его проверяют, поэтому
|
||||
# вложенный регион (77 внутри 50) всегда выигрывает у объемлющего, а будущий
|
||||
# новый регион сам встанет в верную позицию по своей площади — правку этого
|
||||
# места повторять не придётся.
|
||||
# Площадь bbox_region (см. `_bbox_area`) как ключ сортировки решает и это без
|
||||
# ручного списка: чем компактнее регион, тем раньше его bbox-отсев и (при
|
||||
# совпадении) полигон проверяют, поэтому вложенный по bbox регион (77 внутри
|
||||
# 50) почти всегда получает свою точку дешевле, а новый регион сам встанет в
|
||||
# верную позицию по своей площади.
|
||||
_POINT_LOOKUP_ORDER: tuple[int, ...] = tuple(
|
||||
sorted(REGIONS, key=lambda code: (_bbox_area(REGIONS[code].bbox_region), code))
|
||||
)
|
||||
|
||||
|
||||
def region_for_point(lat: float, lon: float) -> Region | None:
|
||||
"""Регион покрытия, которому принадлежит точка (по bbox_region), или None.
|
||||
"""Регион покрытия, которому принадлежит точка, или None (вне охвата).
|
||||
|
||||
Обход — `_POINT_LOOKUP_ORDER` (компактный bbox_region раньше обширного), а
|
||||
не числовой код региона: код как ключ порядка ломается ровно на паре
|
||||
50/77, см. комментарий у `_POINT_LOOKUP_ORDER`.
|
||||
Два уровня отсева на каждого кандидата (порядок — `_POINT_LOOKUP_ORDER`,
|
||||
компактный bbox_region раньше обширного, см. комментарий там):
|
||||
1. `is_within_bbox` по `bbox_region` — дешёвый прямоугольный предфильтр,
|
||||
НЕ финальный ответ (прямоугольники Москвы и области пересекаются).
|
||||
2. Полигон (`_point_in_region_polygon`) — настоящая граница, решает
|
||||
результат. Если bbox прошёл, а полигон точку не принял (точка в
|
||||
прямоугольнике области, но не в её реальных границах — то есть,
|
||||
например, внутри вырезанной дыры Москвы), идём к следующему
|
||||
кандидату, а не возвращаем None сразу.
|
||||
Региона без загруженного полигона в реестре нет (все REGIONS покрыты
|
||||
boundaries.geojson.json) — на practике до fallback-ветки дело не доходит,
|
||||
но она есть, чтобы новый регион без границы не «пропадал» молча, а
|
||||
работал по старому bbox-поведению до того, как для него добавят полигон.
|
||||
"""
|
||||
for code in _POINT_LOOKUP_ORDER:
|
||||
if is_within_bbox(lat, lon, REGIONS[code].bbox_region):
|
||||
return REGIONS[code]
|
||||
region = REGIONS[code]
|
||||
if not is_within_bbox(lat, lon, region.bbox_region):
|
||||
continue
|
||||
if code not in _REGION_BOUNDARIES or _point_in_region_polygon(code, lat, lon):
|
||||
return region
|
||||
return None
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -50,7 +50,17 @@ def test_regions_do_not_overlap() -> None:
|
|||
— там же настоящий инвариант для вложенной пары: `region_for_point` обязан
|
||||
резолвить точку во ВЛАДЕЮЩИЙ, более специфичный регион по площади bbox, а не
|
||||
по числовому коду). 66 (Урал) физически за тысячи км от 50/77 — для него
|
||||
старый плоский инвариант «никто ни в кого не попадает» остаётся в силе."""
|
||||
старый плоский инвариант «никто ни в кого не попадает» остаётся в силе.
|
||||
|
||||
#3052: этот тест — про СЫРЫЕ прямоугольники (`is_within_bbox` /
|
||||
`bbox_region`), не про итоговый резолв точки. Переход `region_for_point`
|
||||
на настоящий полигон (см. regions._point_in_region_polygon) исключение
|
||||
здесь НЕ снимает: bbox_region(77) геометрически остаётся подмножеством
|
||||
bbox_region(50) как прямоугольник — полигон это не меняет, он работает
|
||||
ПОСЛЕ bbox-предфильтра, а не вместо него. Честный инвариант «точка резолвится
|
||||
ровно в один регион» теперь проверяется на уровне function-результата в
|
||||
test_3051_region_registry_moscow_oblast.py (Химки/Балашиха и девять
|
||||
контрольных точек issue #3052), не здесь."""
|
||||
nested_pairs = {(50, 77), (77, 50)}
|
||||
# Исключение из инварианта обязано опираться на ДОКАЗАННУЮ вложенность, а не
|
||||
# на допущение в докстринге: сначала проверяем, что bbox_region(50)
|
||||
|
|
|
|||
|
|
@ -13,6 +13,12 @@ bbox_region(77) (Москва) как прямоугольники, а 50 < 77
|
|||
Область на дату этого PR — тир обогащения пуст (frozenset()), в deals/listings
|
||||
0 строк (импорт Росреестра из FDW — отдельный PR). Тесты ниже проверяют РЕЕСТР
|
||||
(геометрию/классификацию), не данные.
|
||||
|
||||
#3052: резолв внутри `region_for_point` перестал быть чисто bbox-based —
|
||||
`_POINT_LOOKUP_ORDER` (регрессия ниже) остался прежним дешёвым предфильтром,
|
||||
но финальный ответ теперь даёт настоящий полигон границы региона. Тест на
|
||||
Химки/Балашиху обновлён под это (было «известное ограничение» bbox, теперь —
|
||||
исправленное поведение).
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
|
@ -73,20 +79,20 @@ def test_point_outside_all_regions_is_none() -> None:
|
|||
assert res is None
|
||||
|
||||
|
||||
def test_known_limitation_khimki_balashikha_resolve_to_77_via_bbox() -> None:
|
||||
"""ИЗВЕСТНОЕ ОГРАНИЧЕНИЕ, не баг этого PR: bbox_region(77) — генеральный
|
||||
fallback-net Москвы, по долготе тянется до 38.10 (~13 км восточнее МКАД).
|
||||
Химки (55.91, 37.41) и Балашиха (55.7965, 37.9388) физически лежат внутри
|
||||
ЭТОГО прямоугольника, хотя административно это область. region_for_point
|
||||
(bbox-based) резолвит их в 77; region_by_city (точное имя) — правильно в
|
||||
50 (см. test_region_by_city_resolves_moscow_oblast_cities). Сузить
|
||||
bbox_region(77), чтобы это исправить, — отдельное решение вне скоупа
|
||||
ordering-фикса (риск задеть geocoder/estimator-потребителей 77 без
|
||||
возможности перепроверить их в этом PR)."""
|
||||
def test_khimki_balashikha_resolve_to_50_via_polygon() -> None:
|
||||
"""#3052: bbox_region(77) — генеральный fallback-net Москвы, по долготе
|
||||
тянется до 38.10 (~13 км восточнее МКАД). Химки (55.91, 37.41) и Балашиха
|
||||
(55.7965, 37.9388) физически лежат внутри ЭТОГО прямоугольника, хотя
|
||||
административно это область — до #3052 region_for_point (bbox-based)
|
||||
резолвил их в 77 («известное ограничение», см. issue #3052: 8 647
|
||||
московских лотов уходили в отказ по покрытию, а 11 172 областные сделки
|
||||
ложно попадали в московское ядро). С настоящим полигоном (bbox остался
|
||||
только дешёвым предфильтром — см. regions.region_for_point) обе точки
|
||||
физически вне контура Москвы → резолвятся в 50, как и region_by_city."""
|
||||
khimki = regions.region_for_point(55.91, 37.41)
|
||||
assert khimki is not None and khimki.code == 77
|
||||
assert khimki is not None and khimki.code == 50
|
||||
balashikha = regions.region_for_point(55.7965, 37.9388)
|
||||
assert balashikha is not None and balashikha.code == 77
|
||||
assert balashikha is not None and balashikha.code == 50
|
||||
|
||||
|
||||
# ── 2. region_by_city ─────────────────────────────────────────────────────
|
||||
|
|
|
|||
|
|
@ -0,0 +1,68 @@
|
|||
"""#3052: `region_for_point` резолвит по настоящей границе региона (полигон
|
||||
OSM/Nominatim из `region_boundaries/boundaries.geojson.json`), не по
|
||||
прямоугольнику `bbox_region`.
|
||||
|
||||
До фикса прямоугольники Москвы (77) и области (50) пересекались — Химки,
|
||||
Реутов, Котельники, Люберцы резолвились в Москву (8 647 московских лотов
|
||||
получали отказ по покрытию, 11 172 областные сделки ложно попадали в
|
||||
московское ядро, при том что коридор ДКП фильтрует строго по коду региона).
|
||||
|
||||
Девять контрольных точек ниже — приёмка issue #3052, значения сверены
|
||||
`region_for_point` на реальных полигонах до мержа (ray casting с поддержкой
|
||||
дыр, см. regions._point_in_region_polygon)."""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import os
|
||||
|
||||
os.environ.setdefault("DATABASE_URL", "postgresql+psycopg://test:test@localhost:5432/test")
|
||||
|
||||
from app.services import regions
|
||||
|
||||
# (название, lat, lon, ожидаемый code региона или None)
|
||||
_CONTROL_POINTS: tuple[tuple[str, float, float, int | None], ...] = (
|
||||
("Химки", 55.8970, 37.4297, 50),
|
||||
("центр Москвы", 55.7558, 37.6173, 77),
|
||||
("Ломоносовский проспект", 55.7020, 37.5140, 77),
|
||||
("Серпухов", 54.9147, 37.4147, 50),
|
||||
("Балашиха", 55.7965, 37.9391, 50),
|
||||
("Зеленоград", 55.9840, 37.2140, 77),
|
||||
("Троицк (Новая Москва)", 55.4750, 37.3000, 77),
|
||||
("Екатеринбург", 56.8389, 60.6057, 66),
|
||||
("Пермь", 58.0105, 56.2502, None),
|
||||
)
|
||||
|
||||
|
||||
def test_nine_control_points_resolve_by_real_boundary() -> None:
|
||||
for name, lat, lon, expected_code in _CONTROL_POINTS:
|
||||
res = regions.region_for_point(lat, lon)
|
||||
got_code = res.code if res is not None else None
|
||||
assert got_code == expected_code, (
|
||||
f"{name} ({lat}, {lon}): ожидался регион {expected_code}, получен {got_code}"
|
||||
)
|
||||
|
||||
|
||||
def test_point_outside_all_boundaries_is_none() -> None:
|
||||
"""Пермь — вне всех трёх полигонов (не только вне bbox), инвариант отдельно
|
||||
от общего прогона выше — чтобы регрессия на None была явной сама по себе."""
|
||||
assert regions.region_for_point(58.0105, 56.2502) is None
|
||||
|
||||
|
||||
def test_ekaterinburg_unaffected_by_polygon_switch() -> None:
|
||||
"""Регион 66 физически за тысячи км от 50/77 — переход на полигон не должен
|
||||
был задеть его резолв вообще."""
|
||||
res = regions.region_for_point(56.8389, 60.6057)
|
||||
assert res is not None and res.code == 66
|
||||
|
||||
|
||||
def test_bbox_prefilter_never_rejects_what_polygon_would_accept() -> None:
|
||||
"""Инвариант «полигон вложен в свой bbox_region»: если бы это было не так,
|
||||
дешёвый bbox-предфильтр в region_for_point молча отбрасывал бы точки,
|
||||
которые честный point-in-polygon принял бы — тихая потеря покрытия."""
|
||||
for code, rings in regions._REGION_BOUNDARIES.items():
|
||||
bbox = regions.REGIONS[code].bbox_region
|
||||
for ring in rings:
|
||||
for lon, lat in ring:
|
||||
assert regions.is_within_bbox(lat, lon, bbox), (
|
||||
f"вершина полигона региона {code} ({lat}, {lon}) вне его bbox_region"
|
||||
)
|
||||
Loading…
Add table
Reference in a new issue