1b971ac72d
Correct the multi-rebalance backtest accounting so ending wealth is capital + price_pnl + dividend_income with no double counting: - price_pnl now measures equity change excluding dividends (was reusing ending holdings value as 'price profit') - dividend proxy is included in final_value and net_return, exposed as dividend_method=final_holdings_yield_proxy - regression tests: flat price => zero price_pnl; flat + dividend => dividend-only return; rising no-dividend => correct bucket P&L; multi-rebalance accounting identity - UI (result card + saved-run history) labels dividends as ประมาณการปันผล (Proxy) and shows descriptive non-PIT badge when leakage_guard=false Backend 239 tests passed; targeted backtest 11 passed; frontend build, npm audit (0), static scan and diff check passed; fresh independent review deleg_10918fed passed with empty blocker arrays. Backtest remains descriptive non-PIT (leakage_guard=false) with the default current-score scorer.
190 lines
14 KiB
Markdown
190 lines
14 KiB
Markdown
# Audit + Implementation Plan — SET50 Alternative Data Platform
|
||
|
||
Date: 2026-08-26 · Branch: main (clean tree, @ 6e78b6a)
|
||
Scope: 4 questions (dead logic / backend↔frontend contract / methodology / code hygiene) + new
|
||
point-in-time backtest feature for factor-weight learning.
|
||
|
||
---
|
||
|
||
## 0. Executive summary
|
||
|
||
The app works and 205 tests pass (`PYTHONPATH=. python -m pytest tests -q`). But the **architectural
|
||
core has a real flaw**: a declarative `FACTORS`/`THEMES` framework was built, yet the **actual scoring
|
||
by-passes it** with hand-written blocks in `dashboard._theme_surprises`. The registry only drives the
|
||
*sources table*; it does **not** drive the analysis. Every new collector needs manual scoring code — the
|
||
exact thing `factors.py` claims to have eliminated. This is the root cause behind nearly every finding
|
||
below, including the source-count confusion and the hard-to-maintain scoring.
|
||
|
||
Findings triple-confirmed by 3 independent read-only subagent audits (2026-08-26) plus my own reading.
|
||
Exact counts established: **11 FACTORS entries → 5 fetch-module groups → 6 rendered source rows**;
|
||
no code path yields 7 (the user's "7" is registry entries labeled BOT / category conflation).
|
||
|
||
---
|
||
|
||
## 1. Q1 — Built-but-unused / misaligned code
|
||
|
||
### 1.1 DEAD METADATA: the declarative factor framework is not what computes scores [HIGH]
|
||
- `factors.py`: `FACTORS`, `_FETCH_MODULE`, `factor_value()`, `normalize()`, `z_score()`
|
||
- `themes.py`: `THEMES` (dict w/ per-theme factor weights), `list_themes()`, `Theme` dataclass
|
||
- **Evidence:** grep shows `factor_value`, `normalize`, `z_score`, `list_themes` are referenced
|
||
**nowhere** outside their own definitions. Nothing iterates `THEMES` to compute a surprise
|
||
(`grep THEMES` in app/*.py → only the definition). `factor_value` is dead. The `weight`/`sign`/
|
||
`value_key` fields in `FACTORS` are **never consumed by scoring** — only the *sources table*
|
||
(`dashboard._build_sources`) reads `FACTORS` for label/source/frequency.
|
||
- **Purpose misalignment:** `factors.py` module docstring promises "ADDING A FACTOR … requires NO
|
||
change to any scoring function." **False in practice** — see 1.2.
|
||
|
||
### 1.2 TWO parallel scoring systems (the big one) [HIGH]
|
||
The real analysis lives in hand-written code:
|
||
- `dashboard._theme_surprises` (z-scores + `_norm(...)` per theme, all inline)
|
||
- `dashboard._build_board` → `themes.build_siamchart_score` + `combine_score` + `quality_within_theme`
|
||
- The `THEMES` factor weights (e.g. auto NPL weight −0.6, banks macro_investment 1.0) are **ignored**;
|
||
the effective weights are the inline math in `_theme_surprises` (e.g. auto: `new_car_sales_yoy` then
|
||
`npl` subtract block).
|
||
- **Consequence:** editing a factor's weight in `THEMES`/`FACTORS` does nothing; you must edit the
|
||
hand-written block. Docs and `symbol_breakdown`'s "transparent" notes therefore can drift from truth.
|
||
|
||
### 1.3 `/api/v1/simulation` recomputes a THIRD, different scoring path [HIGH]
|
||
- `__init__.py:567-662` re-derives scores **inline**: only 3 themes, `auto_sign`/`en_sign` = binary
|
||
±1 (not the real surprise), **no** `quality_within_theme`, **no** momentum (R2), **no** dividend
|
||
screen (R5). It even pulls its own `auto_credit`/`energy_thai` cache keys instead of the board.
|
||
- **Result:** the "จำลอง" (simulation) and "Backtest" panes can disagree with the "ตารางหุ้น" board on
|
||
which names rank highest — same bug class as the frontend mismatch the user is worried about, but
|
||
*inside the backend*. This is the strongest "doesn't match its purpose" find.
|
||
|
||
### 1.4 `bank_npl` IS wired into banks score — but via hand-code, not the registry [MEDIUM]
|
||
- Not dead: `dashboard.py:276-281` blending it into `banks_s` **is live** (I initially flagged it dead,
|
||
then verified it is reached). It **does** affect the banks surprise and hence the board.
|
||
- But the blend ignores `FACTORS['bank_npl']['weight']`/`sign`/`value_key` — it hardcodes
|
||
`(bnpl - 0.5)/3.0` capped at 0.5. If `bank_npl` were removed from `FACTORS`, the sources row would
|
||
drop but the scoring block would silently keep running (and vice-versa). **Registry ↔ scoring are
|
||
unlinked.**
|
||
|
||
### 1.5 Minor dead/vestigial code [LOW]
|
||
- `themes.list_themes()` + `Theme` dataclass: **unused** (legacy of the 3-theme era).
|
||
- `backtest.BacktestResult.rebalances` is set to `len(dates)` but the loop `break`s after the **first**
|
||
date → the number is **misleading** (see 3.1).
|
||
- `simulation.load_price_snapshot()` vs `backtest`'s own loader — two snapshot loaders; verify they read
|
||
the same file family.
|
||
|
||
---
|
||
|
||
## 2. Q2 — Backend↔frontend contract
|
||
|
||
### 2.1 Source count: NOT hardcoded — already derived [INFO — corrects the assumption]
|
||
- Frontend count = `dashboardSources.value.length` (`App.vue:54`) → purely derived from backend
|
||
`sources` array. **No hardcoded "5" or "7" anywhere.**
|
||
- Backend `_build_sources` derives rows from `FACTORS` grouped by fetch module → currently **6 unique
|
||
source rows** (7 factors → 6 rows because `auto_npl`+`bank_npl` share `BOT FI_NP_003_S2`, and
|
||
`macro_*` collapse under `macro_thai`).
|
||
- **"7 vs 5"** = the user is counting differently (7 factor keys vs 6 distinct providers vs the 20+
|
||
thing). The right fix is to make the **displayed count unambiguous** — show "N ปัจจัย · M แหล่ง" and
|
||
add a factor-level detail, so the number can't be misread.
|
||
|
||
### 2.2 Real contract gaps (what to verify at runtime, not trust) [MEDIUM]
|
||
- Frontend renders `s['จาก']`, `s['แหล่ง']`, `s['ข้อมูล']`, `s['ความถี่']`, `s['อัปเดตครั้งต่อไป']`,
|
||
`s['dึงมาเมื่อ']`. The backend emits these exact Thai keys — **good**, but the `v-for` row uses
|
||
`key="i"` (index) — fine here since static, but brittle if sources reorder/refresh.
|
||
- Frontend's `themes`/`combinedBoard` come from `/api/v1/themes`; **board** from `/api/v1/dashboard`;
|
||
`simulation` from `/api/v1/simulation` — three endpoints that **each recompute** instead of sharing
|
||
one canonical payload. The board and simulation can diverge (see 1.3).
|
||
- **Verification still needed:** the last rendered-frontend check was against a stale/refused Vite
|
||
(5173). The contract is *derived-correct now*, but I will re-render to confirm rows/columns show 6
|
||
and the board == simulation ordering.
|
||
|
||
### 2.3 Recommended contract cleanup
|
||
- Make `/api/v1/simulation` and `/api/v1/backtest` **call the same `RealDashboard.build()` board**
|
||
(pass a `score_fn` that returns the live board) instead of re-deriving. One source of truth.
|
||
|
||
---
|
||
|
||
## 3. Q3 — Methodology: can the analysis improve? (incl. the new backtest feature)
|
||
|
||
### 3.1 Current backtest is NOT point-in-time — it leaks the future [CRITICAL]
|
||
- `backtest.run_backtest` uses `default_scores(syms)` = **today's** combined score for **every**
|
||
rebalance date, including 2024. So a 2024→2026 backtest "invests in 2024" using **2026 knowledge**
|
||
(price, fundamentals, theme surprises). Sign is therefore meaningless as evidence.
|
||
- `rebalances` counts all dates but it allocates **once then holds** (`break` at line 167). Mislabeled.
|
||
- Dividend = `cost × current_yield` — a rough proxy, not a real per-period payment.
|
||
|
||
### 3.2 What the user's new feature needs (the Dec 10 → Nov 30 example)
|
||
Goal: **learn factor weights from history** — e.g. does a rising "children/population" factor lift the
|
||
relevant SET50 names the following year? If yes, weight that factor up.
|
||
|
||
Design (phased, honest about PIT):
|
||
1. **Vintages / point-in-time factor series.** For each alternative factor, keep the *as-published*
|
||
series with a `published_at` (release date), not just the latest value. (The repo already has a
|
||
`vintages` store + `event_study` infra — reuse that pattern, which already guards
|
||
`require_price_known_at`.)
|
||
2. **Factor→forward-return attribution.** At each monthly rebalance `t`, using only data published
|
||
`≤ t - lag` (e.g. lag = release cadence), rank symbols by each factor's z-score; measure the
|
||
forward 1–12m return of the top-minus-bottom spread. Aggregate IC (information coefficient) per
|
||
factor across the window.
|
||
3. **Weight update rule (the user's ask).** Start from current weights; after each validation year,
|
||
raise/lower a factor weight proportional to its realized predictive IC (e.g.
|
||
`w' = w * (1 + shrink * IC)`), clamped to sane bounds. This is the "if population up → children
|
||
stocks up → weight up" learning loop, made explicit and testable.
|
||
4. **Honesty guardrails:**
|
||
- apply release-date lag (no lookahead by construction)
|
||
- keep a **holdout year** untouched by the weight learning to avoid overfit
|
||
- report IC spread with confidence (n symbols, EM is noisy)
|
||
- the multi-rebalance engine must **actually rebalance** (sell/re-buy), not allocate-once
|
||
- real dividend handling per payout date (or clearly call it a proxy)
|
||
|
||
### 3.3 Methodology improvements worth doing (independent of the backtest)
|
||
- **Unify scoring**: one function computes theme surprise from `FACTORS`+`THEMES` registry (kill the
|
||
parallel hand-code). This *alone* removes the biggest correctness risk.
|
||
- **Quantile/IC instead of absolute z**: the regime gate and LONG/SHORT bars already use quartiles;
|
||
apply the same normalization consistently to components.
|
||
- **Model the "children/population" style factor properly**: it's a **slow-moving thematic tilt**, not a
|
||
high-frequency surprise. It should enter as a **structural theme weight / industry tilt** (longer
|
||
horizon), not as a monthly z-score — otherwise it will rank as noise.
|
||
- **Frequency-honest mixing**: `THEME_FREQUENCY` is declared but never enforced; factor values of
|
||
different cadence are combined as if same-`as_of`. Align to the slowest, or carry real `as_of`.
|
||
|
||
---
|
||
|
||
## 4. Q4 — Code hygiene / clarity (no behavior change)
|
||
|
||
- `__init__.py` is **804 lines** — split route handlers into a `routes/` package (or at least pull the
|
||
three big recompute endpoints into one `build_dashboard_payload()`).
|
||
- `dashboard.py` and `themes.py` both define surprise math; consolidate into `themes.py` registry-driven
|
||
scorer; `dashboard.py` keeps only assembly.
|
||
- Remove dead exports: `list_themes()`, `Theme`, `factors.factor_value/normalize/z_score` if unused
|
||
after unification (keep whatever becomes consumed).
|
||
- `backtest.py`: rename `rebalances`→`planned_rebalances`, add `actual_rebalances`; add `leakage_guard`
|
||
flag that is `False` until a PIT `score_fn` is supplied.
|
||
- `_build_sources`: the in-function `_source_label` / `_cadence` dicts are fine but should move to the
|
||
registry (each FACTOR carries its own `source_label`, cadence) so sources can't drift from factors.
|
||
- Tests: currently **must** run with `PYTHONPATH=.` (`ModuleNotFoundError: app` otherwise) → add
|
||
`tests/conftest.py` inserting the backend dir, and a CI-safe `pytest` config. 205 tests pass but none
|
||
cover: backtest look-ahead guard, sources-count derivation, or the simulation↔board consistency.
|
||
|
||
---
|
||
|
||
## 5. Proposed implementation order (each step independently shippable + tested)
|
||
|
||
| # | Change | Risk | Acceptance | Status |
|
||
|---|--------|------|-----------|--------|
|
||
| P0 | **Unify scoring** — registry-driven `compute_theme_surprises()` from `FACTORS`+`THEMES`; board calls it; delete parallel blocks | HIGH | board scores change → snapshot before/after; all 205 pass; sources unchanged | ✅ DONE (Option B: registry is source of truth; board re-ranked; parameters recentred) |
|
||
| P1 | **Simulation reuses board** — `/api/v1/simulation` calls the same `RealDashboard` score (kill 3rd path) | MED | sim ordering == board ordering | ✅ DONE (verified live: sim picks PTT = top board) |
|
||
| P2 | **Source count clarity** — payload adds `factor_count` + per-factor detail; frontend shows "N ปัจจัย · M แหล่ง" | LOW | rendered shows both; 7≠5 confusion gone | ✅ DONE (`source_summary{factor_keys:11, rows:6}` live) |
|
||
| P3 | **Backtest PIT honesty** — real multi-rebalance engine, release-lag score_fn, actual rebalances, leakage flag, real-ish dividend | HIGH | 2024 backtest no longer uses 2026 scores; IC attribution added | 🟡 Partial — multi-rebalance accounting invariants fixed; public endpoint still uses current scores historically (`leakage_guard=false`); dividend remains an explicitly disclosed final-holdings yield proxy |
|
||
| P4 | **Factor-weight learning loop** (user's new feature) — IC-based weight update + holdout | MED | a "before/after weight" is produced per factor per year | ✅ DONE (momentum IC=0.012 live; machinery ready — macro/demographic blocked on historical vintages) |
|
||
| P5 | **Hygiene** — split `__init__.py`, drop dead code, conftest/PYTHONPATH, registry-owned source metadata | LOW | diff is behavior-neutral | 🟡 Partial — dead code removed + conftest; `__init__` split + registry-owned metadata deferred |
|
||
|
||
**Canonical repo conventions:** plan-first, fix only reported bug, log to engineering-log.md + HANDOFF.md
|
||
per session (engineering-handoff skill), push WIP early to gitea origin.
|
||
|
||
---
|
||
|
||
## 6. Open decisions for the user
|
||
|
||
1. **Scope now:** do I execute P0–P2 (correctness + contract) in this pass, and leave P3/P4 (the
|
||
point-in-time backtest + weight learning) as a separately planned feature? Or attempt the full stack?
|
||
2. **Backtest horizon feature (P4):** the "children/population" factor — implement as a **long-horizon
|
||
thematic tilt** weight (my recommendation) vs a monthly surprise?
|
||
3. **Acceptable score change:** P0 will shift board scores (registry weights may differ from today's
|
||
hand-math). OK to re-baseline, or must output stay bit-identical (forcing registry weights = current
|
||
hand-math)?
|