diff --git a/keel/sim/report.py b/keel/sim/report.py index dea7222..0059980 100644 --- a/keel/sim/report.py +++ b/keel/sim/report.py @@ -100,6 +100,7 @@ "edge_table", "group_trades_by_class", "render_markdown", + "rule_keys", ] # G1: an asset's ONE_HOUR coverage below this many bars (~166 days) is excluded from the pooled @@ -121,6 +122,37 @@ # `edge_table`'s pooled entry key. POOLED_KEY = "__pooled__" + +def rule_keys(rules: list[Rule]) -> list[str]: + """One row key per rule in `rules`, positionally -- unique across the list (#829). + + The key is `"{rule.name}:{asset}"`, which is what every existing reader of these tables + expects, and it stays exactly that whenever it is unique. Only rules that SHARE it -- two + rules of one kind on one asset, e.g. the deployment's $50 and $5 BTC DCA rows -- are told + apart, by their `rules.id` (`"dca#19:BTC"`), or by their 1-based position among the + colliding rules when a hand-built rule has no id. Before this, the later rule overwrote the + earlier one's row, and `group_trades_by_class`, looking both up by the shared key, counted + the survivor twice and the other not at all. + + `edge_table`, `accumulation_table` and `group_trades_by_class` must each be handed the SAME + `rules` list for their keys to agree -- `simulate` passes one list to all three. + """ + bases = [f"{rule.name}:{_asset(rule.product_id)}" for rule in rules] + counts: dict[str, int] = {} + for base in bases: + counts[base] = counts.get(base, 0) + 1 + seen: dict[str, int] = {} + keys: list[str] = [] + for rule, base in zip(rules, bases, strict=True): + if counts[base] == 1: + keys.append(base) + continue + seen[base] = seen.get(base, 0) + 1 + tag = rule.rule_id if rule.rule_id is not None else seen[base] + keys.append(f"{rule.name}#{tag}:{_asset(rule.product_id)}") + return keys + + # A coverage "first_ts starts this much later than requested" margin (30 days) before flagging # partial history -- small pagination/inception-boundary slop shouldn't itself be a gap. _PARTIAL_HISTORY_MARGIN_SEC = 30 * 86400 @@ -166,9 +198,10 @@ def edge_table( convention). A rule is backtested on the series for ITS OWN trading timeframe (`_rule_trading_tf` -- `ONE_HOUR` for the hourly rules, `ONE_DAY` for the daily-native `TurtleBreakout`), with the `ONE_HOUR` series passed as `finer_candles` for intrabar - stop/target resolution when the rule's own timeframe is coarser. Results are keyed - `"{rule.name}:{asset}"` (not bare `rule.name`) so two rules of the same kind bound to - different assets don't collide. `POOLED_KEY` (`"__pooled__"`) holds + stop/target resolution when the rule's own timeframe is coarser. Results are keyed by + `rule_keys` -- `"{rule.name}:{asset}"`, so two rules of the same kind on different assets + don't collide, and disambiguated by rule id when two share one asset (#829). + `POOLED_KEY` (`"__pooled__"`) holds `strategy.stats.summarize()` over every rule's trades in EXIT-TIME order (`_chronological`) -- the pooled sample `build_verdict`'s G2 gate is checked against, and whose drawdown must be a path that happened, not one rule's history followed by the next's. @@ -191,7 +224,7 @@ def edge_table( results: dict[str, BacktestResult] = {} pooled_trades = [] - for rule in rules: + for rule, key in zip(rules, rule_keys(rules), strict=True): if rule.accumulates: continue asset = _asset(rule.product_id) @@ -207,7 +240,7 @@ def edge_table( slippage_pct=slippage_pct, slippage_by_product=slippage_by_product, ) - results[f"{rule.name}:{asset}"] = result + results[key] = result pooled_trades.extend(result.trades) results[POOLED_KEY] = summarize(_chronological(pooled_trades)) @@ -222,7 +255,8 @@ def accumulation_table( slippage_by_product: Callable[[str], Decimal] | None = None, ) -> dict[str, DcaSleeve]: """The edge pass for ACCUMULATING rules (`Rule.accumulates` -- `Dca`), keyed like - `edge_table` (`"{rule.name}:{asset}"`) but never mixed into it (#821). + `edge_table` (`rule_keys`: `"{rule.name}:{asset}"`, disambiguated on collision) but never + mixed into it (#821). Each rule is driven over its asset's `ONE_DAY` series, one completed day at a time: every bar in that series has closed, so `Dca.detect`'s completed-day guard keeps them all, and @@ -238,7 +272,7 @@ def accumulation_table( zero-buy row rather than raising. """ rows: dict[str, DcaSleeve] = {} - for rule in rules: + for rule, key in zip(rules, rule_keys(rules), strict=True): if not rule.accumulates: continue asset = _asset(rule.product_id) @@ -271,7 +305,7 @@ def accumulation_table( qty += buy_qty cost += fill * buy_qty * (Decimal(1) + fee_pct) last_close = daily[-1].close if daily else Decimal("0") - rows[f"{rule.name}:{asset}"] = DcaSleeve.marked(buys, qty, cost, last_close) + rows[key] = DcaSleeve.marked(buys, qty, cost, last_close) return rows @@ -296,14 +330,15 @@ def group_trades_by_class( `build_verdict`'s G2 gate checks against each class's own floor (KB ยง25.5): a low-win/high-R:R trend-follower is judged by the trend floor rather than the global one, while other classes keep the canonical floor. `edge` is `edge_table`'s output; its - per-rule keys are `"{rule.name}:{asset}"` (the `POOLED_KEY` entry is ignored -- it pools + per-rule keys come from `rule_keys(rules)`, so `rules` must be the list `edge_table` was + given (the `POOLED_KEY` entry is ignored -- it pools across *all* classes and so isn't meaningful per-class). A rule whose edge entry is missing is skipped (absent data is a coverage gap, not a crash -- mirrors `edge_table`). Each class's pool is summarised in exit-time order, like `edge_table`'s (`_chronological`). """ trades_by_class: dict[str, list[Trade]] = {} - for rule in rules: - result = edge.get(f"{rule.name}:{_asset(rule.product_id)}") + for rule, key in zip(rules, rule_keys(rules), strict=True): + result = edge.get(key) if result is None: continue trades_by_class.setdefault(promotion_class_of(rule), []).extend(result.trades) diff --git a/tests/sim/test_rule_keys.py b/tests/sim/test_rule_keys.py new file mode 100644 index 0000000..f4bac75 --- /dev/null +++ b/tests/sim/test_rule_keys.py @@ -0,0 +1,117 @@ +"""Two rules of one kind on one asset must not share a row (#829). + +`edge_table` and `accumulation_table` keyed rows `"{rule.name}:{asset}"`, so a second rule of +the same kind on the same asset overwrote the first. On the deployment's paper db that hid a +$50/week BTC DCA behind a $5 one. `group_trades_by_class` looked rules up by the same key, so +both rules then read the SURVIVOR's trades: one rule counted twice in G2's class pool, the +other not at all. A key that is unique already stays exactly as it was. +""" + +from __future__ import annotations + +from decimal import Decimal + +from keel.sim import report +from keel.sim.report import POOLED_KEY, edge_table, group_trades_by_class +from keel.strategy.promotion import promotion_class_of +from keel.strategy.rules.dca import Dca +from tests.sim.test_dca_sleeve import _cadence_days, _market +from tests.sim.test_report import _HOUR, _OneShotRule, _winning_series + +_ZERO = Decimal("0") + + +def _one_shot(product_id: str, trigger_ts: int, rule_id: int | None) -> _OneShotRule: + rule = _OneShotRule( + product_id, trigger_ts, entry=Decimal("100"), stop=Decimal("90"), target=Decimal("120") + ) + rule.rule_id = rule_id + return rule + + +def _dca(budget: str, rule_id: int | None) -> Dca: + rule = Dca("BTC-USD", cadence_days=7, budget_usd=Decimal(budget)) + rule.rule_id = rule_id + return rule + + +def _btc() -> dict: + from keel.types import Granularity + + return {"BTC": {Granularity.ONE_HOUR: _winning_series("BTC")}} + + +def test_two_dca_rules_on_one_asset_get_a_row_each_at_their_own_budget() -> None: + """The deployment's shape: rule 19 at $50 and rule 20 at $5, both on BTC.""" + n = len(_cadence_days()) + + rows = report.accumulation_table( + [_dca("50", 19), _dca("5", 20)], _market(), fee_pct=_ZERO, slippage_pct=_ZERO + ) + + assert sorted(rows) == ["dca#19:BTC", "dca#20:BTC"] + assert (rows["dca#19:BTC"].buys, rows["dca#19:BTC"].cost_usd) == (n, Decimal("50") * n) + assert (rows["dca#20:BTC"].buys, rows["dca#20:BTC"].cost_usd) == (n, Decimal("5") * n) + + +def test_two_edge_rules_on_one_asset_get_a_row_each_and_the_pool_counts_both() -> None: + fires = _one_shot("BTC-USD", _HOUR, 7) + never = _one_shot("BTC-USD", 99 * _HOUR, 8) + + edge = edge_table([fires, never], _btc(), fee_pct=_ZERO, slippage_pct=_ZERO) + + assert sorted(edge) == [POOLED_KEY, "one_shot#7:BTC", "one_shot#8:BTC"] + assert edge["one_shot#7:BTC"].n_trades == 1 + assert edge["one_shot#8:BTC"].n_trades == 0 + assert edge[POOLED_KEY].n_trades == 1 + + +def test_the_class_pool_counts_each_colliding_rule_once() -> None: + """Before #829 both rules read the survivor's (the later rule's, zero-trade) result, so + G2's class pool held 0 trades while the pooled row held 1.""" + fires = _one_shot("BTC-USD", _HOUR, 7) + never = _one_shot("BTC-USD", 99 * _HOUR, 8) + rules = [fires, never] + + edge = edge_table(rules, _btc(), fee_pct=_ZERO, slippage_pct=_ZERO) + by_class = group_trades_by_class(edge, rules) + + assert by_class[promotion_class_of(fires)].n_trades == 1 + + +def test_a_key_that_is_already_unique_is_unchanged() -> None: + btc = _one_shot("BTC-USD", _HOUR, 7) + eth = _one_shot("ETH-USD", _HOUR, 8) + from keel.types import Granularity + + candles = { + "BTC": {Granularity.ONE_HOUR: _winning_series("BTC")}, + "ETH": {Granularity.ONE_HOUR: _winning_series("ETH")}, + } + + edge = edge_table([btc, eth], candles, fee_pct=_ZERO, slippage_pct=_ZERO) + + assert sorted(edge) == [POOLED_KEY, "one_shot:BTC", "one_shot:ETH"] + + +def test_colliding_rules_without_an_id_are_still_told_apart() -> None: + """A hand-built rule has `rule_id=None`; its position in `rules` disambiguates instead, so + no row is ever lost for want of a database id.""" + first = _one_shot("BTC-USD", _HOUR, None) + second = _one_shot("BTC-USD", 99 * _HOUR, None) + + edge = edge_table([first, second], _btc(), fee_pct=_ZERO, slippage_pct=_ZERO) + + assert len([key for key in edge if key != POOLED_KEY]) == 2 + assert edge[POOLED_KEY].n_trades == 1 + + +def test_the_accumulation_section_renders_both_rows() -> None: + rows = report.accumulation_table( + [_dca("50", 19), _dca("5", 20)], _market(), fee_pct=_ZERO, slippage_pct=_ZERO + ) + + lines = report._render_accumulation_section(rows) + + data = [line for line in lines if line.startswith("| dca")] + assert [line.split("|")[1].strip() for line in data] == ["dca#19:BTC", "dca#20:BTC"]