fix(best-layouts): address review-bot minor items (#113 PR C)
3 carryover 🟡 items из re-review verdict на a88917f:
1. Coverage % denominator now post-filter (was mixing pre-filter count
с post-filter intersection → artificially low confidence when
user passed exclude/filter list)
2. MAX(snapshot_date) pre-computed via db.scalar() + bind :latest_snap
instead of subquery in WHERE (avoid re-execution per spatial scan)
3. LAYOUT_CONFIDENCE_HIGH_PCT / LAYOUT_CONFIDENCE_MEDIUM_PCT as
module-level constants instead of hardcoded literals
Integration test gap (#4 в re-review) — отдельный follow-up issue,
не в scope этого PR.
Tests: 10/10 best-layouts pass, 43/43 regression OK.
This commit is contained in:
parent
a88917f0cf
commit
8488012f05
2 changed files with 46 additions and 21 deletions
|
|
@ -39,6 +39,11 @@ from app.services.site_finder.layout_signature import area_bin, layout_signature
|
||||||
|
|
||||||
logger = logging.getLogger(__name__)
|
logger = logging.getLogger(__name__)
|
||||||
|
|
||||||
|
# Confidence thresholds (per coverage % of objects with MV velocity data)
|
||||||
|
# Tune via PR if business feedback требует.
|
||||||
|
LAYOUT_CONFIDENCE_HIGH_PCT = 50.0
|
||||||
|
LAYOUT_CONFIDENCE_MEDIUM_PCT = 20.0
|
||||||
|
|
||||||
# Делители velocity: 24 мес → масштаб на указанный window
|
# Делители velocity: 24 мес → масштаб на указанный window
|
||||||
_VELOCITY_DIVISORS: dict[str, float] = {
|
_VELOCITY_DIVISORS: dict[str, float] = {
|
||||||
"last_month": 24.0,
|
"last_month": 24.0,
|
||||||
|
|
@ -139,7 +144,7 @@ _SUPPLY_BATCH_SQL = text("""
|
||||||
)::geography,
|
)::geography,
|
||||||
CAST(:radius_m AS float)
|
CAST(:radius_m AS float)
|
||||||
)
|
)
|
||||||
AND f.snapshot_date = (SELECT MAX(snapshot_date) FROM domrf_kn_flats)
|
AND f.snapshot_date = CAST(:latest_snap AS date)
|
||||||
GROUP BY rb, ab
|
GROUP BY rb, ab
|
||||||
""")
|
""")
|
||||||
|
|
||||||
|
|
@ -279,18 +284,29 @@ def get_best_layouts(
|
||||||
)
|
)
|
||||||
|
|
||||||
# ── Step 5: supply side (батч-запрос) ────────────────────────────────────
|
# ── Step 5: supply side (батч-запрос) ────────────────────────────────────
|
||||||
try:
|
# Pre-compute последний snapshot_date один раз — избегаем subquery на каждый scan.
|
||||||
supply_rows = (
|
latest_snap: dt.date | None = db.scalar(text("SELECT MAX(snapshot_date) FROM domrf_kn_flats"))
|
||||||
db.execute(
|
if latest_snap is None:
|
||||||
_SUPPLY_BATCH_SQL,
|
logger.warning("best_layouts: domrf_kn_flats пустой (нет snapshot_date), supply=0 fallback")
|
||||||
{"center_lon": center_lon, "center_lat": center_lat, "radius_m": radius_m},
|
|
||||||
)
|
|
||||||
.mappings()
|
|
||||||
.all()
|
|
||||||
)
|
|
||||||
except Exception:
|
|
||||||
logger.warning("best_layouts: supply query failed, supply=0 fallback")
|
|
||||||
supply_rows = []
|
supply_rows = []
|
||||||
|
else:
|
||||||
|
try:
|
||||||
|
supply_rows = (
|
||||||
|
db.execute(
|
||||||
|
_SUPPLY_BATCH_SQL,
|
||||||
|
{
|
||||||
|
"center_lon": center_lon,
|
||||||
|
"center_lat": center_lat,
|
||||||
|
"radius_m": radius_m,
|
||||||
|
"latest_snap": latest_snap,
|
||||||
|
},
|
||||||
|
)
|
||||||
|
.mappings()
|
||||||
|
.all()
|
||||||
|
)
|
||||||
|
except Exception:
|
||||||
|
logger.warning("best_layouts: supply query failed, supply=0 fallback")
|
||||||
|
supply_rows = []
|
||||||
|
|
||||||
supply_map: dict[tuple[str, str], int] = {
|
supply_map: dict[tuple[str, str], int] = {
|
||||||
(str(r["rb"]), str(r["ab"])): int(r["units"]) for r in supply_rows
|
(str(r["rb"]), str(r["ab"])): int(r["units"]) for r in supply_rows
|
||||||
|
|
@ -410,22 +426,24 @@ def get_best_layouts(
|
||||||
)
|
)
|
||||||
|
|
||||||
# ── Step 9: data_quality ─────────────────────────────────────────────────
|
# ── Step 9: data_quality ─────────────────────────────────────────────────
|
||||||
|
# Denominator = post-filter set (effective consideration set после exclude/filter).
|
||||||
|
objects_total_after_filter = len(all_obj_ids)
|
||||||
objects_with_data = len(obj_ids_with_data & set(all_obj_ids))
|
objects_with_data = len(obj_ids_with_data & set(all_obj_ids))
|
||||||
coverage_pct = (
|
coverage_pct = (
|
||||||
round(objects_with_data / objects_total_in_radius * 100.0, 1)
|
round(objects_with_data / objects_total_after_filter * 100.0, 1)
|
||||||
if objects_total_in_radius > 0
|
if objects_total_after_filter > 0
|
||||||
else 0.0
|
else 0.0
|
||||||
)
|
)
|
||||||
if coverage_pct >= 50.0:
|
if coverage_pct >= LAYOUT_CONFIDENCE_HIGH_PCT:
|
||||||
confidence: str = "high"
|
confidence: str = "high"
|
||||||
elif coverage_pct >= 20.0:
|
elif coverage_pct >= LAYOUT_CONFIDENCE_MEDIUM_PCT:
|
||||||
confidence = "medium"
|
confidence = "medium"
|
||||||
else:
|
else:
|
||||||
confidence = "low"
|
confidence = "low"
|
||||||
|
|
||||||
data_quality = LayoutDataQuality(
|
data_quality = LayoutDataQuality(
|
||||||
objects_with_velocity_data=objects_with_data,
|
objects_with_velocity_data=objects_with_data,
|
||||||
objects_total_in_radius=objects_total_in_radius,
|
objects_total_in_radius=objects_total_after_filter,
|
||||||
velocity_coverage_pct=coverage_pct,
|
velocity_coverage_pct=coverage_pct,
|
||||||
confidence=confidence, # type: ignore[arg-type]
|
confidence=confidence, # type: ignore[arg-type]
|
||||||
)
|
)
|
||||||
|
|
|
||||||
|
|
@ -3,11 +3,13 @@
|
||||||
Mock-based — не требуют живой БД.
|
Mock-based — не требуют живой БД.
|
||||||
Паттерн mock DB: аналогично test_parcel_competitors.py — dependency_overrides[get_db].
|
Паттерн mock DB: аналогично test_parcel_competitors.py — dependency_overrides[get_db].
|
||||||
|
|
||||||
Порядок вызовов db.execute в get_best_layouts:
|
Порядок вызовов в get_best_layouts:
|
||||||
|
db.scalar() → MAX(snapshot_date) (только когда vel_rows non-empty)
|
||||||
|
db.execute() calls:
|
||||||
1. _PARCEL_CENTROID_SQL → .mappings().first()
|
1. _PARCEL_CENTROID_SQL → .mappings().first()
|
||||||
2. _COMPETITORS_IN_RADIUS_SQL → .mappings().all()
|
2. _COMPETITORS_IN_RADIUS_SQL → .mappings().all()
|
||||||
3. _VELOCITY_BY_ROOM_SQL → .mappings().all()
|
3. _VELOCITY_BY_ROOM_SQL → .mappings().all()
|
||||||
4. _SUPPLY_BATCH_SQL → .mappings().all()
|
4. _SUPPLY_BATCH_SQL → .mappings().all() (пропускается если latest_snap is None)
|
||||||
"""
|
"""
|
||||||
|
|
||||||
from __future__ import annotations
|
from __future__ import annotations
|
||||||
|
|
@ -83,17 +85,22 @@ def _make_db(
|
||||||
id_rows: list[MagicMock] | None = None,
|
id_rows: list[MagicMock] | None = None,
|
||||||
vel_rows: list[MagicMock] | None = None,
|
vel_rows: list[MagicMock] | None = None,
|
||||||
supply_rows: list[MagicMock] | None = None,
|
supply_rows: list[MagicMock] | None = None,
|
||||||
|
latest_snap: dt.date | None = None,
|
||||||
) -> MagicMock:
|
) -> MagicMock:
|
||||||
"""Сконструировать mock Session.
|
"""Сконструировать mock Session.
|
||||||
|
|
||||||
Порядок execute:
|
db.scalar() возвращает latest_snap (MAX snapshot_date) — вызывается перед supply.
|
||||||
|
Порядок db.execute():
|
||||||
1. centroid → .mappings().first()
|
1. centroid → .mappings().first()
|
||||||
2. competitors-in-radius → .mappings().all()
|
2. competitors-in-radius → .mappings().all()
|
||||||
3. velocity → .mappings().all()
|
3. velocity → .mappings().all()
|
||||||
4. supply → .mappings().all()
|
4. supply → .mappings().all() (только если latest_snap is not None)
|
||||||
"""
|
"""
|
||||||
db = MagicMock()
|
db = MagicMock()
|
||||||
|
|
||||||
|
# db.scalar — pre-computed MAX(snapshot_date) для supply query
|
||||||
|
db.scalar.return_value = latest_snap if latest_snap is not None else _TODAY
|
||||||
|
|
||||||
results: list[MagicMock] = []
|
results: list[MagicMock] = []
|
||||||
|
|
||||||
# 1: centroid
|
# 1: centroid
|
||||||
|
|
|
||||||
Loading…
Add table
Reference in a new issue